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.