will this be B/C?

Currently model ContentModelArticle loads tags in view

but ContentModelArticles loads tags in model so there is some inconsistency that should be fixed.

For component content my patch is back compatibility.
I have that changes on production and it works.

For modules that use model ContentModelArticles in general yes.

In short:
In module template will be missing "item->tags" variable
when menu item will have show_tags or show_cat_tags enabled.
I suspect that nobody use that tags when it depends of menu parameters.

Joomla3 built-in modules that use the model are B/C.

When that patch won't be B/C:

If someone created custom template for ex. mod_articles_latest (default.php) and start using tags which depends on menu parameters show_tags then there will be a problem.

If someone created custom template for ex. mod_articles_latest (default.php) and start using tags which depends on menu parameters show_tags then there will be a problem.

Gets a Fatal error right?

No, it should get warning or notice that variable in undefined.

@andrepereiradasilva
If you have some example of code I can take a look

i don't have an example, i'm just guessing.

i don't know if this ok from a B/C point (it's not my call) but anyhow it would be better if you could prevent that php warning or notice and send a deprecated notice to the deprecated log.

Code from blog_item.php

<?php if ($params->get('show_tags') && !empty($this->item->tags->itemTags)) : ?>

describes that there is no problems with error because function empty check that.

For other unexpected codes I can add empty list of tags for B/C in model/articles.php

// Do not load tags but create empty list for B/C
if ($item->params->get('show_tags', '1') == '1')
{
    $item->tags = new JHelperTags;
    $item->tags->itemTags = [];
}

But I found other bug in my patch

show_cat_tags is not related to article tags but to category (itself) tags.

I have fixed my mentioned bug in the patch.
For do that I have to add a new option to show tags for featured and blog view:
Show only for article view

I also removed default value "1" to be more similar to code from model/articles.php.

About B/C.
Previous similar patch (not mine)
#9038
did not require any deprecated information so I do not think that now it is required. But I may think wrong.

as said i'm not the one deciding, was just an idea.

Ok. Thanks for discussion.
If someone can add more advice I listen.

I will be thankful for testing.

You shouldn't need the special case for article view. You can already override the menu settings in the article settings. Which means if the article says to show tags, the tags will be shown even if the category menu item says to hide them. Article settings take precedence over menu settings which take precedence over global settings. There is an exception to that rule, but that doesn't matter in this case ????

So please remove that added option, it only complicates code and doesn't solve anything.

Article settings take precedence over menu settings which take precedence over global settings. There is an exception to that rule, but that doesn't matter in this case ????

Is this a joke, Thomas? The "exception" seems to be the rule here... #9801 (comment)

Is this a joke, Thomas? The "exception" seems to be the rule here

There is only one exception to that rule, and it is if the menu item is specific to the view/id being shown. That is if you have a menu item pointing to article "foo", then the settings from that article "foo" are overriden by the menu item. For the article "bar", the article settings take precedence over the menu item "foo" as usual.
Same for categories. If the menu item is pointing to the category "blubb", then the category options for category "blubb" are overriden by the menu item. But not the options for category "blabla" and not for the articles "foo" and "bar".

Once you figured that out, it's simple and actually makes sense ????

Sorry, I don't want to hijack this PR and if you wish we can continue the discussion in #9801, but when you state that

if you have a menu item pointing to article "foo", then the settings from that article "foo" are overriden by the menu item.

That means that menu item options have the precedence over article options, not the other way 'round as you initially stated.

That means that menu item options have the precedence over article options, not the other way 'round as you initially stated.

Yes, in that specific case (menu item is specific to article "foo" and "foo" is displayed) the menu item takes precedence. It's the one exception to the rule. That's what I explained.

so when the "rule" ("Article settings take precedence over menu settings") does apply?

All other cases as explained.
Of course, if your site is built with menu items for each article, then the menu items always override the article settings as you always have that exception. But that depends on your site structure.

Got it: "Article settings take precedence over menu settings" apply when there is no corresponding menu item... ????

Ok
you say about 164 line from model/article.php

                // Convert parameter fields to objects.
                $registry = new Registry;
                $registry->loadString($data->attribs);

                $data->params = clone $this->getState('params');
                $data->params->merge($registry);

But then I have to change show_tags to show for every articles (thousands).

Or I can use weird configuration like:
1) set up menu item for featured and category view to show_tags=use_article (in article view this will be treats as true)
2) set up component global parameters show_tags=0
3) every article set show_tags use global - empty string (this is the default value)

Results:
1) Then for menu item for category view, show tags will have value "use_article" which is replaced by global
show_tags=0 (because article does not have value) => OK hide tags for category/featured view
2) For article view joomla get value from menu item show_tags=use_article because article parameter show_tags is not set. => OK show tags for article view

