Merge lp:~edb/quam-plures/excerpt_more_bug into lp:quam-plures

[EdB]

Link. I should have a link. But Yabs already knows about the bug and I got this feeling that everyone else getting this email won't do anything about it anyway so I guess it ain't so bad if I don't dig out a link.

[lp:~edb/quam-plures/excerpt_more_bug]

7588. By EdB on 2011-01-18

improved code on both affected files

[EdB (edb)]

Re-did both files based on forum stuff. Got rid of extraneous (s and )s, moved some bits inside the appropriate IF thing, then moved the whole IF thing up a level. And got rid of $r cuz I never did like $r :)

[Kimberly (kimberly-netweb360)]

> And got rid of $r cuz I never did like $r :)

$r is used in the _file.class.php in making the image tags. Is $r local to that class?

[EdB (edb)]

Yeah $r is used everywhere but is always specific to whatever function is happening at the moment. In this case it served one purpose: if( !$r ) or some such, so I replaced that with if( ! $this->excerpt ) which is what $r was and how it was displayed anyway. Can't see the value in defining a variable that gets used only once IF the definition isn't required to get through the one instance of use.

There are cases where it has to be done that way. $r = function( param, param ) where the function will return a value or empty but the function won't work for later stuff. No examples of it in my head right now but they're out there.

Anyway for this case it didn't matter and was kinda like adding more parentheses: there because it looks good but doesn't add value and isn't required.

[Foppe Hemminga (foppe)]

I'm fine with this. Two remarks:
- I haven't looked at the style guide for years but I'd space out the .
(dot) as string concatenator.
- After some thought the $r should have some reason to be there and it
could be quit sophisticated like
 $r = $this->excerpt;
might instantiate / activated / loaded into memory the object or member
while
 if( False && $this->excerpt )
definately will not because it will halt at False
I'm not completely sure what
 ! $this->excerpt
does. It may think "OK I have no reference to this member so let me
validate to True" inside the if.
But you are checking if an instantiated version of the object with
member $this->excerpt holds any information. Not if it exists or not.

And no, I have not a frikking clue how PHP handles these subtleties.
Blueyed may know.

--Foppe

EdB schreef op ma 17-01-2011 om 00:33 [+0000]:
> EdB has proposed merging lp:~edb/quam-plures/excerpt_more_bug into lp:quam-plures.
>
> Requested reviews:
> Quam Plures Core Team (quam-plures-core)
>
> For more details, see:
> https://code.launchpad.net/~edb/quam-plures/excerpt_more_bug/+merge/46432
>
> Link. I should have a link. But Yabs already knows about the bug and I got this feeling that everyone else getting this email won't do anything about it anyway so I guess it ain't so bad if I don't dig out a link.
> verschillen tussen bestanden bijlage (review-diff.txt)
> === modified file 'qp_inc/items/model/_item.class.php'
> --- qp_inc/items/model/_item.class.php 2010-12-31 12:12:03 +0000
> +++ qp_inc/items/model/_item.class.php 2011-01-17 00:32:58 +0000
> @@ -1026,14 +1026,17 @@
>
> $r = $this->excerpt;
>
> - if( !empty($r) )
> + if( !empty( $r ) )
> {
> echo $params['before'];
> echo format_to_output( $this->excerpt, $params['format'] );
> if( !empty( $params['excerpt_more_text'] ) )
> {
> echo $params['excerpt_before_more'];
> - echo '<a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
> + if( $r != $this->content )
> + {
> + echo ' <a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
> + }
> echo $params['excerpt_after_more'];
> }
> echo $params['after'];
>
> === modified file 'qp_templates/_item_content.inc.php'
> --- qp_templates/_item_content.inc.php 2010-12-31 12:12:03 +0000
> +++ qp_templates/_item_content.inc.php 2011-01-17 00:32:58 +0000
> @@ -77,6 +77,10 @@
> case 'posts-next': // next page 2, 3, etc
> default:
> $content_mode = $Blog->get_setting('main_content');
> + if( ( $content_mode == 'excerpt' ) && ( $disp == 'single' ) )
> + {
> + $content_mode = 'full';
> + }
> }
> }
>
>

[Tilman Blumenbach (tblue)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> - I haven't looked at the style guide for years but I'd space out the .
> (dot) as string concatenator.
Most of the time, we don't seem to do that. We need our own definitive
style guide, I guess.

> - After some thought the $r should have some reason to be there and it
> could be quit sophisticated like
> $r = $this->excerpt;
> might instantiate / activated / loaded into memory the object or member
> while
> if( False && $this->excerpt )
> definately will not because it will halt at False
Personally, I think that code is the result of copy-pasting and was not
meant for optimizing anything. Also, the variable's contents should
already have been loaded into memory anyway since the script is being
executed by the time that expression is evaluated.

> I'm not completely sure what
> ! $this->excerpt
> does. It may think "OK I have no reference to this member so let me
> validate to True" inside the if.
> But you are checking if an instantiated version of the object with
> member $this->excerpt holds any information. Not if it exists or not.
If ItemLight::excerpt is not declared explicitly, PHP displays a notice
and the expression evaluates to true (the member variable is being
handled as it were set to NULL). If it is set, then the result of the
expression depends on the member's value (false if NULL, empty string,
empty array, or 0. True otherwise).

As ItemLight::excerpt *is* declared explicitly, the expression should work.

The clean way to check if a variable is set it to use isset() or -- with
some caution -- empty(). But that's not an issue here (unless somebody
unsets() ItemLight::excerpt...).

NOTE: The diff seems to have changed since you submitted your message,
Foppe. :-)

 review +1
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNOdRSAAoJEJBklXsywEyhP/kH/iayyRPX/V270qn1rGKazjN9
07Buf2mL1KPwyullPJCmCzcwYTnF8SGAbz8XnG1DrDq/KlU82QcVK46DhcKPPK4W
rMFpXwdr5cUKMoZ+rihejCLi6+sJkVAuoIvYqna3KYEZ6ZxV6LbxeDX5xKM0BSkP
5i4Fnz4+U1UBO7e5FN5xNQ5Y+YBgTiH7wVhwIPzfwEGiBwtSxbh+EsG8prLsbq4J
rp7lZ9HK0VZUu7NNdyWtoRoKpk7e7maclbAbJgkmmAFh+xqOw/78ZfOhlaXLDJYh
GWwrbtRK0yih3DYYj0BIINQAPzUHBfLHoDpCQcVWc5+6/4X7/HlfDI5a4RTI6HA=
=BXzH
-----END PGP SIGNATURE-----

review: Approve

[Tilman Blumenbach (tblue)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> - I haven't looked at the style guide for years but I'd space out the .
> (dot) as string concatenator.
Most of the time, we don't seem to do that. We need our own definitive
style guide, I guess.

> - After some thought the $r should have some reason to be there and it
> could be quit sophisticated like
> $r = $this->excerpt;
> might instantiate / activated / loaded into memory the object or member
> while
> if( False && $this->excerpt )
> definately will not because it will halt at False
Personally, I think that code is the result of copy-pasting and was not
meant for optimizing anything. Also, the variable's contents should
already have been loaded into memory anyway since the script is being
executed by the time that expression is evaluated.

> I'm not completely sure what
> ! $this->excerpt
> does. It may think "OK I have no reference to this member so let me
> validate to True" inside the if.
> But you are checking if an instantiated version of the object with
> member $this->excerpt holds any information. Not if it exists or not.
If ItemLight::excerpt is not declared explicitly, PHP displays a notice
and the expression evaluates to true (the member variable is being
handled as it were set to NULL). If it is set, then the result of the
expression depends on the member's value (false if NULL, empty string,
empty array, or 0. True otherwise).

As ItemLight::excerpt *is* declared explicitly, the expression should work.

The clean way to check if a variable is set it to use isset() or -- with
some caution -- empty(). But that's not an issue here (unless somebody
unsets() ItemLight::excerpt...).

NOTE: The diff seems to have changed since you submitted your message,
Foppe. :-)

 +1
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNOdTrAAoJEJBklXsywEyhy6UH/AiVMkx/xsjQ1gQKQOwASCC0
uqU8BD+UanIPLQc0R6w0apNv6jvFWTO9BctYP6tCjkIJjdTmb2QHQnkfNDjoQNIr
gseUh9IR0bsds1X27366EnXg5zVc7kwScGgx/ZYiKmA0h1HdnA8LgljenR/Gf1r1
f18MWuVK0+xJYqDTp39DEsa7lUD0hlgQlkmINNibc2XrbWelhA1YaN7oANzyVRc/
3y6rf3z54rFiwPETZ7lipHrKnT84YlLQ2e6vjiqz2B1qAgr5BRY8joun92Qvuogo
ha7SwCUxD/C3gLo8QQpiV2qpQGK/zEjJPBJdeHRWmDhcaq/6oLErBJZYIrjWqW4=
=6apm
-----END PGP SIGNATURE-----

[Tilman Blumenbach (tblue)]

Oops. Sorry for the mess. Still experimenting...

[EdB (edb)]

Lemme see if I can recall all...

The $r = $this->excerpt is gone, and the if( !$r ) is now if( ! $this->excerpt ). I tested by throwing a die() in there and seeing what happened with a post with no excerpt content. An attached image and a title, and no actual text. It didn't die so I figured - as I kinda expected - it works.

Do I have a if( false && something ) in there? If so this branch is junk and won't be fixed for I dunno how long given that Mr Laptop is dumber than a box of rocks.

Oh yeah the dot thing. Pretty much everywhere in the code it is echo $this.$that.' '.$the_other_thing so I figure go with that ... or plan on changing everything to some other way. I reckon if there is a good reason to change then we should but I don't know what it is. And I'm naturally lazy :)

[Foppe Hemminga (foppe)]

Follow up on this one:
I asked Daniel and he agrees with your commit
 if( ! $this->excerpt )
part. Worst case $this->excerpt would be NULL and in PHP `if( ! NULL )`
validates to True, which is what you are looking for.

--Foppe

Foppe Hemminga schreef op vr 21-01-2011 om 12:02 [+0000]:
> I'm fine with this. Two remarks:
> - I haven't looked at the style guide for years but I'd space out the .
> (dot) as string concatenator.
> - After some thought the $r should have some reason to be there and it
> could be quit sophisticated like
> $r = $this->excerpt;
> might instantiate / activated / loaded into memory the object or member
> while
> if( False && $this->excerpt )
> definately will not because it will halt at False
> I'm not completely sure what
> ! $this->excerpt
> does. It may think "OK I have no reference to this member so let me
> validate to True" inside the if.
> But you are checking if an instantiated version of the object with
> member $this->excerpt holds any information. Not if it exists or not.
>
> And no, I have not a frikking clue how PHP handles these subtleties.
> Blueyed may know.
>
> --Foppe
>
> EdB schreef op ma 17-01-2011 om 00:33 [+0000]:
> > EdB has proposed merging lp:~edb/quam-plures/excerpt_more_bug into lp:quam-plures.
> >
> > Requested reviews:
> > Quam Plures Core Team (quam-plures-core)
> >
> > For more details, see:
> > https://code.launchpad.net/~edb/quam-plures/excerpt_more_bug/+merge/46432
> >
> > Link. I should have a link. But Yabs already knows about the bug and I got this feeling that everyone else getting this email won't do anything about it anyway so I guess it ain't so bad if I don't dig out a link.
> > verschillen tussen bestanden bijlage (review-diff.txt)
> > === modified file 'qp_inc/items/model/_item.class.php'
> > --- qp_inc/items/model/_item.class.php 2010-12-31 12:12:03 +0000
> > +++ qp_inc/items/model/_item.class.php 2011-01-17 00:32:58 +0000
> > @@ -1026,14 +1026,17 @@
> >
> > $r = $this->excerpt;
> >
> > - if( !empty($r) )
> > + if( !empty( $r ) )
> > {
> > echo $params['before'];
> > echo format_to_output( $this->excerpt, $params['format'] );
> > if( !empty( $params['excerpt_more_text'] ) )
> > {
> > echo $params['excerpt_before_more'];
> > - echo '<a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
> > + if( $r != $this->content )
> > + {
> > + echo ' <a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
> > + }
> > echo $params['excerpt_after_more'];
> > }
> > echo $params['after'];
> >
> > === modified file 'qp_templates/_item_content.inc.php'
> > --- qp_templates/_item_content.inc.php 2010-12-31 12:12:03 +0000
> > +++ qp_templates/_item_content.inc.php 2011-01-17 00:32:58 +0000
> > @@ -77,6 +77,10 @@
> > case 'posts-next': // next page 2, 3, etc
> > default:
> > $content_mode = $Blog->get_setting('main_content');
> > + if( ( $content_mode == 'excerpt' ) && ( $disp == 'single' ) )
> > + {
> > + $content_mode = 'full';
> > + }
> > }
> > }
> >
> >
>
>

