Dependencies injected to properties #32

AlexMaxHorkun · 2018-10-01T10:51:02Z

Allows backward compatible introduction of new dependencies to classes without having to use ObjectManager directly.
Allows circular dependencies.

Allows backward compatible introduction of new dependencies to classes without having to use ObjectManager directly. Allows circular dependencies.

navarr · 2018-10-02T12:43:18Z

Would this make dependency properties that need such a thing public properties?

AlexMaxHorkun · 2018-10-02T12:54:00Z

Would this make dependency properties that need such a thing public properties?

Those properties would remain private/protected, if that's what you mean

kandy · 2018-10-02T13:44:29Z

design-documents/dependencies-injected-in-properties.md

@@ -0,0 +1,34 @@
+## Introduce the possibility to inject dependencies directly to properties
+### Reasons
+* Allowing backward compatible introduction of new dependencies to classes without having to use ObjectManager directly  


Can you describe the issue with adding optional arguments?

As I've stated - because of having to use ObjectManager::getInstance(). Do you want me to describe why is it a bad thing to use object manager's static instance?

No, I mean that if the problem in static dependency on OB, better to propose to ask for OM in the constructor explicitly and it definitely makes sense for me

kandy · 2018-10-02T13:45:00Z

design-documents/dependencies-injected-in-properties.md

+* Allowing backward compatible introduction of new dependencies to classes without having to use ObjectManager directly  
+&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;Currently we have to add optional arguments to classes and invoke ObjectManager::getInstance() to get a dependency
+for cases when the constructor is invoked from a child class. Declaring such dependencies for properties will allow us to avoid this.
+* Allowing circular dependencies  


This contradicts with Magento Technical Guidelines
3.1. There SHOULD be no circular dependencies between object.

Ok, I'll remove this part

kandy · 2018-10-02T13:47:29Z

design-documents/dependencies-injected-in-properties.md

+after requested object instances are created we can make circular dependecies possible.
+### Details
+#### XML
+We'd have to add *properties*/*property* elements for *type* elements in di.xml's.


Can I ask you to provide an example of XML declaration?

kandy · 2018-10-02T13:50:42Z

design-documents/dependencies-injected-in-properties.md

+*property* element would have *type* and *shared* attributes just as *argument* does.  
+#### Injection
+Key points:  
+* Dependencies list for properties will be collected and injected after


This contradicts with Magento Technical Guidelines
2.2. Object MUST be ready for use after instantiation. No additional public initialization methods are allowed.
2.14. Temporal coupling MUST be avoided

No it doesn't. Classes requesting objects from object manager won't get them before they initiated. This is just details regarding implementation of injecting dependencies to properties.

To be clear - I'm not suggesting we create setters for these properties or make them public. Objects will be fully initiated after the object manager returns them

You cannot create such an object with the "new" operator and use it without additional initialization and we don't have the requirement to create all objects using DI. Actually, we have opposite requirement: ObjectManager MUST NOT be used in unit tests.

kandy · 2018-10-02T13:52:10Z

design-documents/dependencies-injected-in-properties.md

+Key points:  
+* Dependencies list for properties will be collected and injected after
+the requested object's instance is created and added to the shared map (if it's a singleton) to allow circular dependencies
+* A dependency will __NOT__ be inject into a property if it's not equal to *null*


I believe it unexpected behavior. When I pass arguments to constructor I will expect there will be used.

How is it contradicting with what I've written?

Maybe I don't get what you are proposed here.
For me it looks like:

if I will write code like:
$om->create('MyClass', ['MyClass::property_a' => 123]);
and class object has already initialised in same constriuctor, the value "123" will not be used

kandy · 2018-10-02T13:58:58Z

design-documents/dependencies-injected-in-properties.md

+* In order to preserve backward compatibility we can't add an argument to ObjectManagerInterface::create(),
+so to distinguish values passed in $arguments as property dependencies I propose we name them as *ClassName*::*property*
+#### Using ObjectManager::getInstance()
+* Mark it as deprecated because the only sensible use-case was introduction of new dependencies to classes.


There are other use cases. Better to discuss this separately

Ok, if I'm missing some use cases it would be good to know

kandy · 2018-10-02T14:01:11Z

ObjectManager is critical for performance point of view. So, we need also consider the impact on performance

Vinai · 2018-10-03T09:54:57Z

From my point of view the introduction of more reflection "magic" is worse than the use of the static ObjectManager::getInstance().
The current way to add dependencies as optional arguments to a constructor signature also makes the technical dept visible, something that would be lost by this proposal.
Keeping things simple has many of advantages, such as increased compatibility with future PHP versions, lower cognitive load when reading the code, and an easier time when the code needs to be changed in the future.

I believe it's a common misconception that the use of all static method calls should be considered bad.

Lets discuss the case of ObjectManager::getInstance()

The first downside of using the ObjectManager is that it hides class dependencies from human readers and static code analysis. However, this does not apply when the ObjectManager is used in a constructor to instantiate missing dependencies for backward compatibility because the dependency is still part of the constructor signature.

The second downside of using the ObjectManager is that it makes unit testing harder. Again, this does not apply when the ObjectManager is used in a constructor to instantiate missing dependencies for backward compatibility, because the dependency can be passed to the constructor when new is called in the unit test.

To summarize, I think the proposal is trying to solve a non-problem and accepting it would be harmful as it would introduce unneeded complexity.

AlexMaxHorkun · 2018-10-04T13:19:00Z

I also think it is better to have a single place of reference - we have to allow setting up dependencies via XML for 3rd party devs to extend, and without having to write a constructor (which can be generated by object manager if we choose to accept this PR) the di.xml's would be the single point of reference.

paliarush

Agree with @Vinai, need stronger reasoning to introduce a feature which will complicate the framework
Not clear how the configuration would look like
Please provide analysis of dev experience changes with the new approach for core and extension developers

AlexMaxHorkun · 2018-10-08T09:49:12Z

Reasons to introduce this:

avoid breaking Inversion of Control by having to hard-code usage of a specific DI manager instance
When classes themselves control which DI manager instance they use to get their dependencies it prevents us from controlling DI manager instance from outside and using multiple ObjectManager instances in case we want to write a daemon
inability to chose a non-default preference for an interface
When we call ObjectManager:;getInstance()->get() we can only receive a default preference ffor the requested class and cannot control it via di.xml
possibility to leave only 1 source of dependencies config for a class (di.xml)
Right now we have 2 sources of DI config for a class - it's constructor and di.xml element. We can't get a full picture by just looking at a constructor because we can only specify a non-default preference for a dependency in di.xml. By injecting dependencies to properties we can avoid writing constructors and will be able to treat di.xml as the only source of dependencies configuration.

AlexMaxHorkun · 2018-10-08T09:50:52Z

How di.xml would look like:

<type name="Magento\SomeModule\SomeClass">
    <properties>
        <property name="dep" xsi:type="objecty">Magento\SomeModule\SomeOtherClass</property>
    </properties>
</type>

AlexMaxHorkun · 2018-10-08T10:05:52Z

Dev experience:

Magento dev adding a new dependency to a class

Instead of adding a new optional (not really) parameter to the constructor

public function __construct(Type1 $existing, Type2 $newOne = null) {
    $this->existing = $existing;
    $this->newOne = ObjectManager::getInstance()->get(Type2::class) ?? $newOne;
}

you just add a property element to the class' di config (di.xml):

<type name="Magento\SomeModule\SomeClass">
    <properties>
        <property name="newOne" xsi:type="object">Magento\SomeModule\Type2</property>
    </properties>
</type>

Magento dev creating a new class

Instead of defining all the properties and then writing a redundant constructor you just add a type element to the module's di.xml:

class NewMageClass implements SomeInterface
{
    /**
     * @var DepClass1
     */
    private $dep1;

    /**
     * @var DepClass2
     */
    private $dep2;

    /**
     * Do smth
     */
    public function doSmth(): void
    {
        $this->dep1->doSmthElse($this->dep2->getSmth());
    }
}

di.xml:

<type name="Magento\SomeModule\NewMageClass">
    <properties>
        <property name="dep1" xsi:type="object">Magento\SomeModule\DepClass1</property>
        <property name="dep2" xsi:type="object">Magento\SomeModule\DepClass2</property>
    </properties>
</type>

3rd party dev looking up a class' dependencies

Just find the module's di.xml and type element with attr name=TheClass

thomas-kl1 · 2018-10-08T10:12:16Z

The ObjectManager should not touch the constructor behavior of a class. It shouldn't even know how the class works. His ONLY job is to pass values to the constructor on instanciation. Nothing else.

Vinai · 2018-10-08T12:46:56Z

@AlexMaxHorkun

Reasons to introduce this:
*avoid breaking Inversion of Control by having to hard-code usage of a specific DI manager instance
When classes themselves control which DI manager instance they use to get their dependencies it prevents us from controlling DI manager instance from outside and using multiple ObjectManager instances in case we want to write a daemon

Needing to use a different DIC is in my estimate a fringe case that probably would only effect < 1% of all instances. All others would suffer the introduced complexity, too.
On a side note, it is perfectly possible to use the Magento DI container to write deamons. I don't understand how that would introduce the requirement for another DIC.

inability to chose a non-default preference for an interface
When we call ObjectManager:;getInstance()->get() we can only receive a default preference ffor the requested class and cannot control it via di.xml

That is false. Because the "optional" dependencies are part of the constructor signature, too, even though they default to null, so it is absolutely possible to configure type level arguments in di.xml.

possibility to leave only 1 source of dependencies config for a class (di.xml)
Right now we have 2 sources of DI config for a class - it's constructor and di.xml element. We can't > get a full picture by just looking at a constructor because we can only specify a non-default preference for a dependency in di.xml. By injecting dependencies to properties we can avoid writing
constructors and will be able to treat di.xml as the only source of dependencies configuration.

The constructor is the only source to get the full picture. In fact, the whole point of constructor injection is type safety. With this proposal any type could be injected via di.xml. This is no better than using arrays to pass in options, but without the nice things arrays.

AlexMaxHorkun · 2018-10-08T15:09:49Z

Writing a daemon may require multiple object managers for isolation because our code is designed to be called from a script that dies, but by employing an object manager per message we can write daemon safely without putting additional requirements for our code

AlexMaxHorkun · 2018-10-08T15:14:09Z

The problem with inability to specify a specific preference for an optional argument added to a class:
if a new dependency is added to a class via an optional argument to the constructor and we have to use a more specific type in di.xml and some 3rd party dev have extended the class before the argument was added they would be calling parent constructor without the new argument passed, so ObjectManager::getInstance()->get() would be called inside the class and only the default preference for the dependency received

AlexMaxHorkun · 2018-10-08T15:16:18Z

Currently the constructor is not the only source of dependencies config - more specific types can be stated in di.xml and scalar values can only be passed with di.xml (including arrays of objects for Composite classes)

paliarush · 2018-10-08T15:37:22Z

Just find the module's di.xml and type element with attr name=TheClass

Because of backward compatibility and lack of resources for refactoring we will not be able to migrate to property injection completely. It will become yet another way of doing the same thing and developers will have to look in one more place when trying to understand what instance is actually injected. It may work for green field projects, but for Magento does not look like a good idea.

I believe the benefits of the proposal do not justify increased complexity and deterioration of dev experience.

melnikovi · 2018-10-16T15:05:23Z

I agree with previous responders on

Benefits of type checks in the constructor and constructor being source of truths for types
Benefits of having object ready after instantiation
That this approach introduces complexity and another way of doing the same thing

I think dependency on the object manager is not a big problem because it is used only when you don't pass dependency and should be eliminated in the future anyway.

I agree that there is a problem that class can't be configured via di with different dependency. Maybe we should just create another method for this?

Dependencies injected to properties

dbed07e

Allows backward compatible introduction of new dependencies to classes without having to use ObjectManager directly. Allows circular dependencies.

kandy self-requested a review October 2, 2018 13:37

kandy suggested changes Oct 2, 2018

View reviewed changes

melnikovi requested review from paliarush and melnikovi October 3, 2018 16:12

paliarush requested changes Oct 4, 2018

View reviewed changes

melnikovi closed this Oct 18, 2018

Dependencies injected to properties #32

Dependencies injected to properties #32

Uh oh!

Conversation

AlexMaxHorkun commented Oct 1, 2018

Uh oh!

navarr commented Oct 2, 2018

Uh oh!

AlexMaxHorkun commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kandy commented Oct 2, 2018

Uh oh!

Vinai commented Oct 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexMaxHorkun commented Oct 4, 2018

Uh oh!

paliarush left a comment

Choose a reason for hiding this comment

Uh oh!

AlexMaxHorkun commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexMaxHorkun commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlexMaxHorkun commented Oct 8, 2018

Magento dev adding a new dependency to a class

Magento dev creating a new class

3rd party dev looking up a class' dependencies

Uh oh!

thomas-kl1 commented Oct 8, 2018

Uh oh!

Vinai commented Oct 8, 2018

Uh oh!

AlexMaxHorkun commented Oct 8, 2018

Uh oh!

AlexMaxHorkun commented Oct 8, 2018

Uh oh!

AlexMaxHorkun commented Oct 8, 2018

Uh oh!

paliarush commented Oct 8, 2018

Uh oh!

melnikovi commented Oct 16, 2018

Uh oh!

Uh oh!

Vinai commented Oct 3, 2018 •

edited

Loading

AlexMaxHorkun commented Oct 8, 2018 •

edited

Loading

AlexMaxHorkun commented Oct 8, 2018 •

edited

Loading