Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[4.0] System Plugins loose and / or reset loaded language files #17444

Closed
matrikular opened this issue Aug 8, 2017 · 20 comments
Closed

[4.0] System Plugins loose and / or reset loaded language files #17444

matrikular opened this issue Aug 8, 2017 · 20 comments

Comments

@matrikular
Copy link
Contributor

matrikular commented Aug 8, 2017

Steps to reproduce the issue

  • Install and enable the plg_system_foobar_4x.zip test / demo plugin in a fresh Joomla! 4.x-dev installation (no multisite, en-GB).
  • You should immediately see the var_dump(); message for the onContentPrepareForm event, including the untranslated string: PLG_CONTENT_FOOBAR_TEST.

Expected result

The string: PLG_CONTENT_FOOBAR_TEST should be translated to "Test".

Actual result

The string: PLG_CONTENT_FOOBAR_TEST is not translated to "Test".

System information (as much as possible)

Windows NT DESKTOP-G94B9J7 10.0 build 15063 (Windows 10) i586
Database Version 5.5.5-10.1.25-MariaDB
Database Collation utf8mb4_unicode_ci
Database Connection Collation utf8_general_ci
PHP Version 7.1.7
Web Server Apache/2.4.26 (Win32) OpenSSL/1.0.2l PHP/7.1.7
WebServer to PHP Interface apache2handler
Joomla! Version Joomla! 4.0.0-dev Development [ Amani ] 31-March-2017 23:59 GMT
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Additional comments

I was trying to solve this on my own but couldn't get my head around it, yet. To keep the number of loaded language files to a minimum, I've tested / debugged the code starting from within a empty new article (Title: Test, no content, or category). From there, I have then logged all calls to the Language::parse() method and included the callstack every time the internal $path variable was reset.

Since the same plugin / event, though in a different group (content), works as expected, the problem seems to be only accouring* for system plugins and only in J!4.x-dev.

*Looking at the callstacks though, it seems that the test string not being translated is just a symptom of another problem. The reset / re-initialisation of the paths and the "second" call to parse "all" language files seems a bit odd / not intended to me.

You can find the callstack logs, including all plugins I tested (system / content / J!4 / J!3) here:

@ghost
Copy link

ghost commented Nov 9, 2017

@mbabker should this be applied to a Project?

@mbabker
Copy link
Contributor

mbabker commented Apr 29, 2018

#12124 needs to be reverted.

To be able to load system plugins that have autoloaded languages, we have to have Factory::$language seeded. To seed Factory::$language correctly, we have to do most of the steps in CMSApplication::initialiseApp() (which is basically extracting the language to use based in part on routing data and in part on the language filter plugin configuration). There is absolutely zero way to decouple the requirement that the site application's $language_filter attribute is set before the Language instance is loaded, meaning the CMS will conceptually always have a hardcoded dependency to the language filter plugin.

//cc @laoneo @wilsonge

@brianteeman
Copy link
Contributor

And this is a great example of what we were saying about not merging something without tests just because it is for j4. Mistakes happen and time is wasted

@mbabker
Copy link
Contributor

mbabker commented Apr 29, 2018

This has nothing to do with the premature merging issue. I didn't catch the issue 18 months ago when I wrote the PR, or 8 months ago when this issue was logged. I guarantee you nobody else would've tested adequately enough to say that the PR can't work because the core architecture (specifically the implementation of the language filter capability) is so far beyond flawed that it's unsaveable.

@wilsonge
Copy link
Contributor

OK. So rather than just raw reverting - alternative proposal. If we're going to allow realistic overriding of some container properties we need to introduce a new plugin type that's booting as early as physically possible in the app.

What if we moved language filter plugin into that new group - obviously then we would stop loading system plugins this early which would fix this bug. Language filter itself is just registering it's properties into other parts of the system.

For example maybe we can move it to the behaviour group we've added in 4. And move the loading of behaviour plugins (currently https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/Application/CMSApplication.php#L771) higher up the stack - obviously it would need to be tested super hard to make sure we don't break tags + content history... But given it's a new group we still have the freedom to move that around without b/c breaks.

@mbabker
Copy link
Contributor

mbabker commented Apr 29, 2018

Still won't work, you'll lose loaded languages.

Without Factory::$language seeded (as is the case now)...

  • Call PluginHelper::importPlugin('any-group');
  • Import process goes into PluginHelper::import() which is responsible for loading the plugin file and instantiating the plugin class
  • Have a plugin in whatever group is being loaded have language files autoloaded, this will trigger a call to Factory::getLanguage() which will create a Language instance based on config (probably en-GB)
  • Step into CMSApplication::initialiseApp() which is then processing the right language to load (on backend this uses language request param, on frontend this requires language filter config), on a multi-language website where the user is making a request in a language other than default this will result in another Language instance being instantiated without the language files of the first instance loaded and Factory::$language replaced (should be fine on single language sites since Language::getInstance() is singleton)

