? Pending

User tests: Successful: Unsuccessful:

avatar Ruud68
Ruud68
29 Mar 2022

Pull Request for Issue #37012 .

Summary of Changes

com_ajax tries to only load the module helper from the J3 module helper location

Testing Instructions

in browser goto: [yourdomain]/index.php?option=com_ajax&module=articles_news&method=test&format=raw

Actual result BEFORE applying this Pull Request

RuntimeException: The file at mod_articles_news/helper.php does not exist.

Expected result AFTER applying this Pull Request

LogicException: Method testAjax does not exist.

Documentation Changes Required

avatar Ruud68 Ruud68 - open - 29 Mar 2022
avatar Ruud68 Ruud68 - change - 29 Mar 2022
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 29 Mar 2022
Category Front End com_ajax
avatar Ruud68
Ruud68 - comment - 29 Mar 2022

@arenamax @wedal are you able to test this PR?

avatar laoneo
laoneo - comment - 29 Mar 2022

When you are on Joomla 4, then better to let the module implement Joomla\CMS\Helper\HelperFactoryInterface. We want to avoid manual class name compiling. These ways are basically deprecated.

avatar Ruud68
Ruud68 - comment - 29 Mar 2022

When you are on Joomla 4, then better to let the module implement Joomla\CMS\Helper\HelperFactoryInterface. We want to avoid manual class name compiling. These ways are basically deprecated.

can you point me to this in mod_articles_news as i use that as reference

avatar laoneo
laoneo - comment - 29 Mar 2022

Does the pr #32633 help?

avatar Ruud68
Ruud68 - comment - 29 Mar 2022

@laoneo no it doesn't: for e.g. mod_articles_news in what file do I add the implements Joomla\CMS\Helper\HelperFactoryInterface

avatar laoneo
laoneo - comment - 29 Mar 2022

You need to do the same as the quick icon module, convert it first to a service provider and then do the helper stuff.

avatar Ruud68
Ruud68 - comment - 29 Mar 2022

okay... so that would basically mean that I have to redo all modules that use ajax. #snif (and probably someone has to refactor all Joomla core modules?)

Although 'deprecated' this PR improves at least the deprecated+++ thing where it is trying to load the helper file from the J3 location

avatar Ruud68
Ruud68 - comment - 29 Mar 2022

and as a bonus: the module helper file was / is always an abstract class. so not only do you need to refactor the module into a service provider but also refactor the module helper file into not being abstract (as an abstract class cannot be instantiated). and by doing that potentially refactor / rethink everything that uses the helper class....
So now I start to understand why the module/helper.php 'workaround' is in there: it is way easier to not use the src/Helper/helper.php file but keep it in the J3 module helper location...

avatar Ruud68
Ruud68 - comment - 31 Mar 2022

okay, so thanks @laoneo : refactoring the module into a service provider works.

but as this is not an option for some devs (probably adding to the 'lets wait until J5 is out before refactoring again') this PR would definitely add value for those developers who do not want their J4 module be refactored into a J3 file structure.

So if we get two testers (@arenamax @wedal looking at you :)), then one of the maintainers responsible for this code can decide to either accept or not accept. At least then we have some clarity on the effort to report the issue and the effort to propose a PR for it.

avatar wilsonge
wilsonge - comment - 31 Mar 2022

Ok so what's the gap we're trying to fill here? I'm missing something for sure I think.

If your J3 compatible your going to use the J3 file structure

If your J4 only your going to use namespaces and services.

I'm not seeing what scenario your solving by namespacing but not using services?

avatar bembelimen
bembelimen - comment - 31 Mar 2022

but as this is not an option for some devs (probably adding to the 'lets wait until J5 is out before refactoring again') this PR would definitely add value for those developers who do not want their J4 module be refactored into a J3 file structure.

That's one of the worst choices a developer can have, nobody should wait for J5, as now the gap is the smallest possible (and tbh if you programmed in a clean way in the past it's not that much of an effort to get a fully compatible J4 component...beside some renaming you mostly are deleting stuff, if you didn't programmed in a clean way, you shouldn't wait either but start now cleanup and refactor).

then one of the maintainers responsible for this code can decide to either accept or not accept.

I'm not in charge of 4.2 but for me this is a big "No" in terms of merging.

avatar chmst chmst - change - 31 Mar 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-03-31 12:42:12
Closed_By chmst
Labels Added: ?
avatar chmst
chmst - comment - 31 Mar 2022

After reading the whole discussion and comments of maintainers I close this PR. Thanks all for your contributions and explanations!

avatar chmst chmst - close - 31 Mar 2022
avatar Ruud68
Ruud68 - comment - 31 Mar 2022

Hi, thanks for the feedback and decision. please do not forget to re-open the issue again that was closed by this pr.
and as an advise: refactor the core joomla modules to the new namespacing / services as well as now only the administrator module quick-icons is correct. That is what started this issue in the beginning for me (using a core joomla module as boilerplate) and probably as well as for the issue starter.

avatar laoneo
laoneo - comment - 31 Mar 2022

@Ruud68 you are welcome to help to convert the core modules to services.

avatar Ruud68
Ruud68 - comment - 31 Mar 2022

Thanks @laoneo for following up! Maybe the maintainers could make a 'todo' of these kind of changes. Then devs could do PR's for them in a controlled way: the dev then has the 'guarantee' that time invested is appreciated / used and not wasted / ignored.

avatar laoneo
laoneo - comment - 31 Mar 2022

Your pr started some important discussion in the Bug Squad chat. I will do tomorrow a pr where I convert a module for 4.2. It should act then as example for others. I would really appreciate when the outcome of this pr is a start of a process where we convert existing modules together.

avatar Ruud68
Ruud68 - comment - 31 Mar 2022

and thanks again @laoneo this is so important.... #communication :)

avatar laoneo
laoneo - comment - 1 Apr 2022

Can you have a look on #37445. That's how it should be. Ignore the part with the helper factory aware interfaces, jut focus on the module part.

avatar Ruud68
Ruud68 - comment - 2 Apr 2022

Good morning @laoneo , will have a look over the weekend. First need to get a good understanding of how this works as there are more changes (like the dispatcher and dropping the mod_modulename.php then I currently have working on my new.module. I'm am assuming there is no documentation so need to dive into the code for this. Will follow up as soon as possible.
As for the actual work involved: maybe create an issue per module that needs refactoring so we / the devs can 'claim' that. Or use GitHub projects for that? Not sure what is handy ro have an overview and avail double work

avatar laoneo
laoneo - comment - 2 Apr 2022

That's a good idea, as cards do not allow to assign them, I'v opened a simple issue #37455 where devs can post a comment with the module they are converting.

Add a Comment

Login with GitHub to post a comment