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

[6.0] CMSPlugin: use Lazy Object feature #45062

Draft
wants to merge 25 commits into
base: 6.0-dev
Choose a base branch
from

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Mar 3, 2025

Summary of Changes

Alternative to #43658

Use of PHP Lazy Object feature for plugins.
This allows to instantiate the plugin only when event is triggered, and saving some resources (time and memory).

This does not applied automatically for every plugins, the plugin service provider need to be updated to support Lazy Object.

Few thing that need to figure out:

  1. Following code will trigger lazy object initialization, we need to get rid of it, somehow.

    if ($dispatcher && $plugin instanceof DispatcherAwareInterface) {
    $plugin->setDispatcher($dispatcher);
    }

  2. Current use of registerListeners() need to be deprecated. There is PR for it:

As next step we have to:
OR change the return type to boolean for registerListeners(),
OR replace it with new method, see the PR:

Testing Instructions

Apply patch, and test on PHP 8.x and 8.4
The following plugins should work as before:

  • schedulerunner
  • tasknotification
  • webauthn

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: TBD
  • No documentation changes for manual.joomla.org needed

I will rebase it to 6.0 later.

@laoneo
Copy link
Member

laoneo commented Mar 3, 2025

As said already, this should be done in the DI container and not every plugin itself should implement this. Or do you see problems when the DI container does that?

@Fedik
Copy link
Member Author

Fedik commented Mar 3, 2025

How to?

@laoneo
Copy link
Member

laoneo commented Mar 4, 2025

Similar cod you have in the service provider, I would do here https://github.com/joomla-framework/di/blob/3.x-dev/src/ContainerResource.php#L160.

@Fedik
Copy link
Member Author

Fedik commented Mar 4, 2025

hm, yeah, that also could work, need to check. We need it to be flexible.

But I thought that you have some ready to use solution :)

@HLeithner HLeithner changed the base branch from 5.3-dev to 6.0-dev March 4, 2025 17:18
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 6.0-dev.

@HLeithner HLeithner changed the title CMSPlugin: use Lazy Object feature [6.0] CMSPlugin: use Lazy Object feature Mar 4, 2025
@rdeutz rdeutz removed the PR-5.3-dev label Mar 5, 2025
@voronkovich
Copy link
Contributor

I would suggest to add a simple shortcut for creating lazy proxies:

// Container
public function lazy(string $class, callable $factory): callable
{
    if (PHP_VERSION_ID < 80400) {
        return $factory;
    }

    return function () use ($class, $factory) {
        return (new \ReflectionClass($class))->newLazyProxy(fn() => $factory($this));
    };
}

Then defining lazy service would look like this:

$container->share(
    'foo',
    $container->lazy(Foo::class, function($container) {
        return new Foo($container->get('bar'));
    }),
);

Full implementation you can see here:

@voronkovich
Copy link
Contributor

I've created a PR with more advanced implementation: joomla-framework/di#58

@Fedik
Copy link
Member Author

Fedik commented Mar 6, 2025

@laoneo That is require changes in Container code with b/c breaks (need new flags and changed method signature).
@voronkovich That implementation does not look good.

I will keep it without DI, there more important issue that need to resolve to make it work.

@laoneo
Copy link
Member

laoneo commented Mar 6, 2025

I'm pretty sure that this can be achieved in a backwards compatible way or with a minimum of bc breaks which shouldn't affect the CMS code.

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.

7 participants