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

DI\object() should not extend the previous definition by default #234

Closed
mnapoli opened this issue Mar 8, 2015 · 1 comment
Closed

DI\object() should not extend the previous definition by default #234

mnapoli opened this issue Mar 8, 2015 · 1 comment
Milestone

Comments

@mnapoli
Copy link
Member

mnapoli commented Mar 8, 2015

This is a BC break planned for next major version (5.0).

The problem is that we want to add decoration and definition extension, but that already exists for DI\object() today and it's implicit.

The reason it works implicitly today is to take advantage of autowiring/annotations as much as possible, for example:

class Foo {
    public function __construct(Bar $bar, $string) …
}
return [
    'Foo' => DI\object()
        // I only have to define the non type-hinted parameter
        ->constructorParameter('string', 'Hello world'),
];

If we add definition extension and decoration as a main feature with explicit syntax addition (e.g. adding items to arrays, etc.), should we make it also explicit for objects. This is a BC break.

return [
    'Foo' => DI\object()
        ->extend()
        ->constructorParameter('string', 'Hello world'),
];

Without ->extend() the definition is considered as a complete override of any previous existing definition (defined either in a file, autowiring or annotations… doesn't matter).

Pros

Cons

  • BC break: people have to update their config files
  • syntax more verbose to extend a previous definition
@mnapoli mnapoli added this to the 5.0 milestone Mar 8, 2015
@mnapoli mnapoli mentioned this issue Mar 8, 2015
31 tasks
@mnapoli mnapoli mentioned this issue Apr 9, 2015
5 tasks
@mnapoli
Copy link
Member Author

mnapoli commented Apr 11, 2015

I have finally decided not to introduce this change.

I have implemented it in #244 but after writing tests and playing with it, I realized some difficult problems where surfacing which meant that the level of complexity was much higher for the user. Even I had trouble writing very simple object definitions with the patch.

Regarding #193 this can be fixed regardless of this.

Regarding extending another entry like in Symfony, I have never met that use case yet (it's a very rare need): at worst we have to write twice the same definition, which is acceptable compared to adding complexity to every other use case.

Regarding "you can choose not to use it to replace the previous entry (which is currently impossible)" it is still possible to introduce an option to DI\object() to make that possible.

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

No branches or pull requests

1 participant