-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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] Correctly adding layout to links #20211
Conversation
This patch is needed as soon as possible. After 3.8.7 update, more than 10 people have contacted me with this problem. 4 of them had an endless redirect due to the fact that "No Dubles Plugins" considered the link not correct and made a redirect. I will describe why it is so To me, not so many people turn, so, such a count of appeals personally bothers me |
I am confused why you have had so many reports but there have been no others. I would expect the forum to be full of issues |
@brianteeman I do not know why people do not like to use official forums and publicly report problems. Perhaps this is related to the language barrier or just afraid. It's easier for them to approach the master directly and ask them to correct it. In my case, these were former clients and just people who know that I understand joomla In addition, I created #20199 Issue and #20200 PR almost immediately after the update. The next day PR became RTC But in #20200 I did not take the multilanguage sites and the output of links in other components (I had to correct the bug in a row, I corrected it). To be honest I read badly for created #19681. Today I noticed that the problem with the link is being watched in other components (on my website it was com_tags). After the fix I finally read carefully for what was done #19681 PR. And he considered that the proposed idea is not correct in most cases P.S Until recently, I also did not write on github, but simply helped people who addressed me personally or wrote about a problem on any third-party sites. It seemed to me that someone would definitely report a bug and without my participation |
so i am still confused. is this an issue with the core of joomla or a problem that only occurs when using your extension. |
@brianteeman Firstly, these are not my extensions. I do not write such plugins, because those that have already proven themselves well. I sent you an email with a couple of other plugins that started to cause problems after upgrading Joomla However, the bug in the work of these plugins revealed a core bug. If a person has problems with infinite redirects, he would not even notice him. And this is the core problem.
|
Reverting this isn't right either. As the original PR points out, views can have multiple layouts and generating links needs to be aware of that (especially as the layout param is part of the menu item's link, |
@mbabker Are you sure about that?
If I do not understand clearly please follow the instructions beginning with With the original PR #19681 I will have to completely override associates functions or register links manually for each category |
I don't know the specifics of the original PR or what you're trying to fix, but if the layout is not considered when creating links then there can be unintended side effects as has been pointed out. So you're running into a case now where including the layout is creating side effects. We can't replace one bug with another. Both need to be addressed. |
@Septdir Switching via the language switcher DOES display the correct layout in both cases. |
@mbabker This PR does not cause a bug. Because it was not originally. |
You are looking at one use case. Re-read the original PR, reverting it breaks the use case specified in that PR. The use case you're working with:
The use case the PR addressed:
I'm not testing these PRs, just reading code and use cases. The original PR does seem like it is causing issues for your use case, but in the use case the PR was discussing it fixed a legitimate bug for legitimate reasons (layout is a routable parameter and has to be taken into consideration to get correct navigation). So again, we can't replace one bug with another; both need to be adequately addressed. |
I confirm though that when using "Modern Routing", and when we have no menu item for a category, when we click on an article category, we do get urls like Therefore, when "Modern Routing" is enabled, looks like we still have an isssue. |
@mbabker It's worthwhile to add layout to the links, but obviously not as it was done. At the moment, the best option is to revert the original PR. And as soon as possible. Because the indexing of incorrect urls will happen pretty soon. And sites that used different layout for each language can not function normally And then calmly and think about how best to add layout. And do this in a 3.9.0 branch or 4.0 I recorded a video to show the problem https://septdir.ru/video/jbug.flv |
I'm confused because that issue was fixed here (#20200) or not? Maybe just partially? Or not completely? It removes layout attributes with no value. |
Again I get what the problem is. But, I am not going to accept "we are reverting this patch because it introduces a bug for this workflow resulting in restoring a bug for another workflow" as a viable option. The layout must be considered when building links. There is no way to bypass that. What we need now is to ensure the right layout is considered in various use cases and scenarios. |
To be honest, I do not have much desire to argue about the need to revert, I'm not very hard on my hands to fix this bug on sites. @infograf768 #19681 create the bug with list/blog layouts. Although they were initially easy to read Testing Instructions to understand that this is how it should be. If you describe in detail your problem, it will be possible to come up with a more correct solution. Than always open the current show layout in all languages regardless of the created menu item |
@mbabker Good. Do you plan a new more global PR or can I just add a layout in the given pr as a parameter in the function. And then you'll figure out how to use it, which will solve the other problem
|
The problem is exactly as described in the original PR:
In the language switcher, on the frontend you should stay on the layout you started at because of the way you've set up your associations. So if you're switching on the "Category Blog" menu item ( Now you're presenting a use case where you have items associated with different layouts (in one language you have the "Category Blog" menu item associated to a "Category List" menu item and the routing is broken because the code is now making an assumption on the layout based on the current request). Basing the layout parameter on what's in the current request isn't the best solution (as it clearly creates other problems). But, the layout parameter does need to be considered correctly. |
I would say that this is a very bad decision. Usually, we read from the left to the right, but there are not a few languages where you need to read it differently. As a consequence, models are often done differently. Besides. With the original PR, you mislead people. This is not true. If I created a menu item with a different layout, then I need to output another layout. If I want to output in the same layout, I will create a menu item with the same layout or I will indicate the appropriate menu item in associations And earlier it worked. You correctly said you can not fix one bug when creating a new one. But the original PR created several bugs at once. Not deciding prietom any. |
Re-read the reproducer. That is a case where you have two menu items for a category, one in each layout, and the switcher would not route to the correct menu item because the layout parameter was not considered causing the switcher to link to an unexpected page. All the change did was expose other underlying issues because, as I've said repeatedly now, the layout parameter was not previously taken into consideration for generating links and not doing so (and inherently arbitrarily assuming every layout will be default, which breaks using alternative layouts either supplied by core (i.e. Category Blog and Category List) or created as overrides in the template) means routes are not correctly built. |
@mbabker I completely agree with you, but the original PR did not solve this problem. Okay, solves but you only in one case, if you created 4 menu items and only standard layout is used. He just betrayed the current layout in the link, and exclusively in com_contnet / category. Iit's not true, because layout was To solve a problem with layouts in multilanguage sites it is necessary more globally. And as we say from the other side. Firstly, you need to add the setting to take into account the layout in the association or not. Then you have to revise the associations in all the current components, and also add layout to all the links, and not just pass the slug. And so on. It's a very big job. And I'm not sure that he needs a straightforward moment in 3.8 branch. |
ups =) now everything is correct. Here's what happens when you make changes after a working day. @mbabker Please check code. @pavluk Please test again @Arkadiy-Sedelnikov Please test again @infograf768 Please test multilanguage should work both in #19681 P.S Who can explain to me why |
* | ||
* @return array Array of associations for the component categories | ||
* | ||
* @since 3.0 | ||
*/ | ||
public static function getCategoryAssociations($id = 0, $extension = 'com_content') | ||
public static function getCategoryAssociations($id = 0, $extension = 'com_content', $layout = null) |
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.
is it correct that this is hardcoded to com_content? if it is then the docblock is now wong
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.
It's a default value, not a hardcoded lock.
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.
@brianteeman No, I did this not only for com_content, but for the subsequent move in all components to pass layout to form the correct reference.
For example, in com_contacts or in third-party extensions that use com_categories
And it's isn't a hardcoded lock
As I wrote earlier this foundation. @mbabker is right if layout is used in menu items, it must be in links.
* @param integer $id The route of the content item. | ||
* @param integer $catid The category ID. | ||
* @param integer $language The language code. | ||
* @param integer $id The route of the content item. |
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.
Fix space.
Note: Commonly a line after the tag @param consists of the following three parts in order of appearance:
variable type (There must be 3 spaces before variable type.)
variable name (There must be 2 spaces after the longest type.)
variable description (There must be 2 spaces after the longest variable name.)
If there are more than one @param the type, names and description have to be aligned.
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.
@Quy check please
please modify title of pr. will test tomorrow. |
@infograf768 What is more correct name?. |
@mbabker Found another problem association (If three menu items or not standard associations) Total with multilanguage there are 4 or 6 tests. I'll immediately show the languages Two menu items with the same layouts
Works correctly Two menu items with the different layouts
Work incorrectly Three menu items with standard associations
Works correctly Three menu items with not standard associations
Work incorrectly Four menu items with standard associations
Works correctly Four menu items with not standard associations
Work incorrectly And this is only the menu items, and there is still a layout in the Item Options.... Now, with a little bit of the number of different conditions, I understand that #19681 in fact nothing has been fixed I looked and to tell you the truth, I was wrong, the route is nothing to do with it. It is necessary to somehow change the associations function . And I have no idea how to do it correctly After all, there is simply to receive all the appropriate menu items and then check to see if the solution is not the best But this PR does not concern this problem. My updated task was to make the correct transfer of layout in the links. To fix empty layout bug, make the code more correct, and add the transfer layout in the links A little later, when the situation with the current PR is decided, I will create a new Issue where I will list once again all the necessary tests. And maybe together we can come up with the right solution. This PR is renamed. From the description, I remove the |
Using your patch, it keeps working as described above. |
@infograf768 It is necessary to look at the url in the browser. Without my patch, There was a bug with an empty layout That's why worked with different layouts. If fact ignored layout
I will not argue. My business is to warn
If you looked at the code, you would notice much more changes =) But in general, yes, a bug with an empty layout that fixes Somebody test please |
I evidently looked at the urls in the browser... As I repeatedly said, for the cases you describe and which I have tested above, there was NO ISSUE at all. There is NO empty layout added to the urls produced. Category Blog and List ARE differentiated. Your patch solves other stuff. In any case, as I had to manually enter your modifications, I suggest you solve the conflict OR close this PR and start a new one with staging as base. |
If you try to create a (com_content) category URL (route it)
then you will get a &layout=VAL or &layout="empty" After latest commits in this PR, the solution seems to better than #19681 but i have not tested this, i just reviewed the code changes a little |
That is exactly what I have said from the beginning... See the example I gave: #20211 (comment) . Is my English so bad? |
sorry this discussion is a little long, i admit i did not read everything you wrote |
@infograf768 Again! In 19683 only one link to one article_id( Array of links to be more precise). Not considering layout. And maybe other variables. This can cause negative consequences Again, the approach itself to pr does not eliminate the second not needed reference to the database and understand that the second time calls getAssociations, but simply do it by quickly saving the query result to an array. The same thing happened from 19681.
But no one looked in detail at the approach, nor on the correctness, nor on the possible consequences. Perhaps because of the lack of time, perhaps because of the large amount of work, well, or just decided that the author was convinced in everything. To what I write this. I do not want to blame anyone for anything. And of course author have to check everything yourself all the time before PR. Simply when you create PR it would be desirable to be assured that all will be checked up and if it is necessary specified on errors. Maybe I'm wrong, but as for me it's not right I can create a new PR so that there is no conflict, but to be honest I do not see the point and I do not have that much time to deal with the bug itself described in 19683 |
About #19683
For example:
I do not exclude the fact that |
Thoughts out loudWhat do you think? Well, if optimize the code of this function, it's better to do it before foreach. getting at once all the necessary items and not one at a time It is possible to store the parameters of the item in the array... so that if there is no reference to a particular layout, do not make an extra request ... Hmm, and can initially get all possible layout for each item? And if we pass all items data without links to a separate array. That is, first get all the data and then already in turn use them in the formation of links. But then we get two foreach Or, not to make a complex array, we can initially make several arrays and a separate function for retrieving article data
The main problem is that if there are no articles in the article parameters, then you will get two requests to receive article data About something else noticed.
This, too, must be corrected Maybe tomorrow I'll try to make a new PR But here's what title to use now Although the best solution would be to roll back the incorrect #19683, then take this PR if everyone likes a bug with incorrect associations that will remain from #19681 Or in general to roll back both PR #19683 and #19681 In order to restore the normal functioning of sites, which was broken after 3.8.7 And having spent much more time not to do hotfixed and to revise the entire code of associations so that all possible variables and fix all known bugs
P.S Tomorrow I can make PR for 4.0 which add layout to links for com_content and com_contact, including associations and modules. This can be done quite quickly. In 4.0 there is only one router therefore it will be easier to find bugs By the way, why in #19683 was not done like in 4.0 where this query is simply removed |
Something like that |
@mbabker @brianteeman @infograf768 @ggppdk The check for state and access must be done here Maybe add parameters to functions |
try 3 |
Pull Request for Issue #20199.
Summary of Changes
Fix incorrect layout in category link after #19681 PR
Reasons
layout=
orlayout=templatelayout
in com_content category linkThis bug is fraught with the fact that search results could get duplicate pages. And some No Dubles \ Canonical Links Plugins began to cause endless redirects
How reproduce bug by @Quy
http://localhost/joomla387/index.php/using-joomla/extensions/components/content-component/article-category-blog?layout=
Testing Instructions
If you have this bug, apply the patch
Expected result
Link was
/category
Actual result
Now link
/category?layout
= or/category?layout=templatelayout
if override com_content category view