? ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
8 Jun 2018

Pull Request for Issue #20667.

Summary of Changes

Adds a check function if associations is available for categories of the current component.

Testing Instructions

  • Enable the Language filter plugin and make sure Associations is set to Yes
  • Display Articles Categories Manager

Expected result

The Associations column should display.

Actual result

No Associations column is displayed.

avatar laoneo laoneo - open - 8 Jun 2018
avatar laoneo laoneo - change - 8 Jun 2018
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 8 Jun 2018
Category Administration com_categories Libraries
avatar laoneo laoneo - change - 8 Jun 2018
Labels Added: ?
avatar carlitorweb carlitorweb - test_item - 8 Jun 2018 - Tested successfully
avatar carlitorweb
carlitorweb - comment - 8 Jun 2018

I have tested this item successfully on b2f878b


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

avatar infograf768
infograf768 - comment - 8 Jun 2018

@laoneo
Will test but I do not understand what is going on here. Can you explain how
\JLoader::register($hname, JPATH_SITE . '/components/' . $component . '/helpers/association.php');
may work when there is no such file for com_content as you moved and changed the file, its name and its class?

avatar mbabker
mbabker - comment - 8 Jun 2018

may work when there is no such file for com_content as you moved and changed the file, its name and its class?

Simple. That code path won't execute in the case of com_content because of the checks above it. So if a component follows the service architecture, it'll get what it needs from that and return. If it doesn't, then it'll try to locate the class and file the "old" way.

avatar infograf768
infograf768 - comment - 9 Jun 2018

@laoneo

I just don't know that detail. Perhaps @infograf768 can shed some light here.

What was done when was implemented com_associations was to make sure to define for each component its possible associations.

For example for com_contacts in the administrator/components/com_contact/helpers/associations.php the getType()` method (and also in getAssociations() and getItem() methods btw). Same for com_newsfeeds and also in its specific repo for weblinks.

If you look at com_menus similar file, you will see that category is not part of the code.

For com_content you moved the file and changed the class: similar methods are present in administrator/components/com_content/Helper/AssociationsHelper.php

My question above would have been more clear is I had asked why you decided to change the former structure for com_content ONLY as it was working perfectly OK and is still doing so for the other components. The Service Architecture is rather Tibetan for me ( https://en.wikipedia.org/wiki/Service-oriented_architecture ) as you may guess ? .
Or, if I understand well, it means you will also change the way it is done for the other components in core in the near future as it would look unclean to me.

avatar infograf768
infograf768 - comment - 9 Jun 2018

BTW, patch works fine in admin everywhere where we display/edit categories.

avatar laoneo
laoneo - comment - 9 Jun 2018

Yes, the strategy is to bring com_content into a state where we can say it is the way to go for all extensions and then I will convert the rest. Don't worry about different set ups between core components for now.
What my question is, does have a component associations support for categories automatically when it implements associations for its entities and uses categories? Because then we can remove the mentioned check by @mbabker. Otherwise not.

avatar infograf768
infograf768 - comment - 9 Jun 2018

does have a component associations support for categories automatically when it implements associations for its entities and uses categories?

As I said, if it is coded such in the component association helper, it will. Nothing is automatic. Not sure I understand what you exactly mean otherwise.

avatar laoneo
laoneo - comment - 9 Jun 2018

I'm not talking about how it is implemented but more about the set up. Lats try it differently. Can a component has associations and categories but not associations support for categories?

avatar infograf768
infograf768 - comment - 9 Jun 2018

Can a component has associations and categories but not associations support for categories?

If it has, it means that it is wrongly coded. There is no reason to do so and I don't know of any.
When a component has categories and support associations generally speaking, then associations SHOULD work for categories.

avatar laoneo
laoneo - comment - 9 Jun 2018

Because when I remove the check function now, it is not possible (or only with an effort which we can avoid now) to add it later when J4 is released. That's why I want to have confirmation on this.

avatar infograf768
infograf768 - comment - 9 Jun 2018

TBH: if someone coding a multingual aware component which uses categories does not add the necessary code to also associate categories, this coder should just be eliminated from the surface of the Earth by any means, even illegal ones. ?

avatar infograf768
infograf768 - comment - 9 Jun 2018

@laoneo
Looks like we have some places where we need also some changes
The language filter plugin is an example
https://github.com/joomla/joomla-cms/blob/4.0-dev/plugins/system/languagefilter/languagefilter.php#L786-L794

avatar joomla-cms-bot joomla-cms-bot - change - 9 Jun 2018
Category Administration com_categories Libraries Administration com_categories com_contact com_content com_menus com_newsfeeds Libraries Modules Front End Plugins
avatar laoneo
laoneo - comment - 9 Jun 2018

Moved the helper function getAssociations to the extension helper class which is accessible through the service. This comes with the cost of a BC break. An alternative would be to introduce a new helper interface and a function in AssociationServiceInterface like:
public function getAssociationsHelper(): AssociationHelperInterface;.

Additionally all of this must be set from the service provider down to the component class. Not sure if we want to go that way. What do you guys think?

avatar infograf768
infograf768 - comment - 9 Jun 2018

This still does not work for multilingual associations between content categories for example (I guess also articles).
$cassociations is always NULL in this code

			if ($component instanceof AssociationServiceInterface)
			{
				$associations = $component->getAssociationsExtension()->getAssociationsForItem();
			}
			else
			{
				$cName = StringHelper::ucfirst(StringHelper::str_ireplace('com_', '', $option)) . 'HelperAssociation';
				JLoader::register($cName, JPath::clean(JPATH_COMPONENT_SITE . '/helpers/association.php'));

				if (class_exists($cName) && is_callable(array($cName, 'getAssociations')))
				{
					$cassociations = call_user_func(array($cName, 'getAssociations'));
				}
			}

We need $cassociations further down

					// Component association
					case (isset($cassociations[$i])):
						$language->link = Route::_($cassociations[$i] . '&lang=' . $language->sef);
						break;
avatar infograf768
infograf768 - comment - 9 Jun 2018

I tested changing in mod_languages as well as languagefilter
$associations = $component->getAssociationsExtension()->getAssociationsForItem();
to
$cassociations = $component->getAssociationsExtension()->getAssociationsForItem();
and it looks like it solves my issues., i.e. when no specific menu item to display an associated article or an associated category, we can switch OK (not so nice URLS but it works):

For example the display of a sub category
http://localhost:8888/newfolder/joomla40/fr/component/content/category/10-catfrenchsub
will give when using the lang switcher to en-GB
http://localhost:8888/newfolder/joomla40/en/component/content/category/11-catenglishsub

I am rather expecting (as I have a hidden List All Categories menu item for each language with alias categories) something like
http://localhost:8888/newfolder/joomla40/en/categories/11-catenglishsub in en-GB
and
http://localhost:8888/newfolder/joomla40/fr/categories/10-catfrenchsub in fr-FR

But I guess this is due to the "modern router" in 4.0 (as it does work fine in staging)

avatar laoneo
laoneo - comment - 9 Jun 2018

Can you test again, changed to $cassociations.

534e870 9 Jun 2018 avatar laoneo CS
avatar infograf768
infograf768 - comment - 10 Jun 2018

It solves what it did in my test. The remaining issues are the urls produced when no menu items.
For example with associated articles which categories are also associated I get
French
http://localhost:8888/newfolder/joomla40/fr/component/content/article/3-article-fr-fr-2?catid=10
English
http://localhost:8888/newfolder/joomla40/en/component/content/article/4-article-en-gb-2?catid=11
@Hackwar @csthomas

@wilsonge For me this PR is a good step and can be merged, except if it demonstrated that it has an impact on the routing.

Note: wanted to test when associating contacts and contacts categories (as they use the old way) but it is broken. Will create a new issue.
PHP Fatal error: Cannot declare class Joomla\Component\Contact\Site\Helper\Route, because the name is already in use in /Applications/MAMP/htdocs/newfolder/joomla40/components/com_contact/helper/route.php on line 23

avatar infograf768
infograf768 - comment - 14 Jun 2018

In fact, it has an impact on routing, which means that much more has to be done.
When using this patch, switching between home pages is also incorrect.
Let's say I am on a fr home page
url is http://localhost:8888/testnew/joomla40/fr
The language switcher to en gives the url http://localhost:8888/testnew/joomla40/en/component/content/category/11-category-en-gb instead of http://localhost:8888/testnew/joomla40/en
same for other languages.
This breaks multilang login on these pages.

avatar csthomas
csthomas - comment - 14 Jun 2018

There is an issues with router and lang parameter in URL.

The link generated from association looks like:
index.php?option=com_content&view=category&id=8&lang=pl-PL&lang=pl

Associations use $language->lang_code but router requires $language->sef.

A quick fix (for associations with menu items) is to replace if andelseif places in below code.

if (isset($cassociations[$language->lang_code]))
{
$language->link = Route::_($cassociations[$language->lang_code] . '&lang=' . $language->sef);
}
elseif (isset($associations[$language->lang_code]) && $menu->getItem($associations[$language->lang_code]))
{
$itemid = $associations[$language->lang_code];
$language->link = Route::_('index.php?lang=' . $language->sef . '&Itemid=' . $itemid);
}

IMO the menu association takes precedence over category associations.

avatar laoneo
laoneo - comment - 15 Jun 2018

@infograf768 does the routing work correct without the patch? Can you give me step by step instructions how I can reproduce the routing issue?

avatar infograf768
infograf768 - comment - 15 Jun 2018

@csthomas
category or item associations have always taken precedence over menu associations as you can see in the code order in this helper as well as in the languagefilter plugin.
I.e. $cassociations (categories and items) over $associations (menu items)
we may indeed reconsider this. Not sure.
But I do not understand your comment about $language->sef as it correctly set in both files concerned.
also not sure what you mean by dropping if and elseif

Here, on my 4.0 multilingual test site, SEF off, I get for associated articles
index.php?option=com_content&view=article&id=7&catid=15&lang=fr
which is perfectly OK.
and for category
index.php?option=com_content&view=category&id=15&lang=fr
which is perfectly fine too.

EDIT: results obtained with this patch!

Therefore the issue we have is when we use SEF.

avatar infograf768
infograf768 - comment - 15 Jun 2018

@laoneo
Just an example when SEF on: the home pages.
Each is a blog menu item (using the new multilingual sample data helps a lot to set up fastly such a test site)
Without your patch, the url obtained via the language switcher gives the correct ones (In 3.x there is also a slash at the end):
http://localhost:8888/testnew/joomla40/el for Greek
http://localhost:8888/testnew/joomla40/en for English
http://localhost:8888/testnew/joomla40/fr for French

See above what I get:
#20692 (comment)

But, still without your patch, clicking on the article title in this home page is also wrong when switching
For example (I did not remove default language code). Both the categories of these articles and these articles are associated.
original page
http://localhost:8888/testnew/joomla40/fr/7-article-fr-fr
gives, which indeed displays the article concerned but only because they are displayed by their home page.
http://localhost:8888/testnew/joomla40/el for Greek...
http://localhost:8888/testnew/joomla40/en for English

With your patch, it gives
original: http://localhost:8888/testnew/joomla40/fr/7-article-fr-fr as above
association
Greek: http://localhost:8888/testnew/joomla40/el/component/content/article/8-%CE%AC%CF%81%CE%B8%CF%81%CE%BF-el-gr?catid=16 ( I am using unicode alias for that Greek article)
English: http://localhost:8888/testnew/joomla40/en/component/content/article/6-article-en-gb?catid=14

Once switched, when I switch back to French
http://localhost:8888/testnew/joomla40/fr/component/content/article/7-article-fr-fr?catid=15

Also login works fine from the module. Depending on the language, I may get a 404 with your patch.
EDIT: with your patch, the 404 are obtained when the url of the page (or the associated page if a redirect is done to the user preferred language) is of the type
http://localhost:8888/testnew/joomla40/fr/component/content/article/7-article-fr-fr?catid=15

avatar csthomas
csthomas - comment - 15 Jun 2018

Only for debug

To get an info for debug I did:

  • turn off global SEF
  • turn off plugin System Sef
  • replace line 133 (after patch) from $language->link = Route::_($cassociations[$language->lang_code] . '&lang=' . $language->sef);
    to $language->link = $cassociations[$language->lang_code] . '&lang=' . $language->sef;

Then I get weird link like .../index.php?option=com_content&view=category&id=9&lang=en-GB&lang=en
where the "lang" parameter is repeated.

Solution

To get a correct link you can remove the lang parameter as below:
$language->link = Route::_($cassociations[$language->lang_code]);
then all seems to be OK.

avatar laoneo
laoneo - comment - 15 Jun 2018

SO I get it right, with and without the patch it is broken? If yes, can we do the routing problem in another pr then? Otherwise this one turns into a "fix all language problem" pr.

avatar infograf768
infograf768 - comment - 15 Jun 2018

@csthomas
This indeed works for com_content but maybe because $cassociations is now coming from

			if ($component instanceof AssociationServiceInterface)
			{
				$cassociations = $component->getAssociationsExtension()->getAssociationsForItem();
			}

@laoneo
Not sure at all that it would work for other components that have not been touched by your patch.
Before changing, we must test with other comps.

avatar infograf768
infograf768 - comment - 15 Jun 2018

hmm, dumping $cassociations here and from 3.x gives the same type of array.
What has the hell changed in 4.0 with route vs 3.x ?
Tested with Newsfeeds and, alas, I get a similar error as contacts
[15-Jun-2018 10:15:05 Europe/Berlin] PHP Fatal error: Cannot declare class Joomla\Component\Newsfeeds\Site\Helper\Route, because the name is already in use in /Applications/MAMP/htdocs/testnew/joomla40/components/com_newsfeeds/helper/route.php on line 20

avatar infograf768
infograf768 - comment - 15 Jun 2018

@laoneo, can you solve this issue with newfeeds so that we can test?

avatar infograf768
infograf768 - comment - 15 Jun 2018

My limited tests here show that single newsfeeds associations is more broken when we use @csthomas trick.

avatar csthomas
csthomas - comment - 15 Jun 2018

Multilingual links use ...&lang=en or ...&lang=en-GB. There are no consequence.

The class variable Joomla\CMS\Component\Router\Rules\MenuRules::lookup has different keys ('en', 'en-GB') for the same language.
Menu items with selected language appear only in the longer key (for English in 'en-GB').

For com_content the link (after this PR) looks like
index.php?option=com_content&view=category&id=9&lang=en-GB&lang=en.

after my trick it will be:
index.php?option=com_content&view=category&id=9&lang=en-GB (5 chars).

but in other places joomla still uses ...&lang=en. (2 chars)

The main question is, which one is correct?

avatar infograf768
infograf768 - comment - 15 Jun 2018

$language->sef is normally ok, i.e. the content language url code i.e. en
in 3.x joomla also understands the language tag i.e en-GB
not at my desk but the editors xtd do use I think the tag and no issue

avatar csthomas
csthomas - comment - 15 Jun 2018

I prepared a patch which should fix the problem, the above trick is not needed.

diff --git a/libraries/src/Component/Router/Rules/MenuRules.php b/libraries/src/Component/Router/Rules/MenuRules.php
index ab14b509a2..1ea8ed866f 100644
--- a/libraries/src/Component/Router/Rules/MenuRules.php
+++ b/libraries/src/Component/Router/Rules/MenuRules.php
@@ -75,6 +75,17 @@ class MenuRules implements RulesInterface
                // Get query language
                $language = isset($query['lang']) ? $query['lang'] : '*';
 
+               if ($language !== '*')
+               {
+                       // Translate the language sef to lang_code
+                       $languages = \Joomla\CMS\Language\LanguageHelper::getLanguages('sef');
+
+                       if (isset($languages[$language]))
+                       {
+                               $language = $languages[$language]->lang_code;
+                       }
+               }
+
                if (!isset($this->lookup[$language]))
                {
                        $this->buildLookup($language);
avatar infograf768
infograf768 - comment - 15 Jun 2018

Does not solve the single Newsfeeds associations.

For the blog home pages, still same issue: missing the slash after the url language code.

Otherwise, no login error any more for article without specific menu item WHEN THEY are associated as I get "normal" urls.
http://localhost:8888/testnew/joomla40/fr/7-article-fr-fr
http://localhost:8888/testnew/joomla40/en/6-article-en-gb
http://localhost:8888/testnew/joomla40/el/8-%CE%AC%CF%81%CE%B8%CF%81%CE%BF-el-gr

But, if articles are not associated AND their categories are associated, the switcher sends me to the home pages instead of using the allcategories menu item as a base for their urls.

avatar infograf768
infograf768 - comment - 15 Jun 2018

BTW, I checked the xtd code when entering an article link in an article

<p><a href="index.php?option=com_content&amp;view=article&amp;id=6&amp;catid=14&amp;lang=en-GB" hreflang="en">Article (en-gb)</a></p>

so, I was right that it does use the tag and not the sef. URL produced is fine ( http://localhost:8888/testnew/joomla40/en/6-article-en-gb )

avatar laoneo
laoneo - comment - 15 Jun 2018

I will convert newsfeed later to services, then it is very likely that the issue will get fixed too. It was the case as I converted com_config which solved a lot of front end editing stuff for module editing and template/sysconfig changes in front. So please lets not mix things here.

avatar infograf768
infograf768 - comment - 16 Jun 2018

@laoneo
I understand. I am just worried about B/C here for 3pd components.

avatar laoneo
laoneo - comment - 16 Jun 2018

I'm pretty sure that the issue is because the components do get namespaces and not yet migrated to services. An old legavy component should work as beford without a BC break.

avatar infograf768
infograf768 - comment - 16 Jun 2018

@Bakual
can you give us the link to your Sermon Speaker component, the version you already adapted to 3.10/4.0 ?

avatar Bakual
Bakual - comment - 16 Jun 2018
avatar infograf768
infograf768 - comment - 16 Jun 2018

I have issues with sermonspeaker
It has many errors, including a Fatal one
[16-Jun-2018 10:42:06 Europe/Berlin] PHP Fatal error: Class SermonSpeakerAssociationsHelper contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Joomla\CMS\Association\AssociationExtensionInterface::getAssociationsForItem) in /Applications/MAMP/htdocs/testnew/joomla40/administrator/components/com_sermonspeaker/helpers/associations.php on line 228

Also many Notices and Undefined Property and com_associations does not display at all.

As for urls obtained, no good, which I guess is expected when we consider the errors:
A menu item like http://localhost:8888/testnew/joomla40/en/sermonspeaker-cat-list-en displays one category
screen shot 2018-06-16 at 10 40 07

Clicking on the title of this category gives a 404 therefore I can't test if the associated category works or not.
Frontend Notice:
[16-Jun-2018 10:52:57 Europe/Berlin] PHP Notice: Use of undefined constant JPATH_COMPONENT_SITE - assumed 'JPATH_COMPONENT_SITE' in /Applications/MAMP/htdocs/testnew/joomla40/modules/mod_languages/Helper/LanguagesHelper.php on line 82

Are these due to the component or 4.0 with services and this patch? No idea.

avatar Bakual
Bakual - comment - 16 Jun 2018

Hard to say. I've only rewritten it for Joomla 4.0 alpha from last year and the release is from january. There were quite a lot of major changes to the repo since then. I'm not constantely adjusting it to the latest changes in architecture :)

avatar laoneo
laoneo - comment - 16 Jun 2018

Try it with the j3 version as we should support them and we need to make sure these versions do work. Not one which is made for J4 alpha 1.

avatar Bakual
Bakual - comment - 16 Jun 2018

The J3 version likely doesn't work as it uses various deprecated functions.
My plan was to get it working in J4 and then "backport" whatever is needed to get a "transition" version that works both in 3.10 and 4.0.
But that obviously failed as I started to early ?

avatar wilsonge
wilsonge - comment - 20 Jun 2018

@laoneo can you fix the conflicts with this one so I can merge it please?

avatar laoneo
laoneo - comment - 26 Jun 2018

Merged upstream and fixed conflicts.

avatar richard67 richard67 - test_item - 30 Jun 2018 - Tested successfully
avatar richard67
richard67 - comment - 30 Jun 2018

I have tested this item successfully on 720f944

Tested on current 4.0-dev + this PR applied.
Associations column in categories manager is shown.
Associations tab in category edit view is shown.
Category associations can be added/changed/edited/used as it should be.


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

avatar infograf768
infograf768 - comment - 6 Jul 2018

@wilsonge Can this be merged?

avatar infograf768
infograf768 - comment - 13 Jul 2018

I have tested this item successfully on 720f944

May be incomplete, but without it in core we can't test associations at least for com_content.

Please, again, @wilsonge , merge.


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

avatar infograf768
infograf768 - comment - 13 Jul 2018

I have tested this item successfully on 720f944

May be incomplete, but without it in core we can't test associations at least for com_content.

Please, again, @wilsonge , merge.


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

avatar infograf768 infograf768 - test_item - 13 Jul 2018 - Tested successfully
avatar infograf768 infograf768 - change - 13 Jul 2018
Status Pending Ready to Commit
avatar infograf768
infograf768 - comment - 13 Jul 2018

RTC


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

avatar infograf768 infograf768 - change - 13 Jul 2018
Labels Added: ?
avatar infograf768
infograf768 - comment - 13 Jul 2018

Updated branch to allow merging.

avatar infograf768
infograf768 - comment - 13 Jul 2018

Restarting drone.

avatar brianteeman
brianteeman - comment - 13 Jul 2018

@infograf768 i have no idea what that error is but its been there for a while - i did ask what it was but got no response

avatar laoneo laoneo - change - 13 Jul 2018
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2018-07-13 12:57:48
Closed_By laoneo
avatar laoneo laoneo - close - 13 Jul 2018
avatar laoneo laoneo - merge - 13 Jul 2018
avatar laoneo
laoneo - comment - 13 Jul 2018

Going to merge this one to move on with associations testing. Thanks to the testers. Please provide the routing patch in a new pr.

avatar infograf768
infograf768 - comment - 13 Jul 2018

Thanks @laoneo
We certainly will need quite a few modifications/improvements specially for legacy but at least people working on multilang will have one possible test.

avatar laoneo
laoneo - comment - 13 Jul 2018

For legacy everything should work as before. We just extended it with services.

avatar infograf768
infograf768 - comment - 13 Jul 2018

@laoneo

I still get issues in back-end com_associations for com_contact for example (who has its associations class in a different file/location and with a different type of class name).

Here I still get for single contact

Notice: Trying to get property of non-object in /Applications/MAMP/htdocs/installmulti/joomla40/administrator/components/com_associations/Helper/AssociationsHelper.php on line 578
and
Undefined index: language in /Applications/MAMP/htdocs/installmulti/joomla40/administrator/components/com_associations/Field/ItemlanguageField.php on line 55
and
Call to a member function checkin() on null

Did not test but I guess it will be the same for newsfeeds.

avatar infograf768
infograf768 - comment - 13 Jul 2018

I suggest to solve this one before modifying these components to use services.

avatar laoneo
laoneo - comment - 13 Jul 2018

Please open a new issue as I don't think it is related to services at all.

Add a Comment

Login with GitHub to post a comment