di icon indicating copy to clipboard operation
di copied to clipboard

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

Open jakubboucek opened this issue 5 years ago • 6 comments

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 ❤️

jakubboucek avatar Feb 08 '20 03:02 jakubboucek

It makes sense.

dg avatar Feb 09 '20 15:02 dg

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.

JanTvrdik avatar Feb 12 '20 09:02 JanTvrdik

@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!

jakubboucek avatar Feb 12 '20 10:02 jakubboucek

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 a1b1e942b346fccef86493caa543aa0ba74c7919 could be the beginning to replace the initialize() function with something more appropriate.

dg avatar Feb 12 '20 11:02 dg

I promise, next time I will be more read, than write :)

jakubboucek avatar Feb 12 '20 11:02 jakubboucek

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

dg avatar Feb 12 '20 12:02 dg

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 https://github.com/orisai/nette-http/commit/cc6e686371091b1583c13b4d95fa817d1c1cebbf#diff-4c400ef0e67cf26d9094888eff469b5ff8e40233d46419e066ef2117a4361837L51-R72

It was also helpful to allow configurator to not call initialize() https://github.com/orisai/nette-di/commit/76f8cafa02f027bbcefed1286db8135bb07a6224#diff-3a497e56f0277c5ca4e8660abda9bca83312dfb21948ac71ca91a343c5bf928aL252-R264

mabar avatar Nov 29 '22 23:11 mabar