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

[5.1] Improve onRouterInitialise event #43015

Closed
wants to merge 11 commits into from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Mar 12, 2024

Pull Request for Issue #42991, alternative to #43009.

Summary of Changes

Instead of calling the event directly after the class instantiation, we better call it explicitly, before it is used.
I renamed the event.

Also I have add a message to warn the developers not to use Route before initialisation. Hovewer, in future, we need more reliable way to detect when aplication is initialised than use of $app->getLanguage() 😄

@Hackwar please check

Testing Instructions

Please follow #42991
And re test:

Actual result BEFORE applying this Pull Request

#42991 produce an error

Expected result AFTER applying this Pull Request

All works,
Older PRs works as before.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org:
  • No documentation changes for manual.joomla.org needed

@joeforjoomla
Copy link
Contributor

joeforjoomla commented Mar 12, 2024

@Fedik do not rename the event now! I've already updated and released my extensions to users based on the onAfterInitialiseRouter event name.
We are at Beta 1!

@Fedik
Copy link
Member Author

Fedik commented Mar 12, 2024

@joeforjoomla To late! I have renamed it already!

Please learn how to be more polite, thanks in advance.

upd. I will restore old name

@joeforjoomla
Copy link
Contributor

@Fedik thank you 🙏. Current code is both using the AfterInitialiseRouterEvent event class and onAfterInitialiseRouter event name.

@Hackwar
Copy link
Member

Hackwar commented Mar 13, 2024

@Fedik I don't see how this solves the basic issue we have. This helps a bit with people who create the siterouter in the constructor, but if they also create a link or parse a link at that time, we are at the same place we've been before. I also think this is rather a lot of code to achieve this... When I introduced the new event, I thought it would be better to not load the siterouter every time, especially when we don't even need it, but looking at the alternative, I'd rather go back to the old solution and live with that...

@joeforjoomla I understand that you already released code which uses this, but we do have an issue here and we will have to fix it, which also could mean reverting the whole event for this again.

@Fedik
Copy link
Member Author

Fedik commented Mar 13, 2024

but if they also create a link or parse a link at that time, we are at the same place we've been before.

Hm , they should use Route::link() :)

Maybe can make a rule in the SiteRouter constructor? kind of

$this->attachParseRule([$this, 'initialiseCustomRules'], self::PROCESS_BEFORE);

Or yea, just revert that.

@SniperSister
Copy link
Contributor

I have tested this item ✅ successfully on 6187bd5

Test result: Works as described.

BUT: There is a downside compared to the approach taken in #43009.

Core question is: When do we add additional router rules? We have 3 different approaches for that question.

  • Current dev branch: Once the SiteRouter is built in the service Provider - this causes problems if the SiteRouter service is being fetched in the serviceprovider of a system plugin, as other plugins providing rules might have not been booted, so they won't add rules, even after they have been booted later on.
  • This PR: Once the first route is parsed. This would cause the same issue as described above if a system plugin actually parses a route in it's serviceprovider.
  • [5.1] Revert implementation of AfterInitialiseRouterEvent #43009: During "onAfterInitialise" - that binds the rules one after another. Downside of this approach (which is the current behavior in 5.0 and earlier) is that you might get different results for the "parse" call before, after and even during "onAfterInitialise" is been processed. However, at least all rules will indeed be appended.

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

@Fedik
Copy link
Member Author

Fedik commented Mar 13, 2024

hmhm, yeah, it is still not very reliable.
It will be more safe to revert it.

I close this pr, maybe in future we will come up with some better solution.

@Fedik Fedik closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants