Compilation enforces coupling to parent class Context contents

[Vinai]

When running bin/magento setup:di:compile the process throws an error if in a class receives an instance of an object as a constructor argument that is already contained in the Context class of the parent.
This enforces coupling custom code to implementation details of the parent classes, however, parent dependencies should be encapsulated.

In my opinion (and I'm sure there are arguments to keep things the way there are, too), enforcing the use of objects from the parent Context increases coupling in a bad way.
Coupling makes code brittle and rigid. Coupled code is more likely to break during changes (e.g. upgrades).
Even though inheritance is a very strong form of source code dependency in itself, all coupling to the parents implementation should ideally be avoided.
Treating the parent class in the same way as an external collaborator object helps to keep the code maintainable.
I think it would be better to display a warning if a class of a third party module access the Context object of a parent, contrary to the current compiler behavior.

Preconditions

A Magento 2 instance (any version afaik) with a custom module.

Steps to reproduce

Create a module containing the following class and run the compiler:

<?php

namespace My\Cool\Controller\Foo;

use Magento\Framework\App\Action\Action;
use Magento\Framework\App\Action\Context;
use Magento\Framework\Controller\Result\RedirectFactory;

class Example extends Action
{
    /**
     * @var RedirectFactory
     */
    private $redirectFactory;

    public function __construct(Context $context, RedirectFactory $redirectFactory)
    {
        parent::__construct($context);
        $this->redirectFactory = $redirectFactory;
    }

    public function execute()
    {
        $result = $this->redirectFactory->create();
        $result->setPath('/');
        return $result;
    }
}

Expected result

The compiler compiles.

Actual result

The compiler raises an error because the RedirectFactory already is contained in the Magento\Framework\App\Action\Context instance.

I suggest removing that check from the compiler. If it is required by the core team, it should be moved into a separate command or tool.

[Ctucker9233]

I've had this problem as well.

[Vinai]

Solution: delete line dev/tests/static/testsuite/Magento/Test/Integrity/Di/CompilerTest.php:104 and \Magento\Framework\Code\Validator\ContextAggregation.

[grafikchaos]

I'm having this problem as well, but only happens when the mode is set to production. Agree with @fooman in #7659 that surprises in production mode are not welcome and @Vinai thank you for the proposed solution.

Has anyone had success with workaround solution? (we installed Magento 2.1.5 via composer)

[maghamed]

This enforces coupling custom code to implementation details of the parent classes, however, parent dependencies should be encapsulated.

Don't consider this as a coupling to a parent class. Actually there is no possibility for coupling, because Magento 2 encourages core developers as well as 3rd party developers to use private access modifiers for class properties (as Inheritance Based API is discouraged).
So, having something initialized and assigned in parent class into private property you lose the possibility to access this property in a child class. There should not be also protected accessors as well, and public accessors violate the encapsulation, so also should be discouraged.
That's why you don't have a valid mechanism in your child class to access a data assigned in parent one. Thus, there is no possibility for coupling.

So, in your specific case. Your child class constructor should look like:

    public function __construct(Context $context)
    {
        parent::__construct($context);
        $this->redirectFactory = $context->getResultRedirectFactory();
    }

That will work, and you no need to pass additional dependency ( RedirectFactory $redirectFactory) into child's class constructor, which in fact already passed in the scope of Context object.

The main idea behind this logic - is that we want to prevent classes from Dependency Hell (when amount of dependencies is constantly growing) and we have @SuppressWarnings(PHPMD.CouplingBetweenObjects) in many classes to make our static tests pass.
Developer has to consider Context as a place to get needed dependency, not just inject new dependency each time if it's needed.

Here is the slide from presentation made by Jisse Reitsma (@jissereitsma on GitHub and twitter) on recent Meet Magento Croatia regarding this issue. Which he considers as a great feature added in Magento 2.1

[grafikchaos]

@maghamed thanks for the explanation and example. I think the bigger issue is what @fooman brought up in #7659 as to why no exception is thrown in developer mode and we only find out there is an issue when the deploy mode is production and bin/magento setup:di:compile is ran?

We are using a community module that has this context problem and is easily solved by the refactoring you provided, but as a module developer I can see how easy it is to overlook where context is passed in and what factories it provides and try to include my own redirect factory.

And the example provided, the $context should be using the getResultRedirectFactory() not getRedirectFactory() as that method doesn't exist in \Magento\Framework\App\Action\Context

public function __construct(Context $context)
{
        parent::__construct($context);
        $this->redirectFactory = $context->getResultRedirectFactory();
}

[Vinai]

@maghamed Coupling to the parent class is not limited to usage of parent class properties, but any knowledge about parent implementation details is coupling.
The context contains the dependencies of the parent class, so any use of the contents of the context object is coupling to implementation details of the parent class.
To demonstrate this, imagine a dependency is added to a parent class via the context in future.
With the context aggregation check in place any class extending from the parent which already is dependent on the same class would cause compilation to fail.
The same is of course true if something is removed from a context class that a child class depends on.
This is a symptom of coupling coupling. The fact that the coupling happens via a context class is irelevent, it still is the same level of coupling.

The only way to reduce coupling is to reduce the knowledge about other classes.
If a child class has dependencies, they should be listed as constructor dependencies, completely independent of the parent class dependencies.
The context object should simply be passed to the parent class as a necessary evil, but should otherwise be ignored.
In fact, I think a validation that throws an exception if any non-abstract class access a context class would be a good idea.

The only way to avoid dependency hell is to create small, encapsulated, decoupled classes.
Avoiding too many dependencies in the constructor by hiding them in a context object is just as bad as injecting the object manager and instantiating them directly.

The context class in fact increases coupling, so it paves the way into dependency hell.

[Vinai]

@maghamed I'm very sorry to see you closed the pull request #8955
I would like to urge you to reconsider and hopefully reopen the PR.
Even with your comment above I fail to understand how the context aggregation validation is helpful in any way.

[Vinai]

I really feel stupid not getting the point.
I'll try to summarize my current understanding of the problem. Please help me understand if I'm wrong.

Arguments for the context aggrigation check I've heard so far are:

1. Reduce the number of constructor dependencies.
2. Avoid multiple instantiations of the same class if it is required by two classes.

I think both those arguments are nought, since 1) only hides dependencies. It doesn't really reduce the number of dependencies. It just tricks phpcs to not see a dependency.
Rather than cheating a phpcs rule, it would be better to change the rule.
And 2) is invalid since injectable objects are shared anyway; the object manager will only instantiate each object once.

My arguments that the check is bad are:
a) It increases coupling between the child and the parent class.
b) It can break the di compilation during deployment as it is very easy to miss during development.
c) it is a predetermined breaking point if the dependencies of a parent class change in future.

To explain argument a):
Even though some coupling is unavoidable, the less coupling there is the more maintainable code tends to be. The more coupling there is the more rigid and fragile a system becomes.
Every piece of knowledge one class has about the implementation of another class is coupling.
Knowledge about a parent classes dependencies is coupling, just like knowledge about protected or private properties or methods.
Public or protected methods are the cleanest kind of coupling, since the implementation of those methods can still change without breaking the child class.
Protected properties and dependencies are a worse kind of coupling since they directly expose implementation details of the parent class.
Forcing a child class to couple itself to the parent class dependencies via the context object increases coupling.

Explanation of b):
When a site is switched to production mode or setup:di:compile is run, the check in question validates that no constructor dependencies are present in the child class that are already contained in the context class.
If there are any, the compilation for the class in question is aborted and an error is displayed, however, the build process as a whole does not fail.
Very few developers take the time to switch to production mode to test their code. Most tests are also not run in production mode. If the build is done in an automated fashion, this can also very easily be missed.
I already have encountered issues in production where plugins interceptors where not "compiled" because of this, and no automated checks caught the problem.
This makes the whole build and validation cycle more fragile and time consuming than it already is.

And finally point c):
Given a child class does takes a dependency from the context class, rather then having it injected in the constructor directly (e.g. the RedirectFactory in an action).
And lets also assume it has it's own dependency injected directly that are not part of the context, for example the PageFactory.
Now, if ever someone decides to add the PageFactory to the context because it is required by the parent class, that will break the child class compilation, even though the child class did not change at all.
Also, if a class contained in the context that is used by a child class is ever removed because the parent no longer requires it, this will also break the code.
This shows that this check indeed is enforcing coupling.

As I've said above, coupling can not be avoided. Otherwise no classes could collaborate. However, coupling can be controlled and minimized. Most importantly, coupling should only be introduced if we as developers get something in return.
This check doesn't give us anything as far as I can tell. It is coupling without a reason.

Maybe I'm missing something here, why it is a good idea to use dependencies from the context class. If so, please explain it to me.
Otherwise I feel as if I'm stupid. Why increase coupling without having to?

[jissereitsma]

Thanks @maghamed for referring to my talk and thanks @Vinai for giving me a ping on this matter. To summarize my standing point: I don't think the concept of $context is a great idea at all. I completely agree with @Vinai saying that using $context is hiding dependencies, making developers that extend from specific Magento parent classes think that they have fewer dependencies than they actually have. I also consider it a mystery why some objects (that might be useful) end up in the $context object while some other objects (that also might be useful) do not. During my talk, the very existence of $context made me use the term dependency hell. I don't consider myself a real system architect, but I had and still have doubt whether the $context thing is a good idea.

In my presentation, I also referred to Magento 2.1 enforcing the usage of this $context object upon other developers, mainly because I personally believed injecting a class X into your own constructor while it is already injected in the $context, was not meant to happen. However, I did not meant this to be interpreted as that duplicating the dependency injection is a bad thing: It has no big performance impact (because the Object Manager uses get() to fetch already instantiated dependencies). However, I think (but I might be mistaken here) that having the $context object maintain a reference to the same instance that you are also injecting in your own constructor does take a bit more memory.

Because of this, I simply took the stand of Magento itself. If Magento comes up with a new thing, we should not be bitching about it: We should either adapt it, or work together to fix or improve it. That being said (and take note that I really don't see myself as a system architect), if a move is made to get rid of $context slowly, I would completely agree. It is an ugly thing. And it makes the move from inheritance to composition harder. This can't be easily done, it should be done in steps instead.

And on that matter, @Vinai makes a solid point that ignoring $context in your own class should be possible, so that you can attempt to make all your own subclasses dependencies visible in your own constructor. So I completely agree with this change in Magento 2.2 to remove this checking whether your own dependency has already been injected in $context.

I'm not sure how this fits in with the plans of Magento to de-couple everything, and their vision on $context. Because of that, I will still recommend (during trainings for instance) newcomers that they should inspect $context first before injecting something on their own. That was my point also during the presentation I gave earlier, which was about using DI. However, in this thread we are talking about improving DI and getting rid of $context checking definitely improves things.

[maghamed]

Thanks, @jissereitsma for joining this discussion.
@Vinai , From the Magento standpoint, $context object is a necessary evil.
We brought the idea of $context into the codebase when we introduced DI concept and turned invisible Dependency Hell (because of Lazy Loading usage) we had in Magento 1 into visible one in Magento 2.
Also, we wanted to prevent increasing of dependencies with some static check, like PHPMD. PHPMD CouplingBetweenObjects metrics has a default value - 13 external dependencies which we didn't want to increase. But because of Magento 1 legacy code relies on the Inheritance Based API and we have several Super God classes like AbstractBlock, AbstractModel, AbstractController, AbstractHelper most of our models and blocks had more than 13 external dependencies just because parent class accepted more than 13.
So, even creating a new Model or Block we didn't have another choice as to mark this class with PHPMD @SuppressWarnings(PHPMD.CouplingBetweenObjects) annotation.

Another reason was Backward Compatibility we wanted to provide for 3rd party developers. When a 3rd party developer extends from Magento core class, his class starts to depend on constructor signature of extended class. Modification of core class constructor signature will break 3PD code. So, Magento core classes, that are intended to be extended (Controller, Block, Model, Helper), should have an abstraction layer, that will allow modification of their constructor signatures after public release in a backward compatible way. Context objects were introduced to address this problem as well.

Here is an example,

Context class implements Magento\Framework\ObjectManager\ContextInterface and has all dependencies of corresponding "extendable" class. Context class instance is passed as a dependency to "extendable" class instance. "Extendable" class retrieves all it's dependencies from context object instead of retrieving them directly through __construct. Only responsibility of context object is to retrieve all dependencies of corresponding "extendable" class and provide access to them through getters.
Now a 3PD extends from \Magento\Framework\View\Element\AbstractBlock and adds some dependencies.
If Magento Core Team refactors abstract block, the list of its dependencies can be easily edited in context object. This modifications will be transparent to 3PD, and their block will stay signature compatible with new abstract block.

So, summarizing all of the above - Usage of Context is a small hack (maybe this hack "smells" a bit) which provides an ability for Magento Core team to make refactoring of Abstract Classes (actually our desire goal is to eliminate God Block, Contoller, Model and Helper classes as soon as we would be allowed to introduce Backward Incompatible Changes).

But we expect Magento 3rd party developers consider Context object as set of dependencies which are injected into parent class. And if you already have specific dependency in the parent class. And taking into account that child's constructor signature should be compatible with parent one. Also taking into account that two different external dependencies on the same instance of some class considered as logical issue.
Why do you need to duplicate this dependency?

[jissereitsma]

Thanks @maghamed for explaining things. It helps us understand why $context was invented in the first place. However, because it smells, refactoring would be a nice thing. It could be refactored in the Magento core, by exchanging the Abstract god-classes (or at least their dependency on $context with better composition structures. Naturally, this will take time, because Magento needs to guarantee the proper workings.

But 3rd party developers should also be allowed to refactor their code - and maybe even earlier than the Magento core changes. So the reason to duplicate the dependencies, is to remove the dependencies in the child class, so that the child class is up-to-date when the core is being refactored. @Vinai his main point was not about making this refactoring of child classes required. However, every 3rd developer should be able to determine whether he/she wants to use the $context object or wants to separate his/her code from these parent dependencies. So indeed, $context still needs to be there in the child class, because the parent class DI needs it. However, there should be no requirement that the child class should actually use the $context. This was @Vinai his point on de-coupling children from their parents.

[orlangur]

@maghamed

The main idea behind this logic - is that we want to prevent classes from Dependency Hell (when amount of dependencies is constantly growing)
That will work, and you no need to pass additional dependency ( RedirectFactory $redirectFactory) into child's class constructor, which in fact already passed in the scope of Context object.
Which he considers as a great feature added in Magento 2.1

Could you please bring more details on which particular problem this "feature" is trying to solve?

Without it if you implement classes in Magento 1-style, your list of dependencies will grow. If you decompose logic into classes properly, list of dependencies WILL NOT grow.

I see the value in such check for core classes, so that constructor signature is not changed until necessary, but for third-party code why enforce legacy-style?

We brought the idea of $context into the codebase when we introduced DI concept and turned invisible Dependency Hell (because of Lazy Loading usage) we had in Magento 1 into visible one in Magento 2.
Also, we wanted to prevent increasing of dependencies with some static check, like PHPMD
So, even creating a new Model or Block we didn't have another choice as to mark this class with PHPMD @SuppressWarnings(PHPMD.CouplingBetweenObjects) annotation.

If I remember correctly, the main reason was to avoid massive constructor signature changes when you change something in super-abstract class. This is NOT related to PHPMD checks for increase of dependencies at all, as usual way to "fix" violation was to add SuppressWarnings annotation. At least I do not recall a single case where such violation would result in some refactoring in order to have smaller classes.

But we expect Magento 3rd party developers consider Context object as set of dependencies which are injected into parent class. And if you already have specific dependency in the parent class. And taking into account that child's constructor signature should be compatible with parent one. Also taking into account that two different external dependencies on the same instance of some class considered as logical issue.
Why do you need to duplicate this dependency?

Vinai had already explained this. We don't want to rely on parent class implementation details. We want to develop classes here and now properly so that we will not have to refactor later, when context object will change in backward-incompatible manner.

I don't understand why inheritance-based approach for customization is generally discouraged but strictly enforced here.

So, to summarize:

• you suggest to implement classes in current Magento 2 core classes manner influenced by a lot of legacy Magento 1 code;
• we want to use approaches better than this, making our implementations prearrenged for interface-based blocks/controllers/models architecture at some point rather than inheritance-based;

[Vinai]

Thanks for the explanation @maghamed, I understand the benefits the context class gives for "extendable" parent classes. Those are valid as long as abstract classes with a __construct method that should be extended exist in the core.

But as I've tried to explain above, enforcing child classes to also use the context object for their dependencies is damaging (especially point c) in my comment above) in regards to the refactorability of the parent dependencies.

It is true that coupling between the child class and the super class already exists.
But the amount of coupling is not binary (that is, coupled or not coupled).

The child class code containing the class name of the context class in the constructor signature so it can pass it on to the parent constructor is one coupling point.
This still a relatively loose coupling point, as it will only cause breakage if the class name of the context class is changed (which I believe is unlikely).

The same is true for the child class coupling to the parent class. By extending it and overriding the constructor there is coupling.

But with every method that is called on the context (or the parent class), or any property or constant that is accessed, the coupling increases, and the prospect for problems when the core classes are changed is increased. The code gets more and more rigid and fragile.

Even though it isn't as intuitive as for example accessing a protected property, the same kind of coupling is introduced by enforcing the child class to use dependencies from the context class, instead of having it's own dependencies injected in the constructor.

If a child class doesn't know anything about the contents of the context class and just passes it to th parent, the context class and the dependencies of the parent class can freely be refactored, as long as the class names aren't changed.

But if a child class access the a dependency of the context class, then that dependency can not be removed from the context class without causing potential breakage in child classes, even if the parent class no longer requires that dependency!
The same is true for adding a new dependency to the context class.
Any child class that needed the same dependency and injected it directly now would no longer be deployable because of the context aggregation validation.

This kind of breakage is the same that is caused by refactoring protected|public methods or properties in a super class.

This is the main reason why I believe the enforcing child classes the use properties from the context class needs to be changed.
The other two reasons I listed above apply never the less, too.

In summary:
Encouraging 3rd party code to NOT use the context class frees the core team to refactor the platform code more freely, it makes the deployment process more stable, and it encourages keeping parent and child classes more decoupled, thus making it easier to refactor once alternatives to the abstract "extendable" classes become available.

For all these reasons I ask you to please reopen and merge the PR #8955.

I suggest investigating implementing the OPPOSITE of the current context aggregation check, that is, showing a warning if a child class access a dependency from an "extendable" class from the context. However, this should only be a warning and not prevent compilation.

[maghamed]

@jissereitsma Of course, refactoring is the only thing which could save us from the "code smell". But for now we can't just eliminate abstract god classes because that will introduce massive Backward Incompatible changes. The stories for elimination of Abstract God classes are at the top of our developer experience backlog and would be implemented as soon as we would be allowed to introduce such a drastic changes. But that will happen not earlier than in 2.3 - 2.4, because in 2.2 all abstract classes would be marked as @api

We make some deprecation on the method level. For example, methods load/save/delete are marked as @deprecated in Magento\Framework\Model\AbstractModel because Magento provides an ability to get the same functionality using "right" way, not violating Single Responsibility.

@Vinai , Inheritance is the strongest coupling which could exist between the objects. Because inheritance is the implementation of "Is-a" relationship. When a successor re-use implementation details of parent using a dedicated protected contract.

If you would like to decouple class A from B - don't use inheritance at all and use composition instead. If you have an inheritance, and business logic uses child class by contract of its parent - your coupling is too high. And in any case you will need to make a complex refactoring when usage of Parent class would be deprecated.
Thus, using inheritance you can't say about loose coupling at all, because your class exposes two contracts instead of one. One contract is Public another is Protected (dedicated for inheritance).

Regarding discouraging references the $context in child classes. If we will imagine that there is no any $context, but all the bunch of dependencies injected straight into the parent class. Should we ban the usage of these dependencies in child classes either? Should child class duplicate dependency which already exists in parent class, just to reduce coupling on parent class?

Most of Dependency Injection containers (like, Unity in C#) implemented in a way which forbids multiple instances of the same type to be injected in specific class. Because that's another "smell" of bad design.

[orlangur]

@maghamed,

Could you please bring more details on which particular problem this "feature" is trying to solve?

"Unity does not allow this" is not related and could not be considered as a reason :) Please answer this question.

Should we ban the usage of these dependencies in child classes either? Should child class duplicate dependency which already exists in parent class, just to reduce coupling on parent class?

For any new dependency such property will be private thus you have to store it in child class constructor. When such property is protected I believe it's less confusing to use parent one, as otherwise you would have to figure out another name for entity already existing within class scope.

Possible rules:

• injecting dependency present in context directly to child class MUST be allowed in third-party code and MUST NOT be allowed in core
• obtaining dependency from context in child class (your example with $this->redirectFactory = $context->getResultRedirectFactory();) SHOULD NOT be allowed in third-party code (not so strict as "MUST", just because sometimes you wanna do it - like, if your class is just a slightly modified version of core one)

Thoughts?

[Vinai]

@maghamed Inheritance is a strong source code coupling, but it by no means is the strongest.
The more details are known to a class the stronger is the coupling.
You can see that by examining what kind of changes would cause the child to break.
Not every change to a parent class will break the child class.

There can be very strongly coupled parent and child classes and there can be very weakly coupled parent and child classes, where the child knows only the parent's class name and nothing else.

If the child access methods and properties and constants (it doesn't matter whether they are public or protected) the coupling increases.
If the child knows about the paren't dependencies the coupling increases, too.

If the parent class is treated like any other dependency and the knowledge the child class has of the parent is kept to a minimum, then the coupling will be much lower, and thus both the child and the parent much simpler and easier to maintain.

This issue now is confusing two things:

• Removing the enforced coupling of the child class with the parent context.
• Getting rid of the context class.

I don't think that getting rid of the context class is a good idea. It is a good workaround as long as inheritance is used.
So lets focus on the first, as that is what this issue originally is addressing.
Removing the enforced coupling can be done without any problems, and that is what this issue (and the related PR) is about.

[maghamed]

@Vinai , getting rid of the context class is definitely in our plans. But we will eliminate contexts in the same major release with decoupling and removal of god super class abstractions for Block, Controller and Model. Because if we will just eliminate contexts that will bring huge backward incompatible changes but we would not get to the desirable state for our code, because next release will bring another BiC changes for the same classes, so 3PD will have to refactor their code twice.

I brought current conversation to Magento Architectural Council meeting yesterday for additional discussion.
There were two options being discussed:

1. Taking into account that potentially adding new dependency to the context object with current behaviour we could break child classes which already have dependency being added to parent one (as you mentioned above). We could @deprecate Context interfaces and forbid inclusion of new dependencies into Context. Thus all new dependencies should be added directly to class which needs the dependency. New dependency should be optional nullable dependency which goes last in the list of constructor arguments. And if the dependency not specified - to instantiate one with the ObjectManager::getInstange static call.
2. Eliminate current check and allow 3PD to include dependencies which already exist in $context object.

It was agreed that current behavior, when exception is thrown at the time of DI compilation is incorrect. Because DI compilation is not considered as a test scenario. So, shouldn't produce such unexpected errors. Moreover it's not a responsibility of compiler to throw such kind of exceptions.
So, better to have this check in a dedicated static test. Which would not use DI compilation.
Also we agreed to allow 3PD to inject dependencies which exist in $context which in fact belong to parent class.
But at the same time we agreed that it's incorrect to request multiple dependencies of the same type for one class. Like here

class A 
{
 public function __construct(B $b, B $b2)
    {
        ..
    }
}

So, this new static test has to check and prevent this situation.

Based on the above can I ask you @Vinai to prepare this static tests and add to existing PR, so we can take and deliver them all together.

[orlangur]

@maghamed,
Nice to hear that such a feature was revised and evaluated as not so "great" after all 👍

But at the same time we agreed that it's incorrect to request multiple dependencies of the same type for one class

Sorry, I may be missing something, but how is that related to initial discussion besides your observation about Unity?

Avoid multiple instantiations of the same class if it is required by two classes.
is invalid since injectable objects are shared anyway; the object manager will only instantiate each object once.

That's a quote from @Vinai and it seems totally valid to me. Even more, when some instance is shared="false" we can ask for two different new objects to be injected. I don't see how injecting two same dependencies can be useful (no matter shared or not), just it seems a bit our of scope for raised issue.

Could you reopen #8955 or it can be reopened by PR author?

Thanks for the quick response and no rigidity ;)

[Vinai]

@maghamed Great news, thank you for the update!

I agree that injecting two instances of the same type is a code smell (even if shared="false").
A factory would be a better solution.
I also think that implementing a static check that checks for duplicate constructor dependencies is a different issue, so I opened issue #9068 for that reason.

Can you please reopen #8955 please? It would be great to get it into 2.2.

[maksek]

Hi @Vinai, i reopened the PR, sorry for delay, it was due Imagine and @maghamed's wedding :)