Literally, to load plugins we have to have Factory::$language seeded or the global config match the request language 100% of the time.

@wilsonge
Copy link
Contributor

I understand that any plugins in that plugin group wouldn't use the correct languages strings and therefore we'd need to heavily document that fact in several places (but autoloadLanguage property is off by default so someone would have to build a new plugin and explicitly set that to true and even then it's just going to affect that plugin right because once we get to initialiseApp the global language object is going to basically be overwritten). But none of the existing behaviour group plugins nor system filter are using language strings directly right now and this is a new group so again as long as it's documented insanely hard that we're loading this super early and thus you can't use strings is this a problem?

@mbabker
Copy link
Contributor

mbabker commented Apr 29, 2018

The same services would be loaded whether you trigger a plugin event first thing in the app's execution cycle or wait to do several steps (well, there are actually a couple of the extension helper classes you can overload in theory with the before execute event versus after initialise). Personally I don't think it's a good practice to say "this plugin group is restricted from using X capability", which is why architecturally the correct option is to revert unless you can come up with a way to load plugins that doesn't require Factory::$language to be initialized first.

@laoneo
Copy link
Member

laoneo commented Apr 29, 2018

Sounds for me that the language filter plugin needs to be loaded as very first plugin all the time. I even got the impression that this shouldn't be a plugin because it is a required dependency when you want to have a multilanguage site.

@mbabker
Copy link
Contributor

mbabker commented Apr 29, 2018

Plugins are supposed to introduce addon capabilities and should not be hardcoded dependencies anywhere else in the application stack. Hence my spam of issues yesterday where this pattern has been introduced into core with various plugins. And hence trying to introduce an even earlier event into the application process so that the language filter plugin can set the right attributes from its params without having the SiteApplication class hardcoded to the language filter plugin.

You're right, it really shouldn't be a plugin. But it doesn't fit into the system anywhere else.

@infograf768
Copy link
Member

infograf768 commented Apr 30, 2018

Let's not forget that a Monolanguage site may have multiple languages installed AND used as preferred UI language by connected users.
What I mean is that the language filter plugin is not always at stake concerning language loading as it is used only when the site itself is Multilingual.

@laoneo
Copy link
Member

laoneo commented Apr 30, 2018

For me, most of the functionality of the language filter plugin sounds more like that it belongs into to corner of a router middle ware.

@brianteeman
Copy link
Contributor

It looks like this has been resolved elsewhere as I correctly get

string(20) "onContentPrepareForm" string(4) "Test"

@brianteeman
Copy link
Contributor

Closing for now - it can always be reopened

@ReLater
Copy link
Contributor

ReLater commented Jan 10, 2019

@brianteeman

it can always be reopened

Yes, please reopen.

I tested the test plugin. Not translated in J4 (single language, just blog sample data), current nightly, "as-is".

I have a custom plugin with the same base structure and issue in J4 but not J3. I have to force the lang load by something like
Factory::getLanguage()->load('plg_system_something', JPATH_PLUGINS . '/system/something'); in later code where I need it.

@joomla-cms-bot
Copy link

Set to "open" on behalf of @Quy by The JTracker Application at issues.joomla.org/joomla-cms/17444

@SharkyKZ
Copy link
Contributor

What about loading old language instance files into the new instance like Language Filter plugin does?

$language_new = Language::getInstance($lang_code);
foreach ($language->getPaths() as $extension => $files)
{
if (strpos($extension, 'plg_system') !== false)
{
$extension_name = substr($extension, 11);
$language_new->load($extension, JPATH_ADMINISTRATOR)
|| $language_new->load($extension, JPATH_PLUGINS . '/system/' . $extension_name);
continue;
}
$language_new->load($extension);
}

Too hacky?

@mbabker
Copy link
Contributor

mbabker commented Oct 23, 2019

That is a complete hack to work around the problem of having to have the global singleton Language instance created and configured before you can reliably import plugins. Which is an architectural flaw.

@joomla-cms-bot
Copy link

Set to "closed" on behalf of @SharkyKZ by The JTracker Application at issues.joomla.org/joomla-cms/17444

[4.0] PHP automation moved this from To do to Done Nov 26, 2019
@SharkyKZ
Copy link
Contributor

Please test PR #27155.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17444.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
[4.0] PHP
  
Done
Development

No branches or pull requests

9 participants