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

Compiled DI and Developer DI Are Sometimes Incompatible #6739

Closed
maxbucknell opened this issue Sep 24, 2016 · 8 comments
Closed

Compiled DI and Developer DI Are Sometimes Incompatible #6739

maxbucknell opened this issue Sep 24, 2016 · 8 comments
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development

Comments

@maxbucknell
Copy link
Contributor

Preconditions

I have found that there are some cases where classes will be introspected by Reflection in the Developer mode, but will not be caught by the setup:di:compile command. This increases the testing burden of developers, and produces some very difficult to understand error messages.

As far as I know, this affects every version of Magento 2.0 and 2.1, but I was using 2.0.8 at the time. The environment is modern and following all best practices, and was replicated across a variety of environments.

Steps to reproduce

  1. Inject an automatically generated factory of a non-injectable class that is not part of a Magento module. We were using the Facebook\Facebook class from the facebook/graph-sdk Composer package.

  2. Run the code with the Developer Mode DI, and it will generate the factory, using the Object Manager. The Object Manager will dynamically use Reflection to inspect which arguments are required, and match them all up. It works like a dream.

  3. Run the code with the Compiled DI, and it generates the factory, and calls the Object Manager in the same way. However, the Facebook\Facebook class is not in the compiled DI information, and the Object Manager will pass in the default constructor arguments, which is just the Object Manager.

  4. The result is some kind of type error. In my case, it was:

    Uncaught TypeError: Argument 1 passed to Facebook\Facebook::__construct()
    must be of the type array, object given```
    

Expected result

I'm not 100% sure what the correct thing to do here is. If we accept that the behaviour should at least be consistent, our options are to prevent the Developer DI from working with external classes (which I don't like), or to enhance the Compiled DI so that it works with external classes that are referred to through generated classes (I like this one more).

A compromise might be to document it well, or something.

Fix

For anyone stumbling across this issue, it can be fixed by adding an empty class to your module, and having it extend your external class. Tell Anton Kril that I found a use for inheritance.

If you generate a factory for this class in your module, it will find the parent constructor arguments without issue. You can add a preference into di.xml for Facebook\Facebook, to be My\Module\Model\Facebook, and everything else just falls into place.

@antonkril
Copy link
Contributor

Nice try, but composition would work here too ;)

We discussed this issue, and the resolution is to add fallback to reflection in compiled mode.

@kandy
Copy link
Contributor

kandy commented Nov 12, 2016

Internal ticket to track MAGETWO-51176

@maxbucknell
Copy link
Contributor Author

Can I have a status update on this? If it hasn't been worked on, I'd like to take it up.

CC @maksek

@igortregub
Copy link

igortregub commented Jun 12, 2017

I've had the same issue. Have you any updates about it?

@attilai
Copy link

attilai commented Jul 3, 2017

Same issue

@mcspronko
Copy link

mcspronko commented Aug 23, 2017

@antonkril Can you please tell me how composition will help to solve the issue? I am having class A where one argument into __construct() method is passed. I use this class A to inject into MagentoClass B and having same error. Thanks for advice.

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development bug report Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@maghamed
Copy link
Contributor

maghamed commented Sep 14, 2017

@mcspronko @maxbucknell

We made a fix in 2.2.0 for this case
8498e12
(lib/internal/Magento/Framework/ObjectManager/Config/Compiled.php)

but we still state that Compiler may not be able to resolve all dependencies on 3rd party classes. That’s why 3rd party classes should not be injected in the constructor of Magento classes, instead, they should be instantiated via new operator, preferably in adapters only.

Third party (3P) libs don't mean third party libraries that are actually registered with ComponentRegistrar as components of type "library," it means generic php libraries that are not special Magento application components. So this is about unregistered 3P libraries.

@magento-engcom-team
Copy link
Contributor

@maxbucknell, thank you for your report.
The issue is already fixed in 2.2.0

@magento-engcom-team magento-engcom-team added 2.0.x Fixed in 2.2.x The issue has been fixed in 2.2 release line labels Sep 20, 2017
mmansoor-magento pushed a commit that referenced this issue May 11, 2021
[Arrows] MC-41638: [Paypal Express Checkout] Can't place an order
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development
Projects
None yet
Development

No branches or pull requests

9 participants