? ? Pending

User tests: Successful: Unsuccessful:

avatar laoneo
laoneo
1 Apr 2022

Pull Request for pr #37398.

Summary of Changes

Introduces a helper factory aware interface and converts the articles news site module to services. For backwards compatibility a new helper class was introduced and the old abstract one got deprecated.

Testing Instructions

  • Create an article
  • Create an Articles - Newsflash module

Actual result BEFORE applying this Pull Request

It works.

Expected result AFTER applying this Pull Request

It works.

avatar laoneo laoneo - open - 1 Apr 2022
avatar laoneo laoneo - change - 1 Apr 2022
Status New Pending
avatar laoneo laoneo - change - 1 Apr 2022
Title
[4.2]Introduce helper factory aware interface and convert news module to services
[4.2] Introduce helper factory aware interface and convert news module to services
avatar laoneo laoneo - edited - 1 Apr 2022
avatar joomla-cms-bot joomla-cms-bot - change - 1 Apr 2022
Category Libraries Modules Front End
avatar laoneo laoneo - change - 1 Apr 2022
Labels Added: ?
avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

@laoneo please add a line in the xml like <element>mod_article_news</element> so the module could be installable. Also remove the old file mod_article_news.php as it is moved inside the src folder, etc

avatar laoneo
laoneo - comment - 1 Apr 2022

@laoneo please add a line in the xml like <element>mod_article_news</element> so the module could be installable.

No module has such an element tag. Why should I add it?

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

Because #33182

avatar laoneo
laoneo - comment - 1 Apr 2022

This should be enough 13c1cf6. #33182 is basically obsolete as it would already work without it, see here #36042 (comment). Just one entry needs a module attribute, doesn't mater which one.

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

That's an undocumented hack and probably shouldn't be promoted! The docs are clear:
Screenshot 2022-04-01 at 15 33 18

https://docs.joomla.org/Manifest_files

avatar laoneo
laoneo - comment - 1 Apr 2022

Where do you see that in action the element tag? None of my extensions does have that element and the code which uses any of the file elements is there since Joomla 1.7, which do extensions use since ages. Your pr was for 4, so I guess you added a new behavior. I wouldn't call the original way a hack. Anyway, lets open an issue where we can discuss this in more deep if you want to get that sorted.

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

I think the project should have one unique way for defining the element especially for the namespaced extensions. When I did that PR (that was before your discovery?) I thought that normalising the optional element field and move it to the required ones would be self explanatory and also if that was to be standardised then the name field could be left free to be any type of string (translatable or not). This was somehow bite me with 4.1 as the templates name field is not explicitly defined correctly, even for the core ones, (should be tmpl_cassiopeia and not cassiopeia). Anyways either keep adding attributes to a folder or explicitly asking for an element field should be standardised and needs to be noted in the docs. People shouldn't reverse engineer code to figure out how things are working. End of off topic comments

avatar laoneo
laoneo - comment - 1 Apr 2022

Show me one extension which exists before Joomla 4 was released that uses the element tag.

avatar dgrammatiko
dgrammatiko - comment - 1 Apr 2022

I can't, apart from the one I edited in that PR but maybe this Unit can:

public function testgetElementFromElementTag()
{
$xml = file_get_contents(JPATH_ADMINISTRATOR . '/modules/mod_quickicon/mod_quickicon.xml');
// Insert a Joomla 4 <module/> tag
$xml = str_replace('<name>mod_quickicon</name>', '<name>mod_quickicon</name><element>mod_quickicon</element>', $xml);
$xml = simplexml_load_string($xml);
$this->moduleAdapter->setManifest($xml);
$this->assertNotNull($this->moduleAdapter->manifest);
$this->assertEquals('mod_quickicon', $this->moduleAdapter->getElement());
$this->assertEquals('somethingElse', $this->moduleAdapter->getElement('somethingElse'));
}

avatar laoneo
laoneo - comment - 1 Apr 2022

Ok, we leave the pr as it is for now.

avatar Ruud68
Ruud68 - comment - 2 Apr 2022

in your commit you have created a new ArticleNews.php in the src/Helper directory.
Will that replace the ArticleNewsHelper.php (as that file now only has the deprecated message)?

Asking because if that is the case, com_ajax is broken as it hardcoded looks for a module helperfile ending in 'Helper': so ArticleNewsHelper in this example.