[Yabs (yabs)]

Ok, I'm lost, is this approved or not approved?

Afwas : could I ask you to use "needs information" or "needs fixing" or "approved" instead of "comment only" when you raise points like this?

¥

[Tilman Blumenbach (tblue)]

I already approved this change, looks good to me from a code point of view... Not tested, though. What could go possibly wrong? ;-)

review: Approve (again)

[EdB (edb)]

I'm the one who didn't bother with a link and dammit I wish I had a link to the forum thread. The problem was if you put excerpts on the page it always put a "read more" link even though the post didn't need one or you were already on the permalink page. I forget, but once I figured out the nature of the bug Yabs found it was easy to test and reproduce, then not see after this fix.

Did you mean "what (else) could possibly go wrong?"?

Found the link ... on the other puter. Doh!

http://forums.quamplures.net/viewtopic.php?f=16&t=679

[Tilman Blumenbach (tblue)]

> Did you mean "what (else) could possibly go wrong?"?

Yes. I *knew* I had misplaced that word.

Anyway... It's late here (02:28 AM), so I'm not going to test this branch right now, but possibly later today.

[lp:~edb/quam-plures/excerpt_more_bug]

7589. By EdB on 2011-02-04

update from core

[Foppe Hemminga (foppe)]

[Approved]
Can't find second mail I apparently did not post.
I checked the $r issue with blueyed. He's OK with EdB proposal.

[quote blueyed]
        EdB's check is fine.

        Both variants check for the "tueness" of $excerpt, and you will
        get a
        notice if $excerpt is not set in both cases.

        The difference however is that the first method assigns to $r,
        which is typically being used as the return value of a method.
        In case this does not interfere, it's fine.

        In case $excerpt is not set on the instance, it would assign
        NULL to $r, which is the same as when comparing $this->excerpt
        directly (also NULL in this case).
[/quote]
Yabs schreef op do 03-02-2011 om 17:42 [+0000]:
> Ok, I'm lost, is this approved or not approved?
>
> Afwas : could I ask you to use "needs information" or "needs fixing" or "approved" instead of "comment only" when you raise points like this?
>
> ¥

[Tilman Blumenbach (tblue)]

Glad you approve -- but it would even better if you could use the dropdown right under the textarea you are using to submit your message to choose your review type. It's the "Review" dropdown on the left-hand side. :)

In case you are using the email interface, just include " review +1" somewhere in your message (on a new line, note the leading space).

Have fun.

[Foppe Hemminga (foppe)]

 review +1

(Formal approval through email)
Thanks Tblue

review: Approve

=== modified file 'qp_inc/items/model/_item.class.php'
--- qp_inc/items/model/_item.class.php	2010-12-31 12:12:03 +0000
+++ qp_inc/items/model/_item.class.php	2011-02-04 03:27:51 +0000
@@ -1024,20 +1024,19 @@
 			}
 		}
 
-		$r = $this->excerpt;
-
-		if( !empty($r) )
+		if( !empty( $this->excerpt ) )
 		{
 			echo $params['before'];
 			echo format_to_output( $this->excerpt, $params['format'] );
-			if( !empty( $params['excerpt_more_text'] ) )
+			if( !empty( $params['excerpt_more_text'] ) && $this->excerpt != $this->content )
 			{
 				echo $params['excerpt_before_more'];
-				echo '<a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
+				echo ' <a href="'.$this->get_permanent_url().'">'.$params['excerpt_more_text'].'</a>';
 				echo $params['excerpt_after_more'];
 			}
 			echo $params['after'];
 		}
+
 	}
 
 

=== modified file 'qp_templates/_item_content.inc.php'
--- qp_templates/_item_content.inc.php	2010-12-31 12:12:03 +0000
+++ qp_templates/_item_content.inc.php	2011-02-04 03:27:51 +0000
@@ -77,6 +77,10 @@
 		case 'posts-next': // next page 2, 3, etc
 		default:
 			$content_mode = $Blog->get_setting('main_content');
+			if( $content_mode == 'excerpt' && $disp == 'single' )
+			{
+				$content_mode = 'full';
+			}
 	}
 }