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

CMSPlugin refactoring and magic late initialisation #39088

Closed
wants to merge 16 commits into from
Closed

CMSPlugin refactoring and magic late initialisation #39088

wants to merge 16 commits into from

Conversation

nikosdion
Copy link
Contributor

Pull Request for Issue #39077 .

Summary of Changes

  • Refactored legacy code out of CMSPlugin and into traits. 6.0 can remove the use statements for the legacy implementation traits and 7.0 can remove the traits.
  • Added magic late initialisation. See below for more information.

Testing Instructions

  • Install a Joomla 4.3 (dev) site
  • Install a bunch of third party extensions if you want

Actual result BEFORE applying this Pull Request

The site works.

Expected result AFTER applying this Pull Request

The site still works!

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

Magic Late Initialisation in CMSPlugin

In the past, initialising the plugin was typically done in its constructor. However, plugin objects are constructed for all published plugins as soon as the plugin group they belong in is imported via \Joomla\CMS\Plugin\PluginHelper::importPlugin. Even if the plugin's event handlers are not used the initialisation has executed, with an adverse impact to the site's performance.

This PR introduces a new method, doInitialise. Developers can override it and put their plugin's initialisation code in there. This method is called automatically and exactly once, right before the first event handler of this plugin executes. This makes the plugin's initialisation code execute only when it's truly going to be needed during the request's lifetime. If the plugin's event handlers are not used the initialisation code never runs, therefore mitigating the performance impact.

It is possible that a developer might need to disable this feature completely, e.g. because the initialisation is already executed late in the constructor of services which are accessed through an object implementing the Factory pattern as needed — meaning that the initialisation code runs even later than what the doInitialise method itself can do. In this case, the plugin can set the allowLateInitialisation to false.

Performance impact

I was skeptical as to what the performance impact of late initialisation would be since we are essentially doubling the event handers for each event. To quantify the impact I used Apache Benchmark against the Joomla sample data and the URL index.php/sample-layouts/blog which calls as many events as you are going to get with the sample data.

The command line used was ab -n 500 -c 5 'https://jdev4.local.web/index.php/sample-layouts/blog'. The site runs on a local server on my MacBook Pro with an Apple M2 processor, 24GiB or RAM, Apache 2.4.54, MySQL 8.0.30, and PHP 8.1.10 (all installed with HomeBrew on macOS Ventura). PHP has OPcache enabled with a revalidation frequency of 2 seconds.

Results WITHOUT late initialisation:

Time taken for tests:   7.199 seconds

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        2    3   1.0      3       8
Processing:    55   67  15.1     63     176
Waiting:       54   66  14.9     62     175
Total:         57   70  15.2     66     184

Percentage of the requests served within a certain time (ms)
  50%     66
  66%     68
  75%     69
  80%     70
  90%     79
  95%     93
  98%    131
  99%    136
 100%    184 (longest request)

Results WITH late initialisation:

Time taken for tests:   7.224 seconds

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        2    4   1.5      4       9
Processing:    53   66  16.0     62     169
Waiting:       52   65  15.8     61     164
Total:         56   70  16.0     66     175

Percentage of the requests served within a certain time (ms)
  50%     66
  66%     68
  75%     69
  80%     70
  90%     78
  95%     97
  98%    139
  99%    150
 100%    175 (longest request)

Conclusion: the performance impact is below what can be reasonable measured. The random performance deviation of the server because of OS load, filesystem access, thermal management, PHP compilation, etc has a far more pronounce impact on the test results than the late initialisation feature.

Backwards Compatibility Notes

The PR is written in such a way as to maintain full backwards compatibility with Joomla 4.2 (which is fully b/c to Joomla 4.0 and mostly b/c to Joomla 3.10). If a plugins works on Joomla 4.0–4.2 it will still work with this PR applied.

Deprecation Notes

Here is the intention of this PR with regards to deprecations and b/c management over the next three major versions of Joomla (5, 6, and 7).

Plugins should start using the SubscriberInterface and handle events instead of having callback listeners with a variable number of arguments. The CMSPlugin will stop using the legacy code traits in Joomla 6.0 but the traits will not be removed until Joomla 7.0. This gives a minimum of four and a maximum of six years after Joomla 5.0's release for plugins to be refactored. Note that this is in addition to the five years between the introduction of SubscriberInterface in the Joomla 4 alpha versions and the release of Joomla 5.0.

