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 ajax fix #37398

Closed
wants to merge 3 commits into from
Closed

Com ajax fix #37398

wants to merge 3 commits into from

Conversation

Ruud68
Copy link
Contributor

@Ruud68 Ruud68 commented Mar 29, 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

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 29, 2022

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

@laoneo
Copy link
Member

laoneo commented Mar 29, 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.

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 29, 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

@laoneo
Copy link
Member

laoneo commented Mar 29, 2022

Does the pr #32633 help?

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 29, 2022

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

@laoneo
Copy link
Member

laoneo commented Mar 29, 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.

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 29, 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

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 29, 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...

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 31, 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.

@wilsonge
Copy link
Contributor

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?

@bembelimen
Copy link
Contributor

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.

@chmst
Copy link
Contributor

chmst commented Mar 31, 2022

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

@chmst chmst closed this Mar 31, 2022
@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 31, 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.

@Ruud68 Ruud68 deleted the com_ajax_fix branch March 31, 2022 13:54
@laoneo
Copy link
Member

laoneo commented Mar 31, 2022

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

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 31, 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.

@laoneo
Copy link
Member

laoneo commented Mar 31, 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.

@Ruud68
Copy link
Contributor Author

Ruud68 commented Mar 31, 2022

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

@laoneo
Copy link
Member

laoneo commented Apr 1, 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.

@Ruud68
Copy link
Contributor Author

Ruud68 commented Apr 2, 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

@laoneo
Copy link
Member

laoneo commented Apr 2, 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.

@joomla joomla deleted a comment Apr 6, 2022
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.

6 participants