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

Alternative approach to #9999 to deal with hardcoded import of plugins #10344

Merged
merged 1 commit into from
Jul 16, 2016
Merged

Alternative approach to #9999 to deal with hardcoded import of plugins #10344

merged 1 commit into from
Jul 16, 2016

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented May 8, 2016

This is the implementation of the alternative approach to #9999 I suggested.

To be quite frank, I don't work with the multilingual system so I don't know if this will have unintended side effects, someone more familiar than me on that will need to guide on this.

In essence, instead of importing the languagefilter plugin (which seems to hardcode it as the first plugin to be executed because the event dispatcher doesn't have an internal handling for event priorities), the plugin data is only loaded from the database and the params extracted out to set the application vars.

This defers importing of the plugin until the point when the system plugin group is imported to trigger the onAfterInitialise event and should cause the configured plugin ordering to be respected when importing and dispatching events.

@andrepereiradasilva
Copy link
Contributor

IIRC of that part is used to define the user language, and we need the language filter plugin params to calculate that.

@infograf768 please check

@mbabker
Copy link
Contributor Author

mbabker commented May 8, 2016

That's why I'm importing the plugin's params without loading the plugin to the event dispatcher basically. It's still a big stinking pile of crap keeping a hardcoded dependency to the plugin, but it ultimately fixes a deeper issue in the order that observers are attached to the event dispatcher which is what Soren is trying to work around.

@andrepereiradasilva
Copy link
Contributor

yes i understand that.
i will test this when i have time.

@andrepereiradasilva
Copy link
Contributor

andrepereiradasilva commented May 8, 2016

I have tested this item ✅ successfully on d939290

Applied the patch and tested in a multilanguage sample site (3 languages):

  • Fallback with no cookie to site default language (system language filter plugin option)
  • Fallback with no cookie to browser default language (system language filter plugin option)
  • Use language cookie if exists
  • Language associations
  • Language routing

All worked with no issues


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

@mbabker
Copy link
Contributor Author

mbabker commented Jun 13, 2016

@aDaneInSpain This was in response to your issue...

@aDaneInSpain
Copy link
Contributor

Yes, looks good. Will get it tested ASAP. @vistiyos

@vistiyos
Copy link
Contributor

@mbabker Thank you for your approach but I think this will not solve the issue we are having. I would like to know if remove that hard core dependency of "languagefilter" plugin. Maybe some changes is need in several places, so I would love to get some advice from the people that know how this plugin works.

@mbabker
Copy link
Contributor Author

mbabker commented Jun 14, 2016

@vistiyos without moving the language filter plugin's parameters to either a component or the global configuration, there is no way to 100% decouple the site application from the plugin. Moving the params IMO is more hassle than it's worth (it's a B/C break moving the param from one source to another as now code using the plugin params would have to be updated to use a new source).

This change should resolve the underlying issue of how the plugin is imported into the event dispatcher which causes it to always be the first plugin called when an event is dispatched, regardless of the ordering configuration you have set.

#9999 is one possible fix to this issue but the "flaw" with it is that it is importing everything into the event dispatcher immediately. This PR explicitly is only loading the plugin's parameters (and inherently the plugin data into the static container in JPluginHelper); this PR makes an explicit change to NOT push the plugin to the event dispatcher.

@vistiyos
Copy link
Contributor

I have tested this item ✅ successfully on d939290

Applied the patch and tested in a multilanguage sample site (2 languages):

Fallback with no cookie to site default language (system language filter plugin option)
Fallback with no cookie to browser default language (system language filter plugin option)
Use language cookie if exists
Language associations
Language routing
All worked with no issues

@vistiyos
Copy link
Contributor

@mbabker Thank you so much for you help and your PR.

Looking forward to see it merge on the new release of Joomla!.

@roland-d roland-d added this to the Joomla 3.6.1 milestone Jun 24, 2016
@andrepereiradasilva
Copy link
Contributor

Can we have a RTC here?

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jul 4, 2016
@andrepereiradasilva
Copy link
Contributor

ok we have it now 😄

@roland-d roland-d merged commit afead45 into joomla:staging Jul 16, 2016
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jul 16, 2016
@mbabker mbabker deleted the alt-9999 branch July 16, 2016 11:12
@infograf768
Copy link
Member

@mbabker
This has broken multilang here
See #11161

@mbabker
Copy link
Contributor Author

mbabker commented Jul 17, 2016

Are you testing with both this patch merged plus that patch applied? If anyone is testing with only the changes to the language filter plugin applied then of course it would be broken because they are missing the changes to the application class.

If this is really causing issues I'm going to say someone else devise a patch to fix Soren's original issue because I don't get the language system enough to deal with this type of stuff. I just fixed what was a logical workflow issue.

@infograf768
Copy link
Member

i tested indeed with both.

@mbabker
Copy link
Contributor Author

mbabker commented Jul 17, 2016

Then either the plugin parameters aren't being correctly loaded in the application class or some other change has caused this to misbehave. In either case, I don't have the knowledge of how the language system works to make any further corrections, so if you want to revert it go ahead.

@aDaneInSpain
Copy link
Contributor

If this is reverted we need to re-open #9999

@andrepereiradasilva
Copy link
Contributor

no need to revert it. i think it's ok now. please confirm @infograf768

@infograf768
Copy link
Member

yep

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.

None yet

7 participants