Plugins should move their initialisation to doInitialise. The constructor WILL NOT be made final as it still needs to be overridden when the developer wants to push services to the plugin object through the constructor in the service provider. Any initialisation in the constructor beyond assigning services pushed by the service provider is, however, now considered a bad practice and not in line with the philosophy of the Joomla architecture.

Plugins should not expect that the loadLanguage method will be available by default. Plugins should use the \Joomla\CMS\Plugin\LanguageAwareTrait to make sure the loadLanguage method is available. The CMSPlugin class will stop loading this trait automatically in Joomla 6.0. Using this trait until Joomla 6.0 is released WILL NOT cause any adverse affects to plugins. Therefore, plugin developers can start refactoring their code as soon as this PR is merged.

The autoloadLanguage property is now considered deprecated and will be removed in Joomla 6.0. Plugins need to use the \Joomla\CMS\Plugin\LanguageAwareTrait and call $this->loadLanguage() manually in their doInitialise method instead of setting autoloadLanguage to true. DO NOT try to load any language files in the constructor.

Nicholas K. Dionysopoulos added 4 commits October 27, 2022 17:48
Move application object management into a new ApplicationAwareTrait
Move legacy and optional stuff into traits
I forgot to add something here.
@richard67
Copy link
Member

@nikosdion The issue can be closed as we have this PR now? Or is there something remaining so it should be kept open?

@nikosdion
Copy link
Contributor Author

@richard67 Yes, you can close #39088 as this PR fully addresses the reported problem (and then some).

@richard67
Copy link
Member

@nikosdion Well, since you are the author of the issue, you should have the privilege to close it yourself. It would have saved us time.

@richard67
Copy link
Member

@nikosdion Unit tests for the CMS plugin are failing, see https://ci.joomla.org/joomla/joomla-cms/59133/1/11 . Possibly they need some update, too?

Nicholas K. Dionysopoulos and others added 3 commits October 27, 2022 22:03
Co-authored-by: Brian Teeman <brian@teeman.net>
Co-authored-by: Brian Teeman <brian@teeman.net>
@richard67
Copy link
Member

Unit tests still failing, see my previous comment.

Nicholas K. Dionysopoulos added 4 commits October 28, 2022 11:36
Clearly, I need more coffee
@nikosdion
Copy link
Contributor Author

@richard67 I was asleep last night :) I have fixed the tests today and added more tests for the late initialisation code.

@richard67
Copy link
Member

richard67 commented Oct 28, 2022

@richard67 I was asleep last night :) I have fixed the tests today and added more tests for the late initialisation code.

@nikosdion Sure, you've deserved to sleep and eat and live of course. I did not want to push, only wanted to be sure it doesn't get lost within all the other review comments. Thanks for fixing the unit tests. It will make the other tests (API and system) run, too.

Update: The unit tests are still failing. All fine now.

Co-authored-by: Richard Fath <richard67@users.noreply.github.com>
Nicholas K. Dionysopoulos added 3 commits October 28, 2022 13:20
We need to set allowLateInitialisation
to false for CMSPlugin to only
register one listener per event.
@nikosdion nikosdion closed this Dec 7, 2022
@nikosdion nikosdion deleted the feature/39077_cmsplugin_refactoring branch December 7, 2022 12:31
@nikosdion nikosdion restored the feature/39077_cmsplugin_refactoring branch December 7, 2022 13:40
@nikosdion nikosdion reopened this Dec 7, 2022
@nikosdion
Copy link
Contributor Author

I am not going to be contributing to this project anymore. I won't be replying to any questions or making any changes until then. I will close any outstanding PRs and delete their code on December 31st, 2022. Merge whatever you want until then.

@nikosdion
Copy link
Contributor Author

I guess plugins still going through the CMSFactory to load their language is not considered a problem, given it's three months with no sign of anyone willing to merge it, so I am closing this PR.

@nikosdion nikosdion closed this Mar 5, 2023
@nikosdion nikosdion deleted the feature/39077_cmsplugin_refactoring branch May 10, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants