Skip to content

Comments

Support proxies with new class parameter#63

Closed
laoneo wants to merge 2 commits intojoomla-framework:4.x-devfrom
laoneo:lazy
Closed

Support proxies with new class parameter#63
laoneo wants to merge 2 commits intojoomla-framework:4.x-devfrom
laoneo:lazy

Conversation

@laoneo
Copy link
Contributor

@laoneo laoneo commented Apr 25, 2025

Alternative to #58. A new parameter is added which allows to define the class to proxy. A service can then be defined in such a way:

$container = new Container();
$container->set('foo', fn ($container) => new \stdClass(), false, false, \stdClass::class);

$obj = $container->get('foo'); // The $obj class s a proxy instance till it is used the first time
$ob->bar; // This will instantiate the class and nob $obj is a stdClass object

the callback function (second argument) will not be called before the first access will be done on the returned object.

Breaks backwards compatibility

Classes which extend the container and do override the set, protect, share and getResource functions must add the new parameter.

Copy link
Contributor

@voronkovich voronkovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested API without using the lazy flag looks more clean to me.


if (\is_callable($value)) {
$this->factory = $value;
if ($proxyClass && class_exists($proxyClass, false) && PHP_VERSION_ID >= 80400) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if class doesn't exist we should throw an exception.

@Fedik
Copy link
Contributor

Fedik commented Apr 26, 2025

It looks better, but still no go, sorry.

Few issue:

  • we do not need to modify ContainerResource, it just a resource holder.
  • later on it is will be hard to read the service/provider.php without knowing the container set() parameters.

As we already discussed, it can be much smaller and simpler helper:
With only Lazy Proxy support: 3.x-dev...Fedik:joomla-di:lazy-stuff-light
With all Lazy types support: 3.x-dev...Fedik:joomla-di:lazy-stuff

But because it just a helper, wrapper to feature that already can work, it has low priority, and can go in to any minor release.

@laoneo
Copy link
Contributor Author

laoneo commented Apr 26, 2025

This has nothing to do with the provider.php file. It is an additional way to register a resource. You need an extra class parameter, because in most of the cases you register an interface but the class to proxy needs to be about the concrete implementation class. So you actually need two parameters for this, one is the key and the other the actual class to implement. That's why your suggestions do not work, as far as I can read the code.

It can be done with a new lazy function to be bc, but it will end up with the copied code from the set class with the additional lazy code. We will discus this issue in the next maintainer meeting.

@Fedik
Copy link
Contributor

Fedik commented Apr 26, 2025

You need an extra class parameter, because in most of the cases you register an interface but the class to proxy needs to be about the concrete implementation class. So you actually need two parameters for this,

Not really,
You register resource with concrete class. When you need access by interface, you add an alias, clean and easy.

$container
  ->lazy(Foobar::class, $factory)
  ->alias(FoobarInterface::class, Foobar::class);

@laoneo
Copy link
Contributor Author

laoneo commented Apr 27, 2025

It looks wrong and complicated to me when I have to create an alias for an interface resource.
Please open a pr for that approach, so we have then an alternative yo discus. At the end of the day I want to have proxy support. If it goes with a new function or with a changed on I don't care.

@Fedik
Copy link
Contributor

Fedik commented Apr 28, 2025

It looks wrong and complicated to me

Yours looks not easier 😉

Compare final use:

1)
$container
  ->set(FoobarInterface::class, $factory, false, false, Foobar::class);

2)
$container
  ->lazy(Foobar::class, $factory)
  ->alias(FoobarInterface::class, Foobar::class);

What variant is more easy for Developer to guess what is going on (without looking in source code of the container).
1) or 2)?

Third option would be what @HLeithner suggested:

3)
$container
  ->set(FoobarInterface::class, $container->lazy(Foobar::class, $factory));

Please open a pr for that approach, so we have then an alternative yo discus

I could do that, but better keep discussion in one place, decide which variant is optimal for us, and then go from there.

@laoneo
Copy link
Contributor Author

laoneo commented Apr 28, 2025

I would love to discus this in a maintainer meeting, so having your pr would make things easier.

@voronkovich
Copy link
Contributor

I would suggest to use named arguments to improve readability:

$container->set(
      FoobarInterface::class,
      fn () => new Foobar(),
      shared: false,
      protected: false,
      proxyClass: Foobar::class,
);

@HLeithner
Copy link
Contributor

I would suggest to use named arguments to improve readability:

$container->set(
      FoobarInterface::class,
      fn () => new Foobar(),
      shared: false,
      protected: false,
      proxyClass: Foobar::class,
);

Joomla doesn't support named arguments and it's not part of the b/c policy which could lead to unwanted side effects.

@Fedik
Copy link
Contributor

Fedik commented Apr 28, 2025

@laoneo there is PR #64

@laoneo laoneo closed this May 1, 2025
@laoneo laoneo mentioned this pull request May 1, 2025
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.

4 participants