Skip to content
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

Closed
wants to merge 9 commits into from
Closed

[com_content] Correctly adding layout to links #20211

wants to merge 9 commits into from

Conversation

Septdir
Copy link
Contributor

@Septdir Septdir commented Apr 22, 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

@Septdir Septdir changed the title [com_content] Fix category link (Revert #19681) [com_content] Fix category link and category layout in multilanguage association items (Revert #19681) Apr 22, 2018
@Septdir Septdir changed the 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) Apr 22, 2018
@Septdir Septdir changed the 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) Apr 22, 2018
@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@brianteeman
Copy link
Contributor

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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@brianteeman
Copy link
Contributor

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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@infograf768
Copy link
Member

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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 2018

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

@infograf768
Copy link
Member

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

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@infograf768
Copy link
Member

infograf768 commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@ReLater
Copy link
Contributor

ReLater commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 2018

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

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@infograf768
Copy link
Member

infograf768 commented Apr 22, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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;
	}

``

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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.

@mbabker
Copy link
Contributor

mbabker commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 22, 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.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 23, 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 ?

*
* @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)
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

@Septdir Septdir Apr 23, 2018

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.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Quy check please

@infograf768
Copy link
Member

please modify title of pr. will test tomorrow.

@Septdir
Copy link
Contributor Author

Septdir commented Apr 23, 2018

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

@Septdir Septdir changed the title [com_content] Fix category link and displayed layout in multilanguage association items (Revert #19681) [com_content] Correctly adding layout to links Apr 23, 2018
@Septdir
Copy link
Contributor Author

Septdir commented Apr 23, 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

@infograf768
Copy link
Member

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.

@infograf768
Copy link
Member

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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 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

@infograf768
Copy link
Member

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.

@ggppdk
Copy link
Contributor

ggppdk commented Apr 24, 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

@infograf768
Copy link
Member

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

@ggppdk
Copy link
Contributor

ggppdk commented Apr 24, 2018

@infograf768

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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 2018

Something like that

@Septdir
Copy link
Contributor Author

Septdir commented Apr 24, 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

@Septdir
Copy link
Contributor Author

Septdir commented Apr 25, 2018

try 3
New PR #20229

@Septdir Septdir closed this Apr 25, 2018
@Septdir Septdir deleted the revert-19681 branch April 26, 2018 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet