? Pending

User tests: Successful: Unsuccessful:

avatar Septdir
Septdir
22 Apr 2018

Pull Request for Issue #20199.

Summary of Changes

Fix incorrect layout in category link after #19681 PR

Reasons

  1. Bag with layout= or layout=templatelayout in com_content category link
    This 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

  • Install the last demo.
  • Under System > Global Configuration > Articles > Integration > URL Routing, select Modern
  • Search Park Blog
  • Hover Park Blog link to see URL:
    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

avatar Septdir Septdir - open - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 22 Apr 2018
Category Front End com_content
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
Title
[com_content] Fix category link (Revert #19681)
[com_content] Fix category link and category layout in multilanguage association items (Revert #19681)
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
Title
[com_content] Fix category link and category layout in multilanguage association items (Revert #19681)
[com_content] Fix category link and corect layout in multilanguage association items (Revert #19681)
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
Title
[com_content] Fix category link and corect layout in multilanguage association items (Revert #19681)
[com_content] Fix category link and displayed layout in multilanguage association items (Revert #19681)
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir
Septdir - comment - 22 Apr 2018

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
The plugins check to ensure that there are no unnecessary parameters in the url of the page, except for the permitted ones like start or filter. In this case, the url was with the layout=
If the link incorrect plugins do redirect to the url received from helpers / route in which again there was layout=
And the infinite cycle will begin

To me, not so many people turn, so, such a count of appeals personally bothers me

avatar brianteeman
brianteeman - comment - 22 Apr 2018

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

avatar Septdir
Septdir - comment - 22 Apr 2018

@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
And the PR in the comments had a code to fix this bug

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

avatar brianteeman
brianteeman - comment - 22 Apr 2018

so i am still confused. is this an issue with the core of joomla or a problem that only occurs when using your extension.

avatar Septdir
Septdir - comment - 22 Apr 2018

@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.
@brianteeman please answer. You often check all links on your sites after core update?

And this is the core problem.

  1. If I use JRoute::_(ContentHelperRoute::getCategoryRoute($item->id)); I want to get a normal link /category And not this /category?layout= or /category?layout=templatelayout or /category?layout=templatetags_layout in com_tags
    In a week or two, these links will go to google and there will be a bunch of page duplicates
  2. The problem for me personally is not so relevant however. If I specifically create a menu item with another layout for the second language. I want him out.
    Because of this problem, I decided that I had to completely remove the automatic addition of layout to the link
    How to check the problem in multilanguage I described in detail in the description of this PR
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar Septdir Septdir - change - 22 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 22 Apr 2018
avatar mbabker
mbabker - comment - 22 Apr 2018

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, index.php?option=com_content&view=category&layout=blog&id=X is pretty common), as in the very explicit use case pointed out in the original PR.

avatar Septdir
Septdir - comment - 22 Apr 2018

@mbabker Are you sure about that?
I will give an example:

  1. I create menu item Category Blog for en-GB category
  2. I create menu item Category List for de-DE category (The reasons for using another layout can be a lot)
  3. Create a Associates for this menu items
  4. I go site when go en-GB category and see Category Blog layout, all fine
  5. I switch language (click to language flag )
  6. After redirect to de-DE category i see de-DE category in Category Blog layout. this is not true!
    I create Category List layout for this language and i want see this layout. If i want see this category in Category Blog` layout i crate menu item with this layout, Or I'm not right?

If I do not understand clearly please follow the instructions beginning with Fix link when layout and association in this PR and understand what I'm saying

With the original PR #19681 I will have to completely override associates functions or register links manually for each category

avatar mbabker
mbabker - comment - 22 Apr 2018

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.

avatar infograf768
infograf768 - comment - 22 Apr 2018

@Septdir
Using default core, I can't reproduce your example.
I have here a Category list menu item for en-GB ad a Category blog menu item for fr-FR.
I associate them.

screen shot 2018-04-22 at 17 09 54

Switching via the language switcher DOES display the correct layout in both cases.

avatar Septdir
Septdir - comment - 22 Apr 2018

@mbabker This PR does not cause a bug. Because it was not originally.

avatar infograf768
infograf768 - comment - 22 Apr 2018

@Septdir
The original PR #19681 IS solving the category blog/list confusion that we had before.

avatar mbabker
mbabker - comment - 22 Apr 2018

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:

  • In one language, create a "Category Blog" menu item for category X
  • In another language, create a "Category List" menu item for category X
  • Associate the two

The use case the PR addressed:

  • In one language, create a "Category Blog" and "Category List" menu item for category X
  • In another language, create a "Category Blog" and "Category List" menu item for category X
  • Associate the similar menu items

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.

avatar infograf768
infograf768 - comment - 22 Apr 2018

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
http://localhost:8888/joomla/en/?view=category&id=8&layout=
and if I have a menu item displaying all categories in that language, I get:
http://localhost:8888/joomla/en/allcategories-en-gb/category-en-gb.html?layout=
The category displays fine though.

Therefore, when "Modern Routing" is enabled, looks like we still have an isssue.

avatar Septdir
Septdir - comment - 22 Apr 2018

@mbabker It's worthwhile to add layout to the links, but obviously not as it was done.
It is necessary not only to add a parameter to the route.php layout, but also to take into account the menu items in multilinguality And many other factors, you also need to add layout to other components.

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

avatar ReLater
ReLater - comment - 22 Apr 2018

Therefore, when "Modern Routing" is enabled, looks like we still have an isssue.

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.

avatar Septdir
Septdir - comment - 22 Apr 2018

@ReLater Yes, this PR eliminates this problem.
In #20200 I just did not see hurriedly did not notice the other.

avatar mbabker
mbabker - comment - 22 Apr 2018

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.

avatar infograf768
infograf768 - comment - 22 Apr 2018

@ReLater
#19681 solved the list/blog confusion, but it did not solve what I described. In these cases I have no category menu item, just the category link in the article bloc.
I mean this:
screen shot 2018-04-22 at 17 44 58

avatar Septdir
Septdir - comment - 22 Apr 2018

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.
However, I am a little scared of the consequences of the original PR.

@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

avatar Septdir
Septdir - comment - 22 Apr 2018

@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


		public static function getCategoryRoute($catid, $language = 0, $layout = 0)
	{
		if ($catid instanceof JCategoryNode)
		{
			$id = $catid->id;
		}
		else
		{
			$id = (int) $catid;
		}

		if ($id < 1)
		{
			$link = '';
		}
		else
		{
			$link = 'index.php?option=com_content&view=category&id=' . $id;

			if ($language && $language !== '*' && JLanguageMultilang::isEnabled())
			{
				$link .= '&lang=' . $language;
			}
			
			if ($layout)
			{
				$link .= '&layout=' . $layout;
			}
		}

		return $link;
	}

``
avatar mbabker
mbabker - comment - 22 Apr 2018

The problem is exactly as described in the original PR:

  • Create a "Category Blog" menu item for Category X in en-GB
  • Create a "Category List" menu item for Category X in en-GB
  • Create a "Category Blog" menu item for Category X in (insert second language here)
  • Create a "Category List" menu item for Category X in (insert second language here)
  • Create associations for the "Category Blog" menu items
  • Create associations for the "Category List" menu items

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 (layout=blog) then you should stay on that layout, similar for "Category List" (layout=default). The bug was that when on the "Category Blog" layout, because the layout wasn't considered for routing, you would switch to the "Category List" layout.

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.

avatar Septdir
Septdir - comment - 22 Apr 2018

@mbabker

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.
Because if I need to use the same layout, I'll create a menu item with the same layout for another language
I can give you an example.

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.

avatar mbabker
mbabker - comment - 22 Apr 2018

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.

avatar Septdir
Septdir - comment - 22 Apr 2018

@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 templatelayout and should have been template:layout Also it is applied only in com_content \ category and that personally at me in com_tags links to categories com_content were with an incorrect layout In addition, it also breaks the conclusion from those who have previously worked as needed

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.

avatar Septdir
Septdir - comment - 22 Apr 2018

Also in associations, it is worth giving priority to menu items. First they were checked, but there are problems. First with current layout next links with any another layout. And then only items.
The user may need to make the associations as follows.
For the English category, for the German article

One way or another this is a very extensive topic. and it should be discussed separately. and in more detail.

Therefore, in my opinion, it is worth simply returning as it was originally. Thus, restoring the work of the sites that were published. And then you have to figure out how to do better.

But this is only my opinion. And my approach. If I admit any error that leads to a bug, and I can not quickly figure out how to do it correctly. I just apologize. I get hotfix with revert and then do not hurry up to invent something.

avatar Arkadiy-Sedelnikov
Arkadiy-Sedelnikov - comment - 22 Apr 2018

Support @Septdir, to solve one problem generating many other not correctly. It is better to roll back to the previous state and think about how to solve this problem correctly.

avatar pavluk
pavluk - comment - 22 Apr 2018

I have tested this item ✅ successfully on e717f38


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar pavluk pavluk - test_item - 22 Apr 2018 - Tested successfully
avatar CB9TOIIIA
CB9TOIIIA - comment - 23 Apr 2018

Support @Septdir, to solve one problem generating many other not correctly.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar tonypartridge
tonypartridge - comment - 23 Apr 2018

I have to agree too, whilst a bug previously existed it has caused another problem which was not originally there potentially having a detrimental SEO impact, it would be best to revert that PR and ask the original author to review their PR with the bug it creates in mind.

avatar infograf768
infograf768 - comment - 23 Apr 2018

@alikon What do you think?

avatar pavluk
pavluk - comment - 23 Apr 2018

Support @Septdir, to solve one problem generating many other not correctly.

avatar Septdir
Septdir - comment - 23 Apr 2018

I had time to think how it would be right to do what I wanted in the original PR and even more

@mbabker Absolutely right, if layout uses in the formation of a link, then it should be in it and the problems associated with the lack of layout are not limited to multilanguage. But you just need to betray it in a function.

  1. Therefore, to begin with, it is simply necessary to add variables to the helper /roure.php function. And not only in ContentHelperRoute::getCategoryRoute() but in all the components and all the functions.

  2. transfer variable to hepler/route.php wherever you need it. In views, in modules(For example, the category module must take into account the current layout) in associations helper and so on

  3. It is necessary to correct globally Modern router So that it correctly takes into account layout

  • Find the appropriate menu item given layout
  • If this is not the case, find a menu of different options (view, id, slug) and remove the layout from the url
    And a whole bunch of little things
  1. Somehow use the layout parameter in the category settings as default_layout

And this is only a small part of what needs to be done. As a matter of fact it is more to treat router than to multilanguage.
In addition, there may be a lot of mistakes. Therefore, careful testing and development will be required.

Therefore, do not even try to do this in the 3.8 branch. The solution to this problem should be in the branch 3.9 or even 4.0 in order to have enough time to catch all bugs and work it out in detail

avatar Arkadiy-Sedelnikov
Arkadiy-Sedelnikov - comment - 23 Apr 2018

I have tested this item ✅ successfully on e717f38

I have tested this item successfully


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar Arkadiy-Sedelnikov Arkadiy-Sedelnikov - test_item - 23 Apr 2018 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 23 Apr 2018
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 23 Apr 2018

Ready to Commit after two successful tests.

avatar pavluk
pavluk - comment - 23 Apr 2018

I have tested this item ✅ successfully on e717f38


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar pavluk pavluk - test_item - 23 Apr 2018 - Tested successfully
avatar brianteeman
brianteeman - comment - 23 Apr 2018

Please remove the RTC status see #20211 (comment)

avatar mbabker
mbabker - comment - 23 Apr 2018

Therefore, do not even try to do this in the 3.8 branch. The solution to this problem should be in the branch 3.9 or even 4.0 in order to have enough time to catch all bugs and work it out in detail

It's a bug, the work should be against staging unless it involves B/C breaks. The amount of effort to fix a bug does not determine the branch it lands in.

Beyond that, your assessment is pretty much on point about the route helpers that are responsible for building routes needing the layout as a parameter versus relying on whatever layout is used for the current request or only supporting the system default parameter.

#20200 before it added the component and view checks was adequate enough to keep the empty parameter from being appended to the route, that should be re-opened and restored to the state it was flagged RTC in (basically if ($layout) for that line).

As for router fixes, I honestly don't know right now what is needed there. Or if it is actually a router bug that needs to be addressed or if it's logic in front of the router (i.e. something in the language switcher module or the language system plugins where they resolve the alternative language links) that needs patching, because the issue doesn't necessarily look like it's a problem of the wrong URLs being built by the router (legacy or modern) but that the data being given to the router isn't adequate enough or the correct data to build the expected URLs in all cases.

avatar Septdir
Septdir - comment - 23 Apr 2018

@brianteeman

#20200 before it added the component and view checks was adequate enough to keep the empty parameter from being appended to the route, that should be re-opened and restored to the state it was flagged RTC in (basically if ($layout) for that line).

#20200 did not remove the bug with Fix link when layout and association when create only 2 menu items with different layouts. Prior to 3.8.7, there were no problems with this
That is only eliminated part of the problem created by the original pr. In addition, I frankly do not run into its implementation

As for router fixes,

In fact, there is no need for fix. Now route basically works incorrectly with layouts, because it was not initially transferred. For example, in com_users, there are problems with an incorrect link to editing.


My conclusions about the branches are based not so much on the spirits of the cord and strength as on the time of testing.
It was not the complete testing #19681 that led to new problems, although even after reading the description it could be understood that such an approach is erroneous.
I do not blame anyone for this. All are guilty. And people like me who did not even look at PR and those who saw only a quick solution to one problem, not seeing others.

It is worthwhile to understand that any change in routing or link formation is referenced, it can have detrimental consequences for seo. Therefore, they should not be done quickly.

3.8.8 is scheduled for May 22 and if you start doing well now, then there is very little time to take a slow and thoughtful look at everything.

And a possible negative effect on seo can happen at any time. Now people can simply not notice that their links have become ?layout, and when they notice them in the google webmaster it will be too late.

Therefore, a full revert #19681 is the right decision. With what agree @Arkadiy-Sedelnikov @tonypartridge @pavluk @CB9TOIIIA And maybe others.
The first thing it gives this time is to come up and do it right
Although if someone quickly decides all the problems without causing others, I will be only glad

P.S I would also like to know the opinion of the author of the original PR.
@alikon What do you think?

avatar mbabker
mbabker - comment - 23 Apr 2018

Whether #19861 is reverted today or just before the RC for the next release makes no difference. Whether the underlying issue gets fixed in the next release or takes 2-3 months makes no difference.

What I foresee happening is the same thing that happens so many times. An issue is identified, a quick fix is applied, and the root issue is either never identified or never fixed because the quick fix hides it. That has been the Joomla way for years and if you notice I keep yelling loudly about it because that is not how you manage a code base as massive as our's (hide the bugs with lower level quick fixes).

I don't think just reverting #19861 is the right decision because it replaces one bug for another bug, the severity of which is subjective based heavily on how the site is configured (oh yay, another bug where people are now going to comment "your site is misconfigured"). You might not run into it, but clearly there was a use case or scenario where someone did run into the bug that was fixed by #19861. Truthfully its flaw is that it allows an empty layout parameter to be included in generated links and that it relies on the current request to make an assumption, but the end result it is trying to achieve is correct (be able to correctly route to a page using the layout parameter).

avatar Septdir
Septdir - comment - 23 Apr 2018

@mbabker

Whether #19861 is reverted today or just before the RC for the next release makes no difference. Whether the underlying issue gets fixed in the next release or takes 2-3 months makes no difference.

Links with an empty or not harmful layout are not as critical to the operation of the site as to the SEO. And mistakes related to SEO should be solved as quickly as possible.


Well, how do you like this option. As part of this PR, I can do some of the necessary work. I can do it pretty quickly

Proper transfer of layout in the links. This will not fix Fix link when layout and association but will lay the foundation. It's okay to just work out the router so that it works correctly with layouts. Not only in com_content
Then you can gradually refine the remaining components and places where it is needed

In other words, I will re-arrange #19861 so that its problem only concerns those sites where 2 menu items (one for each language) with different layouts

avatar mbabker
mbabker - comment - 23 Apr 2018

Links with an empty or not harmful layout are not as critical to the operation of the site as to the SEO. And mistakes related to SEO should be solved as quickly as possible.

Right, but unless I were to put out a release today, it really doesn't matter as far as the calendar is concerned when either a revert PR or some other change that fixes all known buggy behaviors lands. That's kind of the point I wanted to make. We know what the issues are, they can be worked on, and to put it in a way that is controversial but mildly accurate, the issues are more likely to get attention when something is perceived to be broken by more users than not (i.e. as long as this PR isn't in the current git HEAD people will still run into it and hopefully run into this thread, if this gets merged it's going to be much less likely that any further work happens, and that's just a reflection on what I've seen over the years).

If this PR can be turned into something more than "revert the PR that broke my use case" then awesome. If it really has to be "revert the PR that broke my use case" then so be it, but I'm not a fan of just reverting things which fix valid bugs because they introduce new bugs without some kind of plan to make sure both issues are addressed.

avatar Septdir Septdir - change - 23 Apr 2018
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 23 Apr 2018
Category Front End com_content Administration com_categories Front End com_content
avatar Septdir
Septdir - comment - 23 Apr 2018

@mbabker Like that. It remains only to fix the Modern route itself so that it works correctly with layouts

This fix does not solve the Fix link when layout and association if only 2 menu items are created. That it is necessary to correct it is necessary to make edits in parent router, and to make in it edits I am afraid

PS Bot did not like that I made edits in content Associations helper

avatar Quy Quy - change - 23 Apr 2018
Status Ready to Commit Pending
avatar Quy
Quy - comment - 23 Apr 2018

Revert RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar pavluk
pavluk - comment - 23 Apr 2018

I have tested this item ✅ successfully on 7f78d60

fix empty layout in category links


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/20211.

avatar pavluk pavluk - test_item - 23 Apr 2018 - Tested successfully
avatar Septdir Septdir - change - 23 Apr 2018
Labels Removed: ?
avatar Septdir Septdir - change - 23 Apr 2018
Labels Added: ?
avatar Septdir
Septdir - comment - 23 Apr 2018

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 This branch has conflicts that must be resolved ?

avatar infograf768
infograf768 - comment - 23 Apr 2018

please modify title of pr. will test tomorrow.

avatar Septdir
Septdir - comment - 23 Apr 2018

@infograf768 What is more correct name?.
[com_content] Correctly adding layout to links
Would it do?

avatar Septdir Septdir - change - 23 Apr 2018
Title
[com_content] Fix category link and displayed layout in multilanguage association items (Revert #19681)
[com_content] Correctly adding layout to links
avatar Septdir Septdir - edited - 23 Apr 2018
avatar Septdir Septdir - change - 23 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 23 Apr 2018
avatar Septdir
Septdir - comment - 23 Apr 2018

@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

  1. Create Category Blog (en-GB)
  2. Create Category Blog (de-DE)
  3. Associates Category Blog (en-GB) = Category Blog (de-DE)

Works correctly


Two menu items with the different layouts

  1. Create Category Blog (en-GB)
  2. Create Category List (de-DE)
  3. Associates Category Blog (en-GB) = Category List (de-DE)

Work incorrectly


Three menu items with standard associations

  1. Create Category Blog (en-GB)
  2. Create Category Blog (de-DE)
  3. Create Category List (de-DE)
  4. Associates Category Blog (en-GB) = Category Blog (de-DE)

Works correctly


Three menu items with not standard associations

  1. Create Category Blog (en-GB)
  2. Create Category Blog (de-DE)
  3. Create Category List (de-DE)
  4. Associates Category Blog (en-GB) = Category List (de-DE)

Work incorrectly


Four menu items with standard associations

  1. Create Category Blog (en-GB)
  2. Create Category List (en-GB)
  3. Create Category Blog (de-DE)
  4. Create Category List (de-DE)
  5. Associates Category Blog (en-GB) = Category Blog (de-DE)
  6. Associates Category List (en-GB) = Category List (de-DE)

Works correctly


Four menu items with not standard associations

  1. Create Category Blog (en-GB)
  2. Create Category List (en-GB)
  3. Create Category Blog (de-DE)
  4. Create Category List (de-DE)
  5. Associates Category Blog (en-GB) = Category List (de-DE)
  6. Associates Category List (en-GB) = Category Blog (de-DE)

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 Fix link when layout and association description

avatar Septdir Septdir - change - 23 Apr 2018
The description was changed
avatar Septdir Septdir - edited - 23 Apr 2018
avatar infograf768
infograf768 - comment - 24 Apr 2018

Without your patch, I have tested here the cases you describe as working incorrectly.

  1. Two menu items with different layouts
    It works fine here.
    2menuitems

  2. Four menu items with not standard associations (let's forget about the use of the word standard. ;) )
    It works fine here too.
    4menuitems

I guess we should concentrate on what is really not working.

avatar infograf768
infograf768 - comment - 24 Apr 2018

Using your patch, it keeps working as described above.
One improvement is that I do not get anymore the layout part when no menu item for the category or drilling down to a All categories menu item.
Before:
http://localhost:8888/joomla/en/allcategories-en-gb/category-en-gb.html?layout=
after
http://localhost:8888/joomla/en/allcategories-en-gb/category-en-gb.html

avatar Septdir
Septdir - comment - 24 Apr 2018

@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 guess we should concentrate on what is really not working.

I will not argue. My business is to warn

One improvement is that I do not get anymore the layout part when no menu item for the category or drilling down to a All categories menu item.

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

avatar infograf768
infograf768 - comment - 24 Apr 2018

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 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. components/com_content/helpers/association.php has been modified since you posted this PR.

avatar ggppdk
ggppdk - comment - 24 Apr 2018

@infograf768

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.

  • the issue does not effect URLs of menu items of category type that you were testing

If you try to create a (com_content) category URL (route it)

  • for a category that does not have a menu item pointing directly to the specific category

then you will get a &layout=VAL or &layout="empty"
which depends on the layout of current page,
which is obviously wrong

After latest commits in this PR, the solution seems to better than #19681
as it should fix issue that #19681 fixed, but not introduce new issues

but i have not tested this, i just reviewed the code changes a little

avatar infograf768
infograf768 - comment - 24 Apr 2018

@ggppdk

If you try to create a (com_content) category URL (route it)
for a category that does not have a menu item pointing directly to the specific category

That is exactly what I have said from the beginning... See the example I gave: #20211 (comment) . Is my English so bad?

avatar ggppdk
ggppdk - comment - 24 Apr 2018

@infograf768

sorry this discussion is a little long, i admit i did not read everything you wrote

avatar Septdir
Septdir - comment - 24 Apr 2018

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

  • The author corrected bug.
  • Wrote Testing Instructions (where there was already a bug in Expected result).
  • A long time passed.
  • The testers were tested according to the instructions.
  • PR merged.

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

avatar Septdir
Septdir - comment - 24 Apr 2018

About #19683
But even if to do as suggested by the author at a minimum, $filters must have this structure (approximately, it takes more time to think about everything)

$filters = array(
	'view' => array(
		'language' => array(
			'id' => array(
				'layout' => 'link'
			)
		)
	)
);

For example:

$filters = array(
	'article'  => array(
		'en-GB' => array(
			10 => array(
				'default' => 'index.php?option=com_content&view=article&id=10&layout=default'
			)
		)
	),
	'category' => array(
		'en-GB' => array(
			757 => array(
				'blog' => 'index.php?option=com_content&view=article&id=10&layout=blog'
			)
		)
	)
);

I do not exclude the fact that duplicated queries were not a bug. After all, even if the query is one. links could be different. Which of course is not very good. But it is permissible.
Better odd query than wrong link

avatar Septdir
Septdir - comment - 24 Apr 2018

Thoughts out loud

What 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

  1. Associations of categories with key = id:layout Thus, excluding unnecessary associations for the category

  2. Associations of articles key = id:layout Thus, excluding unnecessary associations for the article

  3. Articles data with key = id (you can store all layout in advance, state and id) Thus excluding unnecessary requests to the database Access this array only if there is no matching item in array 2

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.
The original query does not take that state can be any if

$user->authorise ('core.edit.state', 'com_content')) || ($user>authorise ('core.edit', 'com_content')

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
And without hurrying to optimize the code #19683

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

Take a step back to make two steps forward

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

avatar Septdir
Septdir - comment - 24 Apr 2018

Something like that

avatar Septdir
Septdir - comment - 24 Apr 2018

@mbabker @brianteeman @infograf768 @ggppdk
What do you think?
If this option, with the removal of the superfluous request of all arrange, then tomorrow as there will be an opportunity, I will make a new PR with all the modifications for staging, so that there is no conflict with the branch. And correct #19314

The check for state and access must be done here
https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Language/Associations.php#L39

Maybe add parameters to functions
For example, you can add $stateField and $accessFiled with a default value of null

avatar Septdir
Septdir - comment - 25 Apr 2018

try 3
New PR #20229

avatar Septdir Septdir - change - 25 Apr 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-04-25 01:34:30
Closed_By Septdir
avatar Septdir Septdir - close - 25 Apr 2018

Add a Comment

Login with GitHub to post a comment