Everything above seems to work so I can agree and remove added option "Show only for article view"

My above comment was to first comment @Bakual

Two things that I noticed from review:

• You use params->get('show_tags') to fetch the value, however in most places we use params->get('show_tags', 1) (by default enabled). This seems to be an inconsistency in current code but needs to be addressed to avoid unexpected errors.
• You use if ($item->params->get('show_tags') == '1') however that isn't the same as the current check if ($item->params->get('show_tags')), it would fail with the use_article value (which is a stupid value, but that's another topic). Is there a reason you explicitely check for 1 and not just do a boolean check like we do in the other places and did before?

But then I have to change show_tags to show for every articles (thousands).

Yes, as tedious as it may be. Or you could work with template overrides to achieve what you need.
Adding a specific check for a single parameter into core just to please your specific needs is sure the wrong approach.

Current version remove mentioned inconsistency. I had mess with that params now should be ok.

Is there any issue for current version?

Is there any issue for current version?

There's a possible B/C concern in that the article model will now not return tag data at all in com_content (inconsistent with other components which don't even check the value of this parameter) and that anyone using this model would have to manually bind tags data themselves if they are working with it. Then again, considering the terrible implementation of tags in general, at this point I'm inclined to lean toward "who cares?".

Then I think this pull is ready for testing.

As soon as I see statements such as

I suspect that nobody use that tags when it depends of menu parameters.

and

If someone created custom template for ex. mod_articles_latest (default.php) and start using tags which depends on menu parameters show_tags then there will be a problem.

Then my immediate thought is that we cannot accept this

This is the same or similar B/C please take a look:
#9038

No it is very different

IMHO this PR is right direction.

But what now?

Should I add some code to be more B/C, for ex:

// Do not load tags but create empty list for B/C
if ($item->params->get('show_tags', '1'))
{
    $item->tags = new JHelperTags;
    // Set empty list for B/C to prevent errors 
    $item->tags->itemTags = [];
}

or should this PR wait for 3.7 or later?
I do not see any place to add deprecated logs.

Can I ask a member to remove label "Language Change"?

1) Does the B/C is only the reason for frozen that PR?
2) IMHO tags should be load in view, not in model. I based on model/view for single article. Do you agree?

@roland-d Could you look at the code changes. I know that it is a little break B/C.
Whether these changes can be accepted in the future?

@csthomas Any B/C break will have to wait till at least 4.0. The little break you are talking about is not the only break I see. Since you moved the loading of tags from the model to the view, anybody retrieving articles from the model will also be left with articles without tags.

At the moment in JHelperTags the itemTags is undefined:
https://github.com/joomla/joomla-cms/blob/staging/libraries/cms/helper/tags.php#L428-L428

So you can add a definition of itemTags to the class like this:

     /**
     * Holds a list of item tags
     *
     * @var    array
     * @since  3.1
     */
    protected $itemTags = array();

In the view you can then do this:

$item->tags = new JHelperTags;

if ($item->params->get('show_tags', 1))
{
    $item->tags->getItemTags('com_content.article', $item->id);
}

Now you won't have any undefined variables and the array will only be filled if tags are loaded per the configuration setting. You can use this everywhere and still keep B/C and have conditional loading.

Does that make sense?

I did what you say, but I could not set it as:

    protected $itemTags = array();

and I set it as:

    public $itemTags = array();

to be B/C with others.

Then from now $item->tags->itemTags is always defined in view.

I forgot, thanks @roland-d for comment.

Any code review will be helpful.

Now PR is B/C,
and tags won't be loaded in articles model (but only will be created an object with empty array).

This speed up performance of any module that display articles, (which use Articles model) but not require to load any tags.

It's still not B/C because it changes how data is loaded, specifically data is NOT loaded unexpectedly going forward. IMO a proper fix for this is going to be introducing another state value into the model (default true) and callers that wish to exclude the tags data set this state value.

So something like:

$model->setState('load_tags', false);

In the model...

// Defaults to true for B/C with previous versions that always loaded tags data
if ($this->getState('load_tags', true) {
    // Do loading
}

In general I agree with making this behavior configurable, but looking at it as an API this is a B/C breaking change as the original API call is now missing data and requires the implementor to make a second call to retrieve data that was previously included with the response, demonstrated by the changes required in the view files. Adding the model state "parameter" allows this to be done in a B/C manner while still opting for the "preferred" behavior of excluding some data.

Yes, (after I go your way) it will be full B/C, but now I think that we concern on B/C too much.
The model may have too many options. (show_tags, load_tags)

Tags are loaded in separate loop.
They should be loaded separately. Outside model.

as the original API call is now missing data and requires the implementor to make a second call to retrieve data that was previously included with the response

Yes. I turned off load tags in API and for "non core extension" developer has to do it "outside", means in its view.

I have to think about it more time.

I think that we concern on B/C too much

I just play by the rules. I'm itching to break B/C (no more PHP 5.3 support, cleaning up a lot of dead code and conventions, getting rid of the PHP 4 structure that still lives in the code base, rewriting features correctly to not depend on the awkwardness known as UCM... /wishful-thinking).

i can not say i followed all details in this conversation,

so forgive stupid question

can i ask if

The item->tags always be an object instance of JHelperTags, without the tags being retrieved

and then the item->tags->itemTags

be defined as protected access in its Class
and then use a magic function to populate it according to configuration ?
thus if custom template tries to use it, there will not be a problem

(and optionally if configuration says not to use then set it to empty array)

Currently its access level of itemTags is not explicitly set, right ?
and probably there are none classes extending the TagsHelper model and setting it to public access ?

Do you mind magic method:

protected $itemTags = null;

public function __get($key)
{
    if ($key == 'itemTags' && $this->itemTags === null)
    {
        $this->itemTags = $this->getItemTags(??populated data??);
        return  $this->itemTags;
    }
}

yes,

[EDIT]
with "yes", i meant, yes, use something like the above

Then we will have to treat getItemTags as populate state method.
If we add a new method to populate then it break B/C.

I do not want to complicate it too much,
Probably I have to wait to J4 (and revert B/C stuff).

If we add a new method to populate then it break B/C.

why it will break B/C ?
the purpose of it would be not to break B/C while at the same time avoid tags being always populated

If I understand you well then you want to do something like:

$item->tags->getItemTags('com_content.article', $item->id); // populate state without loading tags
// then
print_r($item->tags->itemTags); // magic load tags

but there is place where getItemTags method return value directly.
https://github.com/joomla/joomla-cms/blob/staging/templates/beez3/html/com_weblinks/category/default_items.php#L133

Case 1, using:
getItemTags() load the tags explicitely, no change here

Case 2, some template code assumes, that tags have been loaded, then accessing:
$item->tags->itemTags

would call the magic method and load the tags ??

Something blinded me.
Now I understand, thanks.

Example of magic way:)

Where should we use method setItem? In model or view?

Where should we use method setItem? In model or view?

Since $item->tags->getItemTags() is currently called in both,

• and since the purpose is to replace it with a call to "seItem()" when configuration says not to use tags

I think you can use it at the same places , and don't try to remove it completely from the view ?

Also since the purpose of the "setItem()" is to set an identifier:

• typeAlias
• itemId

for later retrieval of the tags via magic function __get()

is it a better name instead of setItem(): ?
setItemIdentifier()

• I will change method name setItem to setItemIdentifier
• tags.php $itemId is OK? or should I use contentItemId?
• I will back to 'if ($item->params->get('show_tags', 1))' in views which will contain only setItemIdentifier
• If you don't like any code comments or names in PR please correct me.

tags.php $itemId is OK? or should I use contentItemId?

since in Joomla code "item" means "record" , i think "$itemId" is good enough ?

• and it is different from "$Itemid" that reminds of the menu item, right ?

Now about

return (array) $this->itemTags;

since in all PHP versions array typecast of null, is array(),
and since the layout code / overrides , etc that will try to use tags without considering configuration will always expect an array there ?
i think it is good to do it like that

If you're introducing a magic getter then for all intents and purposes you're treating a variable as public. So either use a proper get method or declare the variables public.

Honestly this whole pull request is starting to get a little awkward. What started as looking for a way to stop tags from always being loaded even in contexts where the data was unnecessary is turning into awkward API changes plugged into an already crappy API with tags in general.

What I suggested yesterday with the model state addresses the original concern with what was trying to be accomplished here without the awkward API changes. These are the types of things model state should be used for if not introducing explicit API methods to set behaviors from the outside. To me that was the simplest solution to this issue.

so you have a solution that will not break B/C and thus can be used today ? and not after 1 or 2 years ?

about magic method it will serve the purpose of not running unneeded and somewhat "performance-unwise" SQL queries
that is one of the purposes of magic __get function to improve performance doing stuff if needed ... why is it bad ?

• I broke B/C again to ask you if that is important for article view only.
• If fix totally B/C then Joomla 3.7 or J3.6.x:)
• I added @deprecated in article view because tagLayout is re-created in template again

so you have a solution that will not break B/C and thus can be used today ? and not after 1 or 2 years ?

Yes. Use the model state as I suggested at #10519 (comment) and you could have a patch merged that is B/C AND allows to opt-out of loading the tags data today.