avatar laoneo
laoneo - comment - 2 Apr 2022

@Ruud68 you are right. Reverted to the original helper file and made it none abstract and added a new getArticles function which is none static. What do you think?

On the other side also like this it still doesn't work with com_ajax, because the function needs to end with "Ajax" https://github.com/joomla/joomla-cms/blob/4.1-dev/components/com_ajax/ajax.php#L99.

avatar Ruud68
Ruud68 - comment - 2 Apr 2022

@Ruud68 you are right. Reverted to the original helper file and made it none abstract and added a new getArticles function which is none static. What do you think?

On the other side also like this it still doesn't work with com_ajax, because the function needs to end with "Ajax" https://github.com/joomla/joomla-cms/blob/4.1-dev/components/com_ajax/ajax.php#L99.

Cool, I think this will work. As for the Ajax in the function name that is not the issue. The issue was that the helperfile wasn't loaded. When that is loaded the method is called (ending in 'Ajax') so in the helper file you need to have that function otherwise you will get an error.

This is only relevant if you have a module that will service com_ajax requests because they have functionality that relies on that (which the Joomla core modules do not do)

What still 'bugs' me is that a module following this new approach will not work on J4.1 / 4.0.
For me as extension dev that complicates things as with every dependency on a Joomla version the 'market share' will drop :S
just saying :) should not hold back improvements.

avatar laoneo
laoneo - comment - 3 Apr 2022

If you leave the old module file from J3 and load the helper the same way in the dispatcher as the quickicon module, then you have compatibility down to J3.

fea9544 4 Apr 2022 avatar laoneo cs
avatar laoneo
laoneo - comment - 4 Apr 2022

@Ruud68 can you give this one here a test?

avatar Ruud68
Ruud68 - comment - 5 Apr 2022

@laoneo so just set up a new nightly 4.2 environment (april 5th), pulled this PR and copied the files over the new test environment.
when going to [domain]/index.php?option=com_ajax&module=articles_news&method=test&format=raw

I get the message: RuntimeException: The file at mod_articles_news/helper.php does not exist.

have not tried to troubleshoot this

avatar laoneo
laoneo - comment - 5 Apr 2022

Did you add the function testAjax to the helper?

avatar Ruud68
Ruud68 - comment - 5 Apr 2022

Hi, that is not necessary. com_ajax first tries to load the helper and when it does then it tries to load the method.
The error I got is for loading the helper > so it didn't get to the loading of the method (testAjax)

avatar laoneo
laoneo - comment - 5 Apr 2022

It does, but when the helper has no such function it tries the old way and when that doesn't succeed, it throws the old error message. You can see it here, it sets the result to null and doesn't throw an error message. If you want to add there an error, then a new pr is required.
Important for this pr is that you test the module on the ordinary way.

avatar Ruud68
Ruud68 - comment - 5 Apr 2022

sorry about that, you are completely right.
So the module works, the helper works and when adding testAjax com_ajax also works for modules in this way!

avatar Ruud68
Ruud68 - comment - 5 Apr 2022

I have tested this item successfully on b081abe


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

avatar Ruud68 Ruud68 - test_item - 5 Apr 2022 - Tested successfully
avatar SumCompanyInc
SumCompanyInc - comment - 26 Apr 2022

I have tested this item successfully on 3ee5c32


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

avatar SumCompanyInc SumCompanyInc - test_item - 26 Apr 2022 - Tested successfully
avatar Ruud68
Ruud68 - comment - 26 Apr 2022

I have tested this item successfully on 3ee5c32


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

avatar Ruud68 Ruud68 - test_item - 26 Apr 2022 - Tested successfully
avatar richard67 richard67 - change - 26 Apr 2022
Status Pending Ready to Commit
avatar richard67
richard67 - comment - 26 Apr 2022

RTC


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

avatar roland-d roland-d - change - 26 Apr 2022
Labels Added: ?
avatar roland-d roland-d - change - 26 Apr 2022
Status Ready to Commit Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2022-04-26 18:37:41
Closed_By roland-d
avatar roland-d roland-d - close - 26 Apr 2022
avatar roland-d roland-d - merge - 26 Apr 2022
avatar roland-d
roland-d - comment - 26 Apr 2022

Thanks everybody

Add a Comment

Login with GitHub to post a comment