Skip to content

[5.0] CMSApplication: Switching to triggerEvent#39752

Closed
Hackwar wants to merge 5 commits intojoomla:5.0-devfrom
Hackwar:4.2-application-event
Closed

[5.0] CMSApplication: Switching to triggerEvent#39752
Hackwar wants to merge 5 commits intojoomla:5.0-devfrom
Hackwar:4.2-application-event

Conversation

@Hackwar
Copy link
Member

@Hackwar Hackwar commented Jan 30, 2023

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

@Hackwar Hackwar added the Small A PR which only has a small change label Feb 25, 2023
@Hackwar Hackwar changed the base branch from 4.2-dev to 4.3-dev March 5, 2023 19:26
@Hackwar
Copy link
Member Author

Hackwar commented Mar 5, 2023

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.

@Quy Quy removed the PR-4.2-dev label Mar 6, 2023
@Hackwar Hackwar changed the title [4.2] CMSApplication: Switching to triggerEvent [4.3] CMSApplication: Switching to triggerEvent Mar 19, 2023
@joomdonation
Copy link
Contributor

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 ?

@Hackwar Hackwar changed the base branch from 4.3-dev to 4.4-dev April 3, 2023 09:45
@Hackwar
Copy link
Member Author

Hackwar commented Apr 3, 2023

Since this change will most likely not go into 4.3, I've rebased it to 4.4.

@Hackwar Hackwar changed the title [4.3] CMSApplication: Switching to triggerEvent [4.4] CMSApplication: Switching to triggerEvent Apr 3, 2023
@Fedik
Copy link
Member

Fedik commented Apr 3, 2023

That looks and works good to me. What about Site, Admin and other applications?

@laoneo
Copy link
Member

laoneo commented Apr 3, 2023

Please rebase to 5.0, safer to use the new way in the next major. Thanks for understanding.

@Quy Quy removed the PR-4.3-dev label Apr 4, 2023
@laoneo laoneo added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Apr 4, 2023
@Hackwar Hackwar added the bug label Apr 7, 2023
@laoneo laoneo changed the title [4.4] CMSApplication: Switching to triggerEvent [5.0] CMSApplication: Switching to triggerEvent Apr 11, 2023
@laoneo laoneo changed the base branch from 4.4-dev to 5.0-dev April 11, 2023 08:59
@laoneo laoneo removed PR-4.4-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested. labels Apr 11, 2023
@Fedik
Copy link
Member

Fedik commented Jul 22, 2023

This was fixed as part of #40522 Implementing application event classes.
I close this PR. Thanks for your work 😉

@Fedik Fedik closed this Jul 22, 2023
@Hackwar Hackwar deleted the 4.2-application-event branch March 22, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Small A PR which only has a small change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants