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

Method initialize() should be part of DI\Container life-cycle & interface #232

Open
jakubboucek opened this issue Feb 8, 2020 · 6 comments
Milestone

Comments

@jakubboucek
Copy link

jakubboucek commented Feb 8, 2020

Some Nette DI extensions are using the container's initialize() method to append some logic which must be executed at end of container's setup phase.

But initialize() is just popular convention, not interface. That produces few problems:

initialize() method is not called automatically before the first use of the container, neither from DI\ContainerLoader nor DI\Container internally. The app is here responsible to call this method externally.

Container instance is generated at the run-time phase. A developer during developing has no way how to guarantee how the geterated container will look because app can only use DI\Container class interface which doesn't contains initialize() method (please, let Reflection sleep).

That's mean, PhpStorm is yelling inspections the method is undefined.

image

App never should use anything out of known interface.

That was formally problem, now the serious problem:

Look to two examples of app's config.neon files which is using ConstantsExtension:

parameters:
constants:
    HELLO: "world"
parameters:
constants:

The first one is create container with initialize() method. If the app does not call them, app will be incorrectly booted.
The second one is create container without them. If the app try call them, app will be crash info fatal error.

That's create very fragile stability of app design.

My suggestions

  • Add empty initialize() to DI\Container to create stable interface.
  • Add to container's life-cycle fixed point to invoke initialize(). Especially to documentation, because currently is mentioned as optional convention, but actually is it very important milestone.

I am happy happy to send PR, but first let me know my idea is valid or not.

Thanks for great tool ❤️

@dg
Copy link
Member

dg commented Feb 9, 2020

It makes sense.

@JanTvrdik
Copy link
Contributor

Note that initialize method usually has side-effects which are not always desired. So there should be a way to create container without running initialize. Ideally I'd like to see initialize method moved outside of Container (because the code is more like bootstrap than container) but I don't see a practical way to it.

@jakubboucek
Copy link
Author

@JanTvrdik You right. Make autoinvoke this method looks like bad practice.

I think about your words, now whole initialize method looks like part for Configurator, not DI\Container because all known examples of that are out of main DI Container's business. For example ConstantsExtension is side-effect only.

Kua!

@dg
Copy link
Member

dg commented Feb 12, 2020

initialize() is not part of the DI container's responsibility and is actually part of the Configurator that calls it. The current form is a big compromise, but very functional and practical.

I think the gradual adaptation of this a1b1e94 could be the beginning to replace the initialize() function with something more appropriate.

@dg
Copy link
Member

dg commented Feb 12, 2020

Why? Sometimes it is the best to discuss :-)

@mabar
Copy link
Contributor

mabar commented Nov 29, 2022

If initialize() is replaced by some other form of initialization, please keep it optional. I just found a use case which would be quite hard to test if initialize() was called orisai/nette-http@cc6e686#diff-4c400ef0e67cf26d9094888eff469b5ff8e40233d46419e066ef2117a4361837L51-R72

It was also helpful to allow configurator to not call initialize() orisai/nette-di@76f8caf#diff-3a497e56f0277c5ca4e8660abda9bca83312dfb21948ac71ca91a343c5bf928aL252-R264

@dg dg added this to the v4.0 milestone Jan 17, 2023
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

No branches or pull requests

4 participants