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

Remove scopes #409

Closed
mnapoli opened this Issue May 15, 2016 · 12 comments

Comments

Projects
3 participants
@mnapoli
Member

mnapoli commented May 15, 2016

The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.

@mnapoli mnapoli added the enhancement label May 15, 2016

@mnapoli mnapoli added this to the 6.0 milestone May 15, 2016

mnapoli added a commit that referenced this issue May 15, 2016

Fixes #409 Remove scopes
The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.

@mnapoli mnapoli referenced this issue May 15, 2016

Merged

Remove scopes #410

mnapoli added a commit that referenced this issue Jun 1, 2016

Fixes #409 Remove scopes
The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.
@bradynpoulsen

This comment has been minimized.

Contributor

bradynpoulsen commented Aug 18, 2016

👍

mnapoli added a commit that referenced this issue Dec 27, 2016

Fixes #409 Remove scopes
The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.

@mnapoli mnapoli referenced this issue Dec 27, 2016

Closed

PHP-DI 6 ideas #395

@bradynpoulsen

This comment has been minimized.

Contributor

bradynpoulsen commented Dec 29, 2016

@mnapoli I just wanted to verify that I understand the implications of removing these scopes.
If I have an object that should be receiving a new instance of a dependency every time, I would now have to inject a literal Factory object instead and then use that factory to get the new instance? Or am I missing that there is still support for this in the container definitions?

@mnapoli

This comment has been minimized.

Member

mnapoli commented Dec 29, 2016

@bradynpoulsen that's correct. I'm still on the fence for this. It would bring some simplifications (and perf improvements since they go hand in hand), but I'm afraid it might be a blocker for some applications…

@mnapoli

This comment has been minimized.

Member

mnapoli commented May 30, 2017

Is there anyone using PHP-DI that would be against removing scopes?

If so please explain your use case :)

mnapoli added a commit that referenced this issue Jun 3, 2017

Fixes #409 Remove scopes
The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.

mnapoli added a commit that referenced this issue Jun 3, 2017

Fixes #409 Remove scopes
The goal is to simplify the package for the v6. Scopes are useless: I don't see a valid use case for the "prototype" scope, if needed then it should be replaced by an actual factory.

By removing scopes the API would be simpler as well as the internals of PHP-DI.

@mnapoli mnapoli closed this in #410 Jun 3, 2017

@mnapoli mnapoli moved this from Unsure to Done in PHP-DI v6 Jun 3, 2017

@holtkamp

This comment has been minimized.

Contributor

holtkamp commented Oct 26, 2017

Is there anyone using PHP-DI that would be against removing scopes?

🙋‍♂️

If so please explain your use case :)

Ok, will try to 😉

UseCase: ViewHelpers
In some cases the use of the "prototype" scope makes perfect sense to me: ViewHelpers. These classes are typically used in ViewScripts to generate "often used" HTML (or some other format).

For example, to render a button element in a ViewScript index.phtml, instead of:

<button id="button1" class="btn-success">ClickMe</button>

a Button ViewHelper class could be used. Note: the request to 'button()' is intercepted by the View::__call() which fetches it from the DependencyInjectionContainer...

For example:

echo $this->button()
    ->setId('button1')
    ->addClass('btn-success')
    ->addContent('ClickMe')
    ->render();

//results in: <button id="button1" class="btn-success">ClickMe</button>

When using the default scope of "singleton", one is forced to "reset" the internal state of the ViewHelper. So in this example we need to reset:

  • the $id property
  • the $attributes property (which is an array)
  • the $content property (which is an array)

When we do NOT reset the internal state and re-use the instance of the ViewHelper, the following occurs:

echo $this->button()
    ->setId('button1')
    ->addClass('btn-success')
    ->addContent('ClickMe')
    ->render();

//results in: <button id="button1" class="btn-success">ClickMe</button>

echo $this->button()
    ->setId('button2')
    ->addClass('btn-warning') //classes are separated by a space
    ->addContent('ClickMeToo')
    ->render();

//results in: <button id="button2" class="btn-success btn-warning">ClickMeClickMeToo</button>

The main challenge in resetting an object "completely" is when it is part of an inheritance hierarchy and the parent classes have properties (private/protected) themselves as well. This involves quite some use of the Reflection classes 😨 .

I started using the Prototype scope for such ViewHelpers. With a fresh object instance each time the ViewHelper is acquired, no more resetting of the objects is required!

@bradynpoulsen

This comment has been minimized.

Contributor

bradynpoulsen commented Oct 26, 2017

Why isn't button() a factory for your button view helper?

@bradynpoulsen

This comment has been minimized.

Contributor

bradynpoulsen commented Oct 26, 2017

@mnapoli An interesting alternative would be to handle scopes like they are handled in Google's Dagger 2. Everything by default is a "prototype" and then the scope is the mechanism that does caching of an instance

@holtkamp

This comment has been minimized.

Contributor

holtkamp commented Oct 27, 2017

Why isn't button() a factory for your button view helper?

you mean like this (PHP-DI 5):

\View\Helper\Html\Element\Button::class => function(){ return new \View\Helper\Html\Element\Button(); }

Yeah, guess that is the same as:

\View\Helper\Html\Element\Button::class => object()->scope(Scope::PROTOTYPE)->lazy()

If it is the same, then I stand corrected... 😉

@mnapoli

This comment has been minimized.

Member

mnapoli commented Oct 27, 2017

@holtkamp nope, using "php-di factories" is not the prototype scope. I guess @bradynpoulsen meant "class factory" as in "the design pattern".

E.g. a ButtonFactory class (built as singleton by PHP-DI) with a create() method. That's (IMO) the recommended way, my goal with this removal was to stop making the container a factory when calling get().

Another solution is to call $container->make() instead of get() in $this->button() => that's in line with my goal of making get() predictable (not behaving as a factory) and make() explicitly behaving as a factory. Is that possible in your case? I think that's what @bradynpoulsen meant with:

Why isn't button() a factory for your button view helper?

@mnapoli An interesting alternative would be to handle scopes like they are handled in Google's Dagger 2. Everything by default is a "prototype" and then the scope is the mechanism that does caching of an instance

My main problem really is that I want get() to be clear about what it does (and also it simplifies the internals a lot). If make() (or writing and injecting a factory class) is a good solution then I'd rather add more code and complexity.

@holtkamp

This comment has been minimized.

Contributor

holtkamp commented Oct 27, 2017

E.g. a ButtonFactory class (built as singleton by PHP-DI) with a create() method

Mm, wel, that would an explosion of these rather "useless" factory classes... If this can be avoided, that would be nice.

Another solution is to call $container->make() instead of get() in $this->button()

Aah, ok, that sounds better.

Is that possible in your case?

Yeah sure, will have my View object use $container->make() to make new ViewHelper instances when they are needed. Sounds good.

I did notice that make() is not part of Psr\Container\ContainerInterface? That is a small detail ofcourse, but also for the $container I tend/try to use the interface instead of the implementation 🤓

@mnapoli

This comment has been minimized.

Member

mnapoli commented Oct 27, 2017

Perfect! And yes it's not in ContainerInterface because it's not the job of the container :D

See http://php-di.org/doc/container.html#make but you can inject FactoryInterface, because what you need is a factory ;)

@holtkamp

This comment has been minimized.

Contributor

holtkamp commented Oct 31, 2017

Another solution is to call $container->make() instead of get()

I tried this using 6.0.0-alpha4 but the same instance of the object is returned over and over.

Reported it in a separate issue: #537

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment