? Pending

User tests: Successful: Unsuccessful:

avatar joomdonation
joomdonation
25 Jul 2021

Pull Request for Issue # .

Summary of Changes

This PR modify some of our frontend modules code to pass application option (which is available via $app variable) to module helper methods. This helps:

  • Using injected $app object instead of having to call Factory::getApplication() to get application object (isn't it the purpose of our new module dispatcher architecture?) ?
  • Allow replacing deprecated Factory::getUser() method with $app->getIdentity();

As these helper methods change in this PR is public method, I'm unsure if changing parameters here are consider backward incompatible changes. If it is, please let me know and I will close the PR. Just want to improve the code a bit.

Testing Instructions

  • Code review
  • Or publish all the modules changed in this PR and make sure it is still working as expected.
avatar joomdonation joomdonation - open - 25 Jul 2021
avatar joomdonation joomdonation - change - 25 Jul 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 25 Jul 2021
Category Modules Front End
avatar joomdonation joomdonation - change - 25 Jul 2021
Labels Added: ?
avatar richard67
richard67 - comment - 25 Jul 2021

As these helper methods change in this PR is public method, I'm unsure if changing parameters here are consider backward incompatible changes. If it is, please let me know and I will close the PR. Just want to improve the code a bit.

@wilsonge Your opinion?

avatar wilsonge
wilsonge - comment - 25 Jul 2021

@wilsonge Your opinion?

This should be fine. Extensions officially anyhow don't fall under the b/c pledge and tbh no one should be using the modules helpers in their own extensions.

avatar laoneo
laoneo - comment - 27 Jul 2021

I like the change. What about making the app an optional parameter and when not set it is fetched from Factory? Pretty sure there are extensions which are using the helpers, especially the ones from content. We can add a warning when the app is not set that in J5 it will be a required parameter.

avatar joomdonation
joomdonation - comment - 27 Jul 2021

What about making the app an optional parameter and when not set it is fetched from Factory?

I'm unsure about this one. The data returned from the method usually depend on parameters from the module itself, so I'm unsure if third party developers should use module helpers, they should use model instead. @wilsonge How do you think about this ?

avatar laoneo
laoneo - comment - 27 Jul 2021

so I'm unsure if third party developers should use module helpers

Even when they shouldn't use them, pretty sure they do. There is no reason to break here. We have been very carefully to make as less BC breaks as possible for J3 extensions to run on J4. Honestly I do not understand why now in RC phase we are leaving this path.

avatar joomdonation
joomdonation - comment - 27 Jul 2021

@laoneo I'm just asking for decision from @wilsonge as he stated earlier

This should be fine. Extensions officially anyhow don't fall under the b/c pledge and tbh no one should be using the modules helpers in their own extensions.

The reason is because if it is allowed, I would like to make our modules consistent. Right now, we code it in different way in different modules. I asked the question is yesterday #34907

If we decide to keep it BC, I will make the change, no problem. Just wait for final decision.

avatar laoneo
laoneo - comment - 31 Jul 2021

It would be nice to have them all working the same way but please no BC breaks even when it doesn't go under our BC promise. It should be possible to use them in J3 and J4 the same thats what I'm asking for.

avatar HLeithner
HLeithner - comment - 1 Aug 2021

I would like to get rid of everything that's called static. No Idea if this is possible in joomla but the DI introduced is the first step. So this PR doesn't make sense at this time.

Maybe we can talk about this concept.

avatar joomdonation
joomdonation - comment - 1 Aug 2021

@HLeithner Isn't this PR removes static calls Factory::getApplication() and Factory::getUser() ?

avatar HLeithner
HLeithner - comment - 1 Aug 2021

@joomdonation true but you change a static class and we should get rid of static classes at least that's my opinion.

So just changing this function call is a bit unnecessary. if we are able to remove all static classes and ask the booted component for the helper which returns the "helper" or anything other object that can do the same which already knows everything about the the current joomla instance.

avatar chmst chmst - change - 31 Jan 2022
Labels Added: ?
Removed: ?
avatar joomdonation joomdonation - change - 30 May 2022
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2022-05-30 15:29:48
Closed_By joomdonation
Labels Added: ?
Removed: ?
avatar joomdonation
joomdonation - comment - 30 May 2022

We will convert our modules to new structure, so the change propose in this PR is not needed anymore. Closing.

avatar joomdonation joomdonation - close - 30 May 2022

Add a Comment

Login with GitHub to post a comment