? Pending

User tests: Successful: Unsuccessful:

avatar SharkyKZ
SharkyKZ
26 Nov 2019

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.

avatar SharkyKZ SharkyKZ - open - 26 Nov 2019
avatar SharkyKZ SharkyKZ - change - 26 Nov 2019
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 26 Nov 2019
Category Libraries Front End Plugins
avatar SharkyKZ SharkyKZ - change - 26 Nov 2019
Labels Added: ?
avatar brianteeman
brianteeman - comment - 26 Nov 2019

This is a hard b/c break ??

avatar joeforjoomla
joeforjoomla - comment - 26 Nov 2019

@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?

avatar SharkyKZ
SharkyKZ - comment - 26 Nov 2019

This was introduced in 4.0.

avatar joeforjoomla
joeforjoomla - comment - 26 Nov 2019

@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

avatar mbabker
mbabker - comment - 26 Nov 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.

avatar joeforjoomla
joeforjoomla - comment - 26 Nov 2019

@mbabker i trust you... go on

avatar wilsonge wilsonge - change - 26 Nov 2019
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2019-11-26 14:48:23
Closed_By wilsonge
avatar wilsonge wilsonge - close - 26 Nov 2019
avatar wilsonge wilsonge - merge - 26 Nov 2019
avatar wilsonge
wilsonge - comment - 26 Nov 2019

Thanks

avatar mbabker
mbabker - comment - 26 Nov 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).

Add a Comment

Login with GitHub to post a comment