Skip to content

[4.0] Remove onBeforeExecute event from CMS applications#27155

Merged
wilsonge merged 2 commits intojoomla:4.0-devfrom
SharkyKZ:j4/revert/onBeforeExecute
Nov 26, 2019
Merged

[4.0] Remove onBeforeExecute event from CMS applications#27155
wilsonge merged 2 commits intojoomla:4.0-devfrom
SharkyKZ:j4/revert/onBeforeExecute

Conversation

@SharkyKZ
Copy link
Contributor

Pull Request for Issues #17444, #26715, #26823.

Summary of Changes

This removes onBeforeExecute event added in #12124 from CMS applications.

Testing Instructions

a) Enable System - Privacy Consent plugin. Edit your profile.
b) Set up a multilingual site. Browse around the frontend.

Expected result

a) No untranslated strings.
b) Works like before.

Actual result

a) Untranslated strings like PLG_SYSTEM_PRIVACYCONSENT_LABEL.

Documentation Changes Required

IDK.

@brianteeman
Copy link
Contributor

This is a hard b/c break ??

@joeforjoomla
Copy link
Contributor

@brianteeman i agree... this is a hard b/c break given that it's no more possible to observe the onBeforeExecute event in a third-party extension during a CMS application execution. Are you sure to do this?

@SharkyKZ
Copy link
Contributor Author

This was introduced in 4.0.

@joeforjoomla
Copy link
Contributor

@SharkyKZ yes but for example i needed it in my extension, listening for onAfterInitialise won't work for my scenario... if you remove it there is no more chance to listen for an event before the doExecute method in the CMSApplication

@mbabker
Copy link
Contributor

mbabker commented Nov 26, 2019

It is either leave this hook in and break language loading in plugins or remove the hook. Considering the hook is new to 4.0 whereas the language loading behavior dates back to 1.5, to me it’s an easy choice to remove the hook until the application architecture can be built in a way that the global Language instance does not have to be fully configured before importing plugins.

It is a harder B/C break to tell system plugin developers they have to manual load language files because their plugins are imported at a point that they can’t rely on the framework to do it for them as they had for the previous decade.

Trust me, I want the hook, that’s why I added it in the first place. But it has created too many problematic side effects for it to sanely go into a stable release, and I’ve been saying that basically ever since the first bug reports came in about those side effects. It should have been reverted much sooner.

@joeforjoomla
Copy link
Contributor

@mbabker i trust you... go on

@wilsonge wilsonge merged commit 6c4e6e8 into joomla:4.0-dev Nov 26, 2019
@wilsonge
Copy link
Contributor

Thanks

@wilsonge wilsonge added this to the Joomla 4.0 milestone Nov 26, 2019
@mbabker
Copy link
Contributor

mbabker commented Nov 26, 2019

And for anyone reviewing this after the merge, the remaining onBeforeExecute hooks are "safe" in the context of the existing APIs for various reasons:

  • Joomla\CMS\Application\CliApplication is deprecated and shouldn't be used by the CMS itself by the time 4.0 ships (still gotta move the CLI Smart Search over to the console application), in addition to the class not importing CMS plugin groups, developers writing CLI applications would be able to cope with the language issue if they needed to (though in CLI context a lot of times languages aren't necessary)
  • Joomla\CMS\Application\DaemonApplication extends the deprecated Joomla\CMS\Application\CliApplication (note the daemon really should be deprecated too), same logic applies here
  • Joomla\CMS\Application\WebApplication::execute() is overridden by Joomla\CMS\Application\CMSApplication, developers may choose though to build their own web application classes and the same logic as the CLI applies (basically, if you're building your own application classes expect to have to deal with some architecture quirks on your own)

There IS one "before" event that is safe and expected to be used in the CMS environment, that being the Joomla\Application\ApplicationEvents::BEFORE_EXECUTE event inside the console application. This one is essential to being able to allow plugins to add command classes to the application (either through the command lazy loader or the application itself) and is the only hook available before the application tries to resolve the command. A Joomla\CMS\Language\Language instance isn't being explicitly set up in the console application, so if you have commands that rely on the language system then you're going to have to set that up (maybe core can offer a helper somewhere to do this?). Plugins in the "console" and "system" groups are imported (though it's really encouraged to use the new console group for this as it will only ever load in the CLI context, but since there are a large number of vendors that use system plugins for a lot of their functionality the system group is imported as well).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants