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

Adjust array definition format (+ some new features) #60

Closed
xepozz opened this issue Oct 26, 2020 · 18 comments
Closed

Adjust array definition format (+ some new features) #60

xepozz opened this issue Oct 26, 2020 · 18 comments
Assignees
Labels
status:ready for adoption Feel free to implement this issue. type:enhancement Enhancement

Comments

@xepozz
Copy link
Member

xepozz commented Oct 26, 2020

Main mind

I suggest to group definition keys into by section:

Constructor, calls and class already existing, but I don't the style of configuration.
Now it looks like the following:

MyClass::class => [
  '__class' => MyClass::class,
  '__construct()' => [/* arguments */],
  'method1()' => 'val1',
  'method2()' => 'val2',
  'method3()' => 'val3',
]

What's bad:

  1. Class name must be in __class key
  2. Constructor arguments must be in __construct() key
  3. Any call of method must be postfixed with ()

What's will:

  1. Class can be in just class key
  2. Constructor arguments must be in separated group just constructor (or construct, or arguments)
  3. Methods calls must be in separated group and () won't needed

What's new:

  1. fluentCalls: new feature that means that methods will calls in fluent-like style: $newObject = $object->withProp($val)
  2. properties: new feature that means that any public (or any, need to think) properties also can be configured just as usual method calls
  3. aliases: new feature that means that you can add alias to this definition to use short aliased name instead of full qualified class name (e.g. doctrine.em instead of \Doctrine\Common\EntityManagerInterface)
  4. lazy: new feature that means that definition must be resolved even when the class is not used in an application (a.k.a. eager definition resolving). For example, it can be used when you need to make the bootstrapping an application

Final resullt

After all adjustments the definition can be looks like this:

MyClass::class => [
  'class' => MyClass::class,
  'tags' => ['tag1', 'tag2'],
  'constructor' => [
    'argument1',
    'argument2',
    'host' => 'host',
    '...variadic' => ['val1', 'val2', 'val3'],
  ],
  'calls' => [
    'method1' => 'val1',
    'method2' => 'val2',
    'method3' => 'val3',
  ],
  'fluentCalls' => [
    'method4' => 'val4',
    'method5' => 'val5',
    'method6' => 'val6',
  ],
  'properties' => [
    'property1' => 'val1',
    'property3' => 'val2',
  ],
  'reset' => fn() => true,
  'aliases' => ['aliased_name1', 'aliased_name2'],
  'decorators' => [
    Decorator1::class,
    Decorator2::class,
    Decorator3::class,
  ],
  'lazy' => false,
]
@viktorprogger
Copy link
Contributor

I like the general idea. But I have a question to the properties proposal: do we really need such a feature when any property can be initialized within a constructor parameters or another method call?
And here is another proposal: we can make bootstrap config instead of lazy. DI container should execute specified in the bootstrap key method after all definitions are parsed.

@mikehaertl
Copy link

mikehaertl commented Oct 27, 2020

I've never heard "fluent call" before but from Wikipedia, a Fluent interface is more about method chaining, right? This does not necessarily imply immutability. But the use case here is for immutable objects that return a new instance of itself when a method is called. Would immutableCalls be a better alternative?

As a sidenote: all this new immutable stuff really needs good explanation (when to use it, when not, why ...)

@viktorprogger
Copy link
Contributor

But the use case here is for immutable objects that return a new instance of itself when a method is called.

Yes, that's the point.

@yiiliveext
Copy link
Contributor

Bootstrapping is not a scope of the container.

@BoShurik
Copy link

Instead of list decorators it's better to set decorated service. The use case is decorating third party service and in this case it's definition not available.

FYI, lazy in symfony container means the opposite.

@samdark
Copy link
Member

samdark commented Oct 29, 2020

@xepozz I like the idea in general.

fluentCalls: new feature that means that methods will calls in fluent-like style: $newObject = $object->withProp($val)

We already have this feature. Any method that returns the instance of the object is assumed to be a fluent interface call. It works well and I don't think we need a separate section for it. It fits pretty well into calls.

properties: new feature that means that any public (or any, need to think) properties also can be configured just as usual method calls

Again, we already have such feature.

aliases: new feature that means that you can add alias to this definition to use short aliased name instead of full qualified class name (e.g. doctrine.em instead of \Doctrine\Common\EntityManagerInterface)

Currently it's done via separate container entry such as:

MyInteface::class => ...,
'my-interface' => Reference::to(MyInterface::class),

I'm not sure we need another syntax.

lazy: new feature that means that definition must be resolved even when the class is not used in an application (a.k.a. eager definition resolving). For example, it can be used when you need to make the bootstrapping an application

As @yiiliveext mentioned, bootstrapping isn't the job of the container. See yiisoft/app#76

@samdark
Copy link
Member

samdark commented Oct 29, 2020

But I have a question to the properties proposal: do we really need such a feature when any property can be initialized within a constructor parameters or another method call?

@viktorprogger Yes. We do not control what's stored in the container and should not limit these object to be of a certain code style.

@samdark
Copy link
Member

samdark commented Oct 29, 2020

Instead of list decorators it's better to set decorated service. The use case is decorating third party service and in this case it's definition not available.

@BoShurik I assume that's for the feature of automatically generating a lazy proxy class. Something similar to what we do in debug proxies:

@samdark samdark added status:ready for adoption Feel free to implement this issue. and removed status:under discussion labels Feb 1, 2021
@samdark
Copy link
Member

samdark commented Feb 1, 2021

MyClass::class => [
  'class' => MyClass::class,
  'tags' => ['tag1', 'tag2'],
  'construct' => [
    'argument1',
    'argument2',
    'host' => 'host',
    '...variadic' => ['val1', 'val2', 'val3'],
  ],
  'call' => [
    'method1' => 'val1',
    'method2' => 'val2',
    'method3' => 'val3',
    // no need to differentiate fluent calls, we know how to detect these
    'method4' => 'val4',
    'method5' => 'val5',
    'method6' => 'val6',
  ],
  'properties' => [
    'property1' => 'val1',
    'property3' => 'val2',
  ],
  'reset' => fn() => true,
  'aliases' => ['aliased_name1', 'aliased_name2'],
  'decorators' => [
    Decorator1::class,
    Decorator2::class,
    Decorator3::class,
  ],
  'lazy' => false,
]

@hiqsol
Copy link
Member

hiqsol commented Feb 2, 2021

Also, consider having this complex definition as object:

MyClass::class => ComplexDefinition::as([
    'class' => MyClass::class,
    'tags' => ['tag1', 'tag2'],
    ...
]);

OR

MyClass::class => ComplexDefinition::for(MyClass::class)
    ->tags(['tag1', 'tag2'])
    ->construct([...])
    ...
]);

This will allow to develop and try it without breaking BC.

@samdark
Copy link
Member

samdark commented Feb 2, 2021

BC is not a concern yet. There's no release.

@viktorprogger
Copy link
Contributor

@samdark, about your proposal:

  • I'd prefer not to set variadic parameters in a different way. Lets leave them without dot-prefix: we know how to detect them.
  • bootstrapping isn't the job of the container, you said previously about the lazy option. Did something change since then?

@samdark
Copy link
Member

samdark commented Feb 11, 2021

bootstrapping isn't the job of the container, you said previously about the lazy option. Did something change since then?

No, nothing changed.

@viktorprogger
Copy link
Contributor

Your example with the lazy option made me confused

@yiiliveext
Copy link
Contributor

MyClass::class => [
  'class' => MyClass::class,
  'tags' => ['tag1', 'tag2'],
  'construct' => [
    'argument1',
    'argument2',
    'host' => 'host',
    '...variadic' => ['val1', 'val2', 'val3'],
  ],
  'call' => [
    'method1' => 'val1',
    'method2' => 'val2',
    'method3' => 'val3',
    // no need to differentiate fluent calls, we know how to detect these
    'method4' => 'val4',
    'method5' => 'val5',
    'method6' => 'val6',
  ],
  'properties' => [
    'property1' => 'val1',
    'property3' => 'val2',
  ],
  'reset' => fn() => true,
  'aliases' => ['aliased_name1', 'aliased_name2'],
  'decorators' => [
    Decorator1::class,
    Decorator2::class,
    Decorator3::class,
  ],
  'lazy' => false,
]

properties - You lead the user to use public properties instead of setters.
reset - This method doesn't work.
aliases - You lead the user to use the service locator.

@samdark
Copy link
Member

samdark commented Apr 4, 2021

properties are to be mentioned in the guide and also it could be useful if https://wiki.php.net/rfc/property_accessors will be accepted. The the comments about aliases and reset - agree.

@xepozz
Copy link
Member Author

xepozz commented Jul 31, 2021

PR is mentioned above doesn't solve this issue.
Why was this PR merged?

@xepozz xepozz reopened this Jul 31, 2021
@samdark
Copy link
Member

samdark commented Aug 1, 2021

It did solve the issue. We have discussed many variants and came up with a good extendable syntax.

@samdark samdark closed this as completed Aug 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:ready for adoption Feel free to implement this issue. type:enhancement Enhancement
Projects
None yet
Development

No branches or pull requests

8 participants