Fix introtext treatment in single article view #9964

smz · 2016-04-17T23:08:49Z

Summary of Changes

An issue emerged in #9830 and was partially fixed in #9865 (onContentPrepare plugins not acting on introtext)

The underlying issue is that (contrary to what is done in the "Category blog" view) introtext is not "treated", even when it is the only text to be displayed due to the show_noauth option being in force and the user being guest.

This PR fixes the above issue.

Testing Instructions / Additional comments

Testing instructions for #9865 can be used.

Compared to current staging nothing should change when using the standard "Protostar" template and the single article view template is not overridden. This is because in #9865 a fix was incorporated in the single article template for the above behaviour.

This is not anyway correct: it shouldn't be a template's concern to "prepare" the item to be displayed: this is "view" territory. The same is done in the "Category Blog" view.

This PR fixes the issue at its roots so other (old and/or overriden) templates can benefit of it.

Also, there shouldn't be (and with this fix there is none) difference between "introtext" and "text", when used at the template level (actually we should only use "text"). Again, it is "view" territory to decide what should be displayed and what shouldn't.

$item->introtext should be assigned to $item->text when the "show_noauth" option is set and user is guest. This is in order to have the introtext correctly parsed by "onContentPrepare" event handlers. After that $item->introtext should be updated with the parsed $item->text so that templates still relying on introtext will display the correct output.

This was hackish as it would had fixed things only for this template. Not needed anymore now that the problem is fixed in the view.

andrepereiradasilva · 2016-04-17T23:32:52Z

I have tested this item ✅ successfully on 1621e7f

Agree with the change. Tested and works.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-17T23:37:19Z

Thanks @andrepereiradasilva, but... you've been too quick! I'm about pushing another commit fixing another minor aspect!! 😄

… "fulltext"

joomla-cms-bot · 2016-04-17T23:40:52Z

This PR has received new commits.

CC: @andrepereiradasilva

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-17T23:42:22Z

OK, ready! I got rid of the extra spurious blank that was appended to introtext when there was no fulltext...

andrepereiradasilva · 2016-04-17T23:47:23Z

I have tested this item ✅ successfully on 976b4cf

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-17T23:48:31Z

@andrepereiradasilva, thanks again and sorry for the "double testing"!

andrepereiradasilva · 2016-04-17T23:57:17Z

no problem

rgmears · 2016-04-18T14:18:58Z

I have tested this item ✅ successfully.

smz · 2016-04-18T14:34:09Z

Thanks for testing, @rgmears, but I think you haven't used the correct procedure to register your result (see that it doesn't reference latest commit of this PR and "Human Test Results" count is still at 1?): you have to register your test at https://issues.joomla.org/tracker/joomla-cms/9964, login with your github credentials and then click on the "Test this" blue button (above my funny face!)

rgmears · 2016-04-18T14:47:41Z

I have tested this item ✅ successfully on 976b4cf

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

rgmears · 2016-04-18T14:48:27Z

@smz
Is that better?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-18T14:54:39Z

@rgmears perfect! 👍 thanks!

lunalars · 2016-04-19T06:52:15Z

Hmmm, I'm not sure if it is a good idea to set $item->introtext = $item->text.
There may be reasons to use introtext in the template and it has been like this for years(?), so it would break b/c?

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

brianteeman · 2016-04-19T09:51:42Z

@lunalars is correct - this PR is not correct at all

smz · 2016-04-19T12:02:41Z

I think instead this is correct and I'll try to explain why.
If you have cases for which you think this is incorrect, can you please bring examples?
It can be that I missed something, but I honestly don't see where...

I think this is the only correct way to fix the the "onContentPrepare" treatment for introtext and it is exactly what is today done in /components/com_content/views/category/tmpl/blog_item.php which, by a large extent, is a clone of the article view (due to Joomla not being so DRY at this regard).

As you surely know content is stored in two columns at the DB level:

introtext
fulltext

there is no "text" column, and, following the MVC architecture, it is the duty of this "view" to build the text that will be later consumed by templates. And its duty is to do so accordingly to the options that are in force at the time it is called. This unhaplly is not what happening now.

When we fixed the treatment of introtext in #9865 (I'm saying "we" because that's code I pushed to your PR, Brian) we fixed only for the "Protostar" template. Millions of other sites not using Protostar are not benefiting of that "half backed fix" and I don't think this a good service to the name of Joomla who advertise itself as a "CMS": we should Manage Content in a correct way for every user, independently of which template he/she is using.

The same (unhappily, IMHO) happened today with #9959, where a bug was fixed, but only for Protostar, not for the entire set of Joomla sites. I think this is unfortunate: I don't want Joomla to become a "PCMS" (Protostar Content Management System).

We have two core site templates, Breeze and Protostar: many bugs are fixed for Protostar only. Breeze is neglected. I think a correct way to check bug fixes would be to verify that they work at the same time for Breeze and Protostar. If they don't, then we are doing something wrong with an hackish fix at the wrong place. We are patching things at the wrong place (unless the bug is a template specific one, that is).

Technically, what that "accused" line of code (again, already present elsewhere) is changing (hint: analyse the preceding if)?

it fixes the treatement of introtext (as per this PR title)
it changes the contentent of $item->introtext only when one of two conditions are met: The article is split by a readmore break and (due to the options in force) we are either
a) displaying the whole text (introtext + fulltext) or, b) only the second part of text, the one below the readmore break (fulltext, which again is an unfortunate name but we can't do anything about that...)

Which sites could be affected by this? Only sites with templates ignoring the options in force, that is displaying introtext ~~only~~ (and NOT processed by the onContentPrepare plugins) when something else was meant to be displayed (probably an handful if any at all, over millions) and this would be under all skies a mistake from their part and they simply deserve to be broken.

brianteeman · 2016-04-19T12:11:26Z

If you change the behaviour so that a site that was only displaying
introtext is now displaying the full text then it doesnt matter if you
guess it is a handfull or a million it is not b/c so will not be merged

On 19 April 2016 at 13:02, Sergio Manzi notifications@github.com wrote:

I think instead this is correct and I'll try to explain why.
If you have cases for which you think this is incorrect, can you please
bring examples?

It can be that I missed something, but I honestly don't see where...

I think this is the only correct way to fix the the "onContentPrepare"
treatment for introtext and it is exactly what is today done in
/components/com_content/views/category/tmpl/blog_item.php which, by a
large extent, is a clone of the article view (due to Joomla not being so
DRY at this regard).

As you surely know content is stored in two columns at the DB level:

introtext

fulltext

there is no "text" column, and, following the MVC architecture, it is the
duty of this "view" to build the text that will be later consumed by
templates. And its duty is to do so accordingly to the options that are in
force at the time it is called. This unhaplly is not what happening now.

When we fixed the treatment of introtext in #9865
#9865 (I'm saying "we"
because that's code I pushed to your PR, Brian) we fixed only for the
"Protostar" template. Millions of other sites not using Protostar are not
benefiting of that "half backed fix" and I don't think this a good
service to the name of Joomla who advertise itself as a "CMS": we should
_M_anage _C_ontent in a correct way for every user, independently to
which template he/she is using.

The same (unhappily, IMHO) happened today with #9959
#9959, where a bug was fixed,
but only for Protostar, not for the entire set of Joomla sites. I think
this is unfortunate: I don't want Joomla to become a "PCMS" (Protostar
Content Management System).

We have two core site templates, Breeze and Protostar: many bugs are fixed
for Protostar only. Breeze is neglected. I think a correct way to check bug
fixes would be to verify that they work at the same time for Breeze
and Protostar. If they don't, then we are doing something wrong with
an hackish fix at the wrong place. We are patching things at the wrong

place (unless the bug is a template specific one, that is).

Technically, what that "accused" line of code (again, already present
elsewhere) is changing (hint: analyse the preceding if)?

it fixes the treatement of introtext (as per this PR title)

it changes the contentent of $item->introtext only when one of two
conditions are met: The article is split by a readmore break and (due
to the options in force) we are either a) displaying the whole
text (introtext + fulltext) or, b) only the second part of text, the
one below the readmore break (fulltext, which again is an unfortunate
name but we can't do anything about that...)

Which sites could be affected by this? Only sites with templates ignoring
the options in force, that is displaying introtext only (and NOT
processed by the onContentPrepare plugins) when something else was meant
to be displayed (probably an handful if any at all, over millions) and
this would be under all skies a mistake from their part and they simply
deserve to be broken.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9964 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

smz · 2016-04-19T12:16:35Z

Brian, that's minutiae compared to what happened with 3.5.0 and 3.5.1 (heck, we even wiped out UCM tables for non core extensions...) and again those site would be in error...

If we change an erroneous behaviour that's "fixing a bug", not "breaking B/C"

lunalars · 2016-04-19T12:17:43Z

$item->introtext should just be introtext and not introtext + fulltext or anything else.

And: I am the one in a million using $item->introtext in some of the templates I created :-)

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-19T12:19:33Z

@lunalars You are using the non processed introtext when introtext wasn't meant to be displayed?
Can you bring a specific example, please?

brianteeman · 2016-04-19T12:35:00Z

You can argue as much as you want but this breaks existing sites and will
not be merged as is

On 19 April 2016 at 13:19, Sergio Manzi notifications@github.com wrote:

@lunalars https://github.com/lunalars You are using the non processed
introtext when introtext wasn't meant to be displayed?
Can you bring a specific example, please?

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#9964 (comment)

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

joomla-cms-bot · 2016-04-19T12:39:58Z

This PR has received new commits.

CC: @andrepereiradasilva, @rgmears

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-19T12:40:35Z

Happy?

smz · 2016-04-19T12:42:18Z

I'll leave to the PLT to decide which solution is deemed to be better: before or after a491c82

lunalars · 2016-04-19T12:43:22Z

Just keep it logical:

introtext is just introtext and nothing else
text can be introtext, intro- + fulltext or just fulltext

I would call it a bug if introtext suddenly becomes fulltext (or whatever).

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-19T12:45:03Z

@lunalars can you please test with your site in a million after latest commit? Is that OK with you?

smz · 2016-04-19T12:46:53Z

@andrepereiradasilva and @rgmears Sorry guys, can you please retest after latest commit (with which I don't agree, but wouldn't do any harm)? Thanks!

smz · 2016-04-19T12:51:55Z

Ooops... that's new... Travis doesn't like // comments spanning multiple lines (but only for PHP 5.6)...

Should this be fixed? Adapting my commit, anyway...

joomla-cms-bot · 2016-04-19T12:55:06Z

This PR has received new commits.

CC: @andrepereiradasilva, @rgmears

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-04-19T12:59:08Z

@lunalars
nitpiking, but:

Just keep it logical:

introtext is just introtext and nothing else
text can be introtext, intro- + fulltext or just fulltext

Actually there is not much logic in the naming of those DB columns/object attributes:

most of the times introtext is the "whole" text (when articles do not have a readmore break)
fulltext NEVER is the "whole" text

smz · 2016-04-19T13:37:25Z

PLT, please beware: with latest commit I pushed following the wishes of @brianteeman and @lunalars, the behavior is fixed only in Protostar (where I have corrected the affected template to use text instead of introtext) but will not correct the behaviour for other templates (Breeze included, I guess...) that are using introtext instead of text for displaying the introtext when needed (sorry for the tongue-twister...)

rdeutz · 2016-04-19T13:38:30Z

@smz code style checks are running only on 5.6

smz · 2016-04-19T13:39:36Z

Thanks @rdeutz ! Still think a three liner // comment is much better than a /* ... */ which will probably become a 5 liner...

JamalTailored · 2016-07-09T11:31:15Z

I have tested this item ✅ successfully on b0eaf3c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

truptikagathara · 2016-08-11T12:10:11Z

I have tested this item ✅ successfully on b0eaf3c

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

rgmears · 2016-08-11T12:49:37Z

I have tested this item ✅ successfully on b0eaf3c

But there is a typo. "Don't do that when introtex ..." should be "Don't do that when introtext ..."

smz · 2016-08-11T14:47:54Z

Thanks to all testers and my apologies to @JamalTailored for not having noticed his test of more than a month ago!

You're right about the typo, @rgmears... and I also think it would be sensible to change the name of that variable I introduced in a491c82 from $full_fix to something nicer (a contest is open for the best name!), but I'm reluctant as I'm afraid that just applying those cosmetic changes will invalidate the 3 positive tests that we now have...

smz · 2016-08-11T14:52:16Z

P.S.: I'm still convinced that this PR was better before applying a491c82, but again I leave this to a PLT decision...

brianteeman · 2016-08-11T18:52:00Z

Which sites could be affected by this? Only sites with templates ignoring the options in force, that is displaying introtext only (and NOT processed by the onContentPrepare plugins) when something else was meant to be displayed (probably an handful if any at all, over millions) and this would be under all skies a mistake from their part and they simply deserve to be broken.

By your own admission this breaks existing web sites - we have no idea if it is one or one million but we should not be breaking exiting sites with an upgrade

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

brianteeman · 2016-08-11T18:52:01Z

Which sites could be affected by this? Only sites with templates ignoring the options in force, that is displaying introtext only (and NOT processed by the onContentPrepare plugins) when something else was meant to be displayed (probably an handful if any at all, over millions) and this would be under all skies a mistake from their part and they simply deserve to be broken.

By your own admission this breaks existing web sites - we have no idea if it is one or one million but we should not be breaking exiting sites with an upgrade

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9964.}

smz · 2016-08-11T20:00:03Z

@brianteeman Got, it! Got it! No need to underline that again (twice).

What you're probably missing is that since a491c82 that's not the case anymore...

roland-d · 2017-05-13T20:15:10Z

@smz What is the status of this PR, is it still needed?

smz · 2017-05-13T23:11:22Z

To be honest I don't know how to answer: the code has changed significantly since, and I don't feel confident advising if/how this should be merged.

If I remember correctly, there where ~~two issues~~ just one issue here:

~~when introtext was the only available text and it was displayed due to current authorization restrictions, then it wouldn't be treated (i.e.: plug-ins applied) as it should..~~
Some authorization restriction was applied at the template level while it should had been applied at the view level (so that templates cannot override an authorization restriction)

smz · 2017-05-14T11:54:10Z

I amended my answer above: the first issue was already resolved in another PR and only the second (architectural) issue was addressed here.

smz · 2017-05-14T12:53:30Z

I just noticed that in #11290 the following code has been added:

// NOTE: The following code (usually) sets the text to contain the fulltext, but it is the
// responsibility of the layout to check 'access-view' and only use "introtext" for guests

So, apparently it has been decided that the concern of content authorization must be moved down to the layout level instead of up to the view (or model) level.

P.S.: I love the "usually"...

P.P.S: I think the comment added in #11290 is wrong: what is usually done is to set $item->text = $item->introtext;, not $item->text = $item->fulltext; As I stated above $item->fulltext never contains the "whole text": it is either empty or it contains the text following the <hr id="system-readmore" />. I think that in an hypothetical major overhaul the name of the properties should auspiciously be corrected.

brianteeman · 2017-05-21T17:42:43Z

Based on the comments above I am closing this

smz added 2 commits April 18, 2016 00:43

Partial rollback of joomla#9865

1621e7f

This was hackish as it would had fixed things only for this template. Not needed anymore now that the problem is fixed in the view.

joomla-cms-bot added the PR-staging label Apr 17, 2016

smz changed the title ~~Smz fix introtext treatment in single article view~~ Fix introtext treatment in single article view Apr 17, 2016

smz mentioned this pull request Apr 17, 2016

Single registered article guest view #9865

Merged

While we are here... get rid of extra spurious blank when there is no…

976b4cf

… "fulltext"

andrepereiradasilva mentioned this pull request Apr 18, 2016

Regression: Redirect to profile when Register to Read More #9959

Merged

Fix for templates using introtext when that's not supposed to happen

a491c82

Making Travis happy

b0eaf3c

brianteeman closed this May 21, 2017

Fix introtext treatment in single article view #9964

Fix introtext treatment in single article view #9964

Conversation

smz commented Apr 17, 2016 • edited by joomla-cms-bot

Summary of Changes

Testing Instructions / Additional comments

andrepereiradasilva commented Apr 17, 2016

smz commented Apr 17, 2016

joomla-cms-bot commented Apr 17, 2016

smz commented Apr 17, 2016

andrepereiradasilva commented Apr 17, 2016

smz commented Apr 17, 2016

andrepereiradasilva commented Apr 17, 2016

rgmears commented Apr 18, 2016

smz commented Apr 18, 2016

rgmears commented Apr 18, 2016

rgmears commented Apr 18, 2016

smz commented Apr 18, 2016

lunalars commented Apr 19, 2016

brianteeman commented Apr 19, 2016

smz commented Apr 19, 2016 • edited

brianteeman commented Apr 19, 2016

It can be that I missed something, but I honestly don't see where...

place (unless the bug is a template specific one, that is).

smz commented Apr 19, 2016 • edited

lunalars commented Apr 19, 2016 • edited

smz commented Apr 19, 2016

brianteeman commented Apr 19, 2016

joomla-cms-bot commented Apr 19, 2016

smz commented Apr 19, 2016

smz commented Apr 19, 2016

lunalars commented Apr 19, 2016

smz commented Apr 19, 2016

smz commented Apr 19, 2016

smz commented Apr 19, 2016 • edited

joomla-cms-bot commented Apr 19, 2016

smz commented Apr 19, 2016 • edited

smz commented Apr 19, 2016

rdeutz commented Apr 19, 2016

smz commented Apr 19, 2016

JamalTailored commented Jul 9, 2016

truptikagathara commented Aug 11, 2016

rgmears commented Aug 11, 2016

smz commented Aug 11, 2016

smz commented Aug 11, 2016

brianteeman commented Aug 11, 2016

brianteeman commented Aug 11, 2016

smz commented Aug 11, 2016

roland-d commented May 13, 2017

smz commented May 13, 2017 • edited

smz commented May 14, 2017

smz commented May 14, 2017 • edited

brianteeman commented May 21, 2017

smz commented Apr 17, 2016 •

edited by joomla-cms-bot

smz commented Apr 19, 2016 •

edited

smz commented Apr 19, 2016 •

edited

lunalars commented Apr 19, 2016 •

edited

smz commented Apr 19, 2016 •

edited

smz commented Apr 19, 2016 •

edited

smz commented May 13, 2017 •

edited

smz commented May 14, 2017 •

edited