Refactor the page cache plugin #36042
Conversation
|
@HLeithner I forgot to mention this. Over the course of last week @laoneo found a way to make the plugins installable without any change in core code! He moved the |
perfect one problem less ;-) |
This solves the issue when multiple plugins are being executed on these 2 events. If a plugin really needs to be executed after the page cache plugin it can still use \Joomla\Event\Priority::MIN for its own event handler.
Actually you can do this already since 3.4 f90ff60#diff-f1347eb6172ab8188fa384c60a35be15af08dc341072aad5e842e912679234de |
|
@HLeithner any comments for triggerEvent usage here? Otherwise I give it a test and get it merged. Afterwards we can discus if we want to migrate more plugins as suggested by @nikosdion. |
|
@laoneo @HLeithner IMHO the triggerEvent helper method shouldn't disappear forever. I have written a number of plugins which do not use it and my takeaway from this is that the alternative is clunky. What I'd like to do is create a new trait where event creation and parsing the event results are separate methods. Think of calling something like Otherwise every time we call an event we need to use the same boilerplate code: $result = $this->getDispatcher()->dispatch($eventName, $new Event($eventName, $argumentsArray));
$actualResult = !isset($result['result']) || \is_null($result['result']) ? [] : $result['result'];I would argue that the boilerplate code looks clunky and inscrutable to newcomers. We don't want to dissuade new developers so a couple method calls may be better than lab clean code. What do you think? |
This is true for J3 events for BC reasons. But for new ones you can use the argument from the event as result as we did in #35396. In this pr here you can just replace |
|
Since the events are not new I wouldn't require a proper event object. But we should get rid of all events without it own class as already discussed. It would be a great time/pr to do this as part of the refactoring. As allon already said for new event we can guarantee that $result['result'] exists (and of course should). If you don't like to do it now or in a future pr, I'm fine with it but as it's also an example PR it would be great if can set a milestone how to do it for events so a refer to this single pr is easier then having a couple of PRs to refer to. |
|
@HLeithner Just to be clear: you'd like me to create Event classes for all the events referenced in this plugin and use them without going through triggerEvent. Say yes and I'll start working on it tonight. |
|
@nikosdion yes when I'm not wrong we discussed this in another Issue that we want to move all legacy events to the "new" event system. And iirc this is possible in a fully b/c way right? |
|
I thought it was a 5.0 goal 😛 You are right that it can be done b/c as long as the event handlers parse the event arguments positionally instead of by name i.e. Throughout the 4.x release cycle this is how event handlers should be written. Moreover, throughout the 4.x release we need to VERY CLEARLY document these specific event classes, which version they appeared and instruct developers to use them. When we start working on 5.0 we can type hint with the correct event subclass and use its named arguments. PS: About the “clearly document” we can also create a trait which generates the correct event class based on the event name. For example, if Joomla has a core event onFoobarScream which should be conveyed through an event object of the class Should I give it a stab? Now that I've worked with events on real world stuff, not just the lab environment I was using back in 2015, I have a much better ‘feel’ for how to do things in a way that doesn't screw the core over in the long term and doesn't screw third party developers over in the short to medium term. |
|
I would like to remove the old event system in j5.0 already and to have all Events have there correct classes already in 4.x ready and used. Maybe the validation have to be more relaxed with deprecation warnings if wrong values are provided. About the trait I can't follow you for what it's needed maybe you can outline it a bit. The plan to start on j5 is with release of 4.1, at this point we will branch out 4.2 and 5.0, we also have a project to migrate to PSR-12 withing 4.1 which is maybe too late... so it's possible that we do this in 4.1.x or 4.2. to reduce the impact on upmerge to 5.0 but I hadn't the time since the j3.10 phpunit and 8.0 comes into my way... |
|
I was busy doing releases today. Tomorrow I'll write a POC as a console plugin so you can see what the Trait would look like and how we can tackle b/c. It's about 80% there in my head, I need to put it into code and get it to the next 20% (to make sure I don't mess up b/c with J3 plugins running in Joomla 4). Then we can get back on the discussion about this PR and how much we want to do now. But as a general idea, yeah, I'd agree that if we convert all of the plugin events at least two minor releases before 5.0 we can definitely remove legacy event handling from 5.0. Why two minor releases? Because this would allow for smooth updates based on my experience with the 3.x to 4.0 migration 😉 In August 2021 I had to simultaneously support 3.9, 3.10 and 4.0. This allowed clients using my software on Joomla 3.9 to follow a dead simple upgrade procedure: update extensions, backup, upgrade to Joomla 3.10, backup, upgrade to 4.0, done. My limiting factor was what the penultimate version of 3.x (i.e. 3.9) supported. In those versions I couldn't use any code that only appears in 3.10 and 4.0. So, if we're touching something as fundamental as events handling we should most definitely do it with this kind conservative upgrade process in mind. Regarding PSR-12 I am NOT a big fan on two accounts:
I think Joomla should have its own code style, like it does now, because the objective of code is to a. perform a certain function; b. make it easily readable; c. minimise busywork and d. minimise the possibility of stupid bugs. PSR-12 fails miserably in the latter three. At some point CS people need to understand the historical context of K&R before trying to apply it indiscriminately on all modern code. |
Sounds great thanks
I know the pain...
This topic is heavily opinion driven but to be honest we don't have the resources to maintain our code style and it really doesn't make sense to do every thing different then the rest of the php eco system. We can discuss this to at a later time in another topic. Just to your mentioned things.
Anyway that topic can be discussed later and if we need exceptions or stick to psr-12 completely. |
|
@nikosdion could you please fix conflicts here? |
This solves the issue when multiple plugins are being executed on these 2 events. If a plugin really needs to be executed after the page cache plugin it can still use \Joomla\Event\Priority::MIN for its own event handler.
|
@nikosdion can you fix the conflicts here? Thanks. |
# Conflicts: # plugins/system/cache/cache.php
|
@laoneo There you go. I also fixed the docblock and inject the frontend router object; these should be the last two pending items. |
|
The intendation is still not correct as the other docblocks do look like: |
|
The funny thing is that the weird indentation comes from me copying that weird docblock from the original file. Whenever I have time (this week is really difficult) I will reformat the docblocks. Unless you want to do a PR to my branch if you have the time? |
|
Guess now it is correct. |
|
Thank you |
|
Thanks for your patience! Now we can start to convert the rest of the plugins. |
|
Thank you for merging! |
Pull Request to replace #35986
Ping @HLeithner @anibalsanchez @laoneo
Summary of Changes
Refactored the system/cache plugin into the new Joomla 4 style.
Discussion
Please test. I did not have the time to do any decent testing before going to bed; I need to wake up early tomorrow to get the kid to school. Sorry.
I am using the SubscriberInterface to register the three plugin events.
The two events which add the page to the cache are rigged to always run last, otherwise the plugin wouldn't work right.Each event checks whether the app is in a state that supports serving cached pages or storing pages into the cache:
The first two checks are cached on first use for a faster false return since these conditions don't change throughout the request. The latter two checks are performed each time the first two checks pass because a user may be logged in and application messages may be generated throughout the request lifetime.
I refactored the page exclusion checks to use a bit more modern PHP code.
The cache controller is not created until it's needed. This saves some initialization time when visiting pages which shouldn't be cached or served from the cache as this means the cache controller would never get to be initialized.
The external (possible SEF) URL to the page is not retrieved until it's needed instead of in the constructor to save a few microseconds there too.