I have tested this item successfully on 1621e7f

Agree with the change. Tested and works.

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

This PR has received new commits.

CC: @andrepereiradasilva

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

I have tested this item successfully on 976b4cf

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

no problem

I have tested this item successfully.

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!)

I have tested this item successfully on 976b4cf

@smz
Is that better?

@rgmears perfect! thanks!

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?

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

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:

1. introtext
2. 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)?

1. it fixes the treatement of introtext (as per this PR title)
2. 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.

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:

1. introtext
2. 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
Manage Content 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)?

1. it fixes the treatement of introtext (as per this PR title)
2. 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/

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"

$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 :-)

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

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/

This PR has received new commits.

CC: @andrepereiradasilva, @rgmears

Happy?

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

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).

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

@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!

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...

This PR has received new commits.

CC: @andrepereiradasilva, @rgmears

@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

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...)

@smz code style checks are running only on 5.6

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

I have tested this item ✅ successfully on b0eaf3c

I have tested this item ✅ successfully on b0eaf3c

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 ..."

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...

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

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

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

@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...

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

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 here:

1. 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..

2. 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)

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

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"...

Based on the comments above I am closing this