-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[com_content] Use condition show_tags in the view instead of in the model #10519
Conversation
will this be B/C? |
Currently model ContentModelArticle loads tags in view For component content my patch is back compatibility. For modules that use model ContentModelArticles in general yes. In short: 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. |
Gets a Fatal error right? |
No, it should get warning or notice that variable in undefined. |
@andrepereiradasilva |
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. I also removed default value "1" to be more similar to code from model/articles.php. About B/C. |
as said i'm not the one deciding, was just an idea. |
Ok. Thanks for discussion. 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. |
Is this a joke, Thomas? The "exception" seems to be the rule here... #9801 (comment) |
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. 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
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. |
Got it: "Article settings take precedence over menu settings" apply when there is no corresponding menu item... 😜 |
Ok // 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:
Results:
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:
|
Yes, as tedious as it may be. Or you could work with template overrides to achieve what you need. |
Current version remove mentioned inconsistency. I had mess with that params now should be ok. |
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
and
Then my immediate thought is that we cannot accept this This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10519. |
why it will break B/C ? |
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 |
Case 1, using: Case 2, some template code assumes, that tags have been loaded, then accessing: would call the magic method and load the tags ?? |
Something blinded me. |
Example of magic way:) Where should we use method |
Since $item->tags->getItemTags() is currently called in both,
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:
for later retrieval of the tags via magic function __get() is it a better name instead of setItem(): ? |
|
since in Joomla code "item" means "record" , i think "$itemId" is good enough ?
Now about
since in all PHP versions array typecast of null, is array(), |
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 |
|
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.
The patch as is makes two protected properties in all essence public in terms of read access. The I don't see why the new API methods are needed in 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. |
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 So the state value defaults to true but through an explicit change you can On Wednesday, July 27, 2016, Georgios Papadakis notifications@github.com
|
The other thing is I don't remember if JObject (which is what the model If it doesn't it takes a couple more lines of code (check if value is On Wednesday, July 27, 2016, Georgios Papadakis notifications@github.com
|
{ | ||
$item->tags = new JHelperTags; | ||
$item->tags->setItemIdentifier('com_content.article', $item->id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbabker In above If I replace only name method setItemIdentifier
to getItemTags
then this break B/C or not?
I do not want to create tags object in show_tags is not enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Though it would not affect the com_content views, you're still making
a B/C breaking change in the model. Ultimately that is why I'm making such
a fuss; you can add all the magic behaviors you want to account for that
change elsewhere, but you're still ultimately introducing a breaking change
that has to be accounted for by any consumer of that model.
On Wednesday, July 27, 2016, Tomasz Narloch notifications@github.com
wrote:
In components/com_content/views/article/view.html.php
#10519 (comment):@@ -152,8 +154,11 @@ public function display($tpl = null)
$item->text = $item->introtext;
}
$item->tags = new JHelperTags;
$item->tags->getItemTags('com_content.article', $this->item->id);
if ($item->params->get('show_tags', 1))
{
$item->tags = new JHelperTags;
$item->tags->setItemIdentifier('com_content.article', $item->id);
}
@mbabker https://github.com/mbabker In above If I replace
setItemIdentifier to getItemTags then this break B/C or not?
I do not want to create tags object in show_tags is not enabled.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/joomla/joomla-cms/pull/10519/files/ae8a6dd5a07efd29521277ebbc0bc34277a66769#r72445013,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoY8cyl6Ci-G9oyKUgCrscUMZbBSBks5qZ2XOgaJpZM4Ifr56
.
@mbabker about #10519 (comment) If I go that way then can I change mod_articles_.. and add line like: Do I have to care about custom template that using tags in that modules? |
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
|
@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. |
Summary of Changes
Tags usually slow down content pages, so I changed code to not load tags when it has not been required.
Reasons:
Testing Instructions
Change options for "Show tags" in menu items related to com_content.