[#39752] - [5.0] CMSApplication: Switching to triggerEvent
- Closed
- 22 Jul 2023
- Medium
- Build: 4.2-dev
- # 39752
- Diff
- Hackwar:4.2-application-event
User tests: Successful: Unsuccessful:
Summary of Changes
This syncs the event triggers in CMSApplication::execute() to use triggerEvent() overall. I know that triggerEvent() is deprecated and that in the end we want to switch to event classes instead. However right now this is not implemented everywhere and the task for someone else. But: The way the event is triggered right now throws a deprecation warning and we are using triggerEvent() in the rest of the method, so I'd rather keep it consistent for now.
Testing Instructions
Basically a codreview... Make sure that the events are still triggered, I guess?
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
joomla-cms-bot
- | Category | ⇒ | Libraries |
| Status | New | ⇒ | Pending |
| Labels |
Added:
?
?
|
||
| Title |
|
||||||
| Labels |
Added:
PR-4.3-dev
Removed: ? |
||
I tested the changes with different kind of listeners:
public function onBeforeRespond()
{
}public function onBeforeRespond(\Joomla\Event\Event $event)
{
}public function onBeforeRespond(\Joomla\Application\Event\ApplicationEvent $event)
{
}The first two are for the old way to trigger event (using triggerEvent method), the last one is for new way of event trigger (using dispatchEvent method). All works good, so this change look fine to me. Could anyone else take a look at this PR, too ? Maybe @Fedik ?
Another question is if this change is OK, should we do the same for SiteApplication and AdministratorApplication ?
Since this change will most likely not go into 4.3, I've rebased it to 4.4.
| Title |
|
||||||
That looks and works good to me. What about Site, Admin and other applications?
Please rebase to 5.0, safer to use the new way in the next major. Thanks for understanding.
| Title |
|
||||||
| Labels |
Added:
bug
Removed: PR-4.3-dev |
||
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-04-18 12:06:36 |
| Closed_By | ⇒ | Hackwar | |
| Labels |
Added:
PR-5.0-dev
|
||
| Status | Closed | ⇒ | New |
| Closed_Date | 2023-04-18 12:06:36 | ⇒ | |
| Closed_By | Hackwar | ⇒ |
| Status | New | ⇒ | Pending |
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2023-07-22 08:38:27 |
| Closed_By | ⇒ | Fedik |
Since I don't see this being merged into 4.2-dev, I've changed the base branch to 4.3-dev. Lets hope it will be merged in there.