Replacing object definitions: inconsistent behavior of DI\object()

[mindplay-dk]

Per the manual:

A DI\object() definition always extends a previous object definition

So then, what do you do if you need to replace a previous object definition?

The reason for this is to allow to easily extend autowiring and annotations definitions:

This may be the most common use-case, but it's not the only use-case.

DI\object() is the only kind of definition that extends by default: all other definitions override the previous definition completely.

Even had that in bold, because this behavior is inconsistent with the rest of the API.

I would strongly prefer a consistent API to lessen the learning curve (we're a big team) and a dedicated additional function for extending a previous definition, e.g. DI\extend() or maybe DI\override() to make this an explicit operation. (Implicit behavior is risky. Behavior that varies based on previous method calls is confusing.)

This would also eliminate the case where you try to "extend" something that hasn't been defined at all, and potentially end up with something that partially works, but probably not as intended.

(PS: I've opened a few issues by now - feel free to ask me to PR on any of those)

[mindplay-dk]

Okay, maybe I'm just asking for DI\decorate() which is already there.

The inconsistency is still an issue though.

[mnapoli]

I completely understand. To be honest the decision on this one was a tough one, I went with the current behavior (always extend) because of:

• backward compatibility (main reason) - changing the behavior so subtly between v4 and v5 was very risky
• main use case was to override autowiring (not the only use case, as you said)
• making the main use case more difficult to use and understand was not a good idea

I have played with a lot of different syntaxes when working on v5 (e.g. DI\extend(), DI\object()->extend(...), …) but it was very confusing. See this #234 (comment) and also #244

The solution I considered but didn't implement yet (feel free to suggest another) is to add a new DI\create() helper (name to be decided). That could be confusing to have both DI\object() and DI\create().

Anyway, I understand your trouble. I want to find a solution to this too. Not extending by default was creating a terrible developer experience from what I've tested. Especially since most users don't picture autowiring as a definition you can extend (DI\object()->extend() alone was just confusing: what does it extend, there's nothing to extend). Maybe we can imagine something like DI\autowiring() or DI\object()->autowire(), but that means making autowiring a "special" definition source. But if it's better for the user, why not…

So feel free to suggest.

[mindplay-dk]

Okay, so here's what I would suggest:

• add DI\create() (basically DI\object() but checking against a previous definition)
• add DI\extend() (again, basically DI\object() but checking for a previous definition)
• deprecate (but don't remove) DI\object - retaining backwards compatibility (perhaps indefinitely) but guiding everyone torwards a cleaner solution.

Both create and extend are verbs, consistent with decorate, so all functions for working with objects would be verbs.

Deprecating object() will prompt you to clarify your definitions by choosing either create() or extend(), so the red scribble under object() in an IDE should provide useful guidance.

Does it make sense to extend() an auto-wired definition? I don't think so, but maybe I'm overlooking something.

Having create() and extend() in the same definition would probably be a pretty exotic case? So one file might read:

ConnectionConfig::class => create(),

And another file might read:

ConnectionConfig::class => extend(function (ConnectionConfig $config) {
    $config->username = 'root';
    $config->password = '12345678';
}),

Although I'm not a big fan of duplicating the type-hint here, it's required for IDE support.

We also added a configure method to ContainerBuilder for the same reason - in our configuration scripts, we do something like this instead:

$container->configure(function (ConnectionConfig $config) {
    $config->username = 'root';
    $config->password = '12345678';
});

The configure() method reflects on the parameter type and builds an appropriate definition that "replaces" ConnectionConfig with the same instance after exposing it to the given closure. It works nicely, but it felt a bit lumpy. Sometimes you don't want to replace, you just want to expose an existing object to a closure at the time when it gets resolved, to set a property or change a setting. We do this all the time. It was a central feature in the DI container we used before. (I can post the code tomorrow if you'd like, I don't have it with me right now.)

Not sure if I'm getting sidetracked here, sorry. I've been coding for 20 hours in two days :-)

[mnapoli]

I think the DI\extend() you suggested is exactly what DI\decorate() is doing. E.g. I think you can replace you $container->configure() with DI\decorate() and be done with it:

ConnectionConfig::class => decorate(function (ConnectionConfig $config) {
    $config->username = 'root';
    $config->password = '12345678';
    // Don't forget to return the new or updated object!
    return $config;
}),

But you could also even do it with object() if I understood your example correctly:

// config-base.php
ConnectionConfig::class => object(),

// config-local.php
ConnectionConfig::class => object()
    ->property('username', 'root')
    ->property('password', '12345678'),

What I have in mind for DI\extend() (or any other name) is exactly the behavior of DI\object() today. I.e. define object injections (not use a closure) like ->constructor('foo', 'bar', get(Logger::class)), but all the while keeping any previous definition. If the previous definition defined a constructor injection, then I will be overriding it, but if it defined a setter injection I would keep it.

Again for this the main use case is for autowiring: you want to be able to define what to inject in a constructor parameter that isn't type-hinted. The second use case is to be able to override a parameter/property injection in another config file (e.g. the example above). The third, minor, use case is to extend a parent config, here is how Symfony does it: http://symfony.com/doc/current/components/dependency_injection/parentservices.html

So to sum up:

• we need a way to define how to create an object from scratch (major use case)
  • currently not implemented, but object() will work correctly to do that in 90% of the cases (because either there is no parent/autowiring definition, or we define all parameters anyway so we override the parent definition completely (no side effect because of the parent definition)
• we need a way to override/complete autowiring definitions
  • currently object() does that
• we need a way to customize/complete a previous (PHP config) definition (to e.g. override a single parameter)
  • currently object() does that
  • decorate() can do that too
• we need a way to extend a different (PHP config) definition to factorize configuration
  • it's a minor use case, quite complex and confusing, I'd say it's fine to not implement it (for now)
• we need a way to decorate the object returned by a previous definition
  • decorate() does that

So object() has a valid behavior, but maybe an invalid name. And we need to be able to define an object from scratch with 100% guarantee not to extend anything else (which doesn't exist today, unless using a closure but that's a workaround).

[mindplay-dk]

I think you can replace you $container->configure() with DI\decorate() and be done with it

I can, but it's intended to replace values rather than updating existing values - if you forget to return the same value at the end, you accidentally remove the value. This is great for e.g. scalar values, but for objects, I'd prefer to have a function that just performs further initialization on an existing object without replacing anything, e.g. without the required return statement.

But you could also even do it with object() if I understood your example correctly

I could, but I try to avoid the DSL and strongly favor using closures, because using the DSL breaks automated refactoring and IDE inspections, due to the fact there is no static coupling. I strongly prefer using PHP code when possible, rather than making weak references to parameter names, method-names, etc. - if I can't trust automated refactoring, I feel I'm doing it wrong; I want all my code to pass inspections and survive simple operations like renaming a property, method or parameter, changing parameter order, etc.

we need to be able to define an object from scratch with 100% guarantee not to extend anything else

Well, decorate only decorates if you choose to decorate - the name is a bit misleading in that sense, as what you're actually doing, is replacing the value. Whether you replace it with a decorator or something different is really up to you.

[mnapoli]

I can, but it's intended to replace values rather than updating existing values - if you forget to return the same value at the end, you accidentally remove the value.

OK I see. However I'm not 100% convinced (personally) that's such a big issue, because in the end it's just a matter of remembering to return the value.

I could, but I try to avoid the DSL and strongly favor using closures, because using the DSL breaks automated refactoring and IDE inspections

Fair enough, totally valid reasons.

Well, decorate only decorates if you choose to decorate - the name is a bit misleading in that sense, as what you're actually doing, is replacing the value. Whether you replace it with a decorator or something different is really up to you.

Yep, the goal was to provide a replace() and extend() into one. "decorate" felt like a middleground name to imply that you could do both with it, it's up to you.

I wonder if adding replace() and extend() (and deprecate decorate() probably) would make things simpler or more complicated… I'm not sure honestly. For users going through the docs, that's 2 concepts to discover and understand (and understand the subtle difference of when to use each). Whereas having one (more generic) concept might be a little easier…

Maybe the documentation can be improved to show 2 examples for decorate(): one replacing the value, and one just modifying it.

[mindplay-dk]

in the end it's just a matter of remembering to return the value

Yeah, I don't personally want to write integration tests for every container configuration ;-)

the goal was to provide a replace() and extend() into one. "decorate" felt like a middleground name to imply that you could do both with it, it's up to you.

Okay, makes sense.

So, what do you think about just changing the current behavior, so that, if the function doesn't return anything, it keeps the current value? Is there a real use-case where you would want to deliberately replace a current value with null? If not, we can simply change it so that returning a value means replace it, and not returning a value means don't - seems pretty intuitive?

[mnapoli]

so that, if the function doesn't return anything, it keeps the current value?

it does sound good but…

Is there a real use-case where you would want to deliberately replace a current value with null?

unfortunately yes :( It's a use case I've seen in Piwik for example, but can't remember exactly what it was. Anyway that's definitely valid (configuring e.g. optional dependencies).

I also thought of that solution but the goal was to throw an exception if decorate() doesn't return anything. Your suggestion is maybe better. But in any case, it's not really doable I think.