User tests: Successful: Unsuccessful:
Pull Request for Issue # .
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:
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.
Status | New | ⇒ | Pending |
Category | ⇒ | Modules Front End |
Labels |
Added:
?
|
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.
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 ?
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.
@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.
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.
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.
@HLeithner Isn't this PR removes static calls Factory::getApplication()
and Factory::getUser()
?
@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.
Labels |
Added:
?
Removed: ? |
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2022-05-30 15:29:48 |
Closed_By | ⇒ | joomdonation | |
Labels |
Added:
?
Removed: ? |
We will convert our modules to new structure, so the change propose in this PR is not needed anymore. Closing.
@wilsonge Your opinion?