about magic method it will serve the purpose of not running unneeded and somewhat "performance-unwise" SQL queries
that is one of the purposes of magic __get function to improve performance doing stuff if needed ... why is it bad ?

The patch as is makes two protected properties in all essence public in terms of read access. The $itemId property which has zero manipulation around it whatsoever and $itemTags which does some checks and returns different values. These should be in getItemXXX() methods, magic getters like this aren't giving any benefit over proper API endpoints and even worse can mask errors if you don't implement a proper error handling mechanism for non-existing properties.

I don't see why the new API methods are needed in JHelperTags as they quite frankly aren't adding any real value other than some magic behavior. If you're really insistent on going the new API route, I'm going to insist on properly declared API methods versus magic behaviors.

As is, this patch is NOT backward compatible. The only way to achieve backward compatibility is for the model to continue the existing behavior of default loading the tags data. An API endpoint that in the middle of a major version series stops returning data as part of a method call has introduced a B/C break. This is somewhat confirmed by the fact that this patch requires changes in view classes as a proxy layer for the existing behavior and a need for the magic getter to ensure the data actually loads. A new toggle (i.e. a model state condition) to exclude the tags data, defaulting to the data being loaded to match the existing behavior, is an acceptable backward compatible change because it creates a new API that allows a caller to opt-out of loading the data going forward.

... A new toggle (i.e. a model state condition) to exclude the tags data, defaulting to the data being loaded to match the existing behavior, is an acceptable backward compatible change because it creates a new API that allows a caller to opt-out of loading the data going forward.

yes , yes

but what about existing sites with template overrides that assume that tags have been loaded, and try to use them without checking configuration ?

I mean the "model state condition" will have to be left to ON , to retieve tags always regardless of configuration so that the thing remains B/C in all cases

In a module or another extension where you use this model you set the state
value before calling the getItem method. Other modules are already doing
this with other conditions.

So the state value defaults to true but through an explicit change you can
turn it to false without any other implications beyond that data not being
there anymore, which you know because you turned it off.

On Wednesday, July 27, 2016, Georgios Papadakis notifications@github.com
wrote:

I mean the "model state condition" will have to be left to ON , to retieve
tags always regardless of configuration so that the thing remains B/C in
all cases

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10519 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoQlpk0hV8sdfsk8-elc77Ec_ED2tks5qZ2EAgaJpZM4Ifr56
.

The other thing is I don't remember if JObject (which is what the model
state is represented by right now) has a def method similar to the Registry
class. If it does then the model has a simple one line check, $getTags =
$this->getState()->def('load_tags', $item->params->get('tags_condition',
true));

If it doesn't it takes a couple more lines of code (check if value is
defined in state and if not use item param value basically). Either way
it's not all that complex. Similar changes could/should be made to all
models for consistency then.

On Wednesday, July 27, 2016, Georgios Papadakis notifications@github.com
wrote:

... A new toggle (i.e. a model state condition) to exclude the tags data,
defaulting to the data being loaded to match the existing behavior, is an
acceptable backward compatible change because it creates a new API that
allows a caller to opt-out of loading the data going forward.

yes , yes

but what about existing sites with template overrides that assume that
tags have been loaded, and try to use them without checking configuration ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10519 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoRBiWzS0rP89xxG13RfBVoQRi93qks5qZ2CGgaJpZM4Ifr56
.

@mbabker about #10519 (comment)

If I go that way then can I change mod_articles_.. and add line like:
$model->setState('load_tags', false); to Joomla core modules.

Do I have to care about custom template that using tags in that modules?
I know that I can do that for my own modules, but what about core mod_articles_...? B/C?

For the core modules you'd probably need to add that as a parameter if it isn't already. I honestly don't know in what case the tags data would get used with latest or related articles but since the functionality's there now you have to assume someone's doing something crazy with it. Then again, at the same time since the modules aren't designed to display associated tags data, you could argue that it's not needed and hardcode it in too. For pure B/C you'd have to do the former; for practical purposes the latter would work. Personally I'd lean toward the latter there because modules have a lot less flexibility and re-use compared to components, but the component models do need to retain some resemblance of B/C since they have re-use potential (i.e. all the articles modules or com_contact's ability to display articles from a user).

Is anyone unhappy with current code?

At a glance it looks fine.

On Wednesday, July 27, 2016, Tomasz Narloch notifications@github.com
wrote:

Is anyone unhappy with current code?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10519 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoW4AVgxBwjJzjfWzVOC0jF8Mh9aaks5qZ77mgaJpZM4Ifr56
.

@csthomas is this good for testing?

I have to resolve conflict. Please wait.

I do not have more interest for that PR. I close it.