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

FIX for issue #21916 - Elasticsearch6 generation does not exist #22046

Conversation

phoenix128
Copy link
Contributor

@phoenix128 phoenix128 commented Mar 29, 2019

Magento was not checking concrete class while applying plugins

Description (*)

If the pluginized class was virtualtyped by a class with name ending with an autogenerated suffix and without the implementation of the base concrete class, the di compile was failing.

This PR prevents code generation for VirtualTypes.

Fixed Issues

  1. Elasticsearch6 generation does not exist #21916: Elasticsearch6 generation does not exist
  2. Magento doesn't work after upgrade from 2.3.0 to 2.3.1 #21976: Magento doesn't work after upgrade from 2.3.0 to 2.3.1

Additional

  • Added test coverage for autogeneration on virtual types
  • Fixed too broad tests check not verifying the successful exit of generateClass
  • Small test codestyle adjustments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@m2-assistant
Copy link

m2-assistant bot commented Mar 29, 2019

Hi @phoenix128. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@phoenix128 phoenix128 changed the title FIX for issue #21916 - Elasticsearch6 generation does not exist [WIP] FIX for issue #21916 - Elasticsearch6 generation does not exist Mar 29, 2019
@ghost ghost assigned rogyar Mar 31, 2019
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-4620 has been created to process this Pull Request

@hostep
Copy link
Contributor

hostep commented Apr 1, 2019

Hi @phoenix128

I'm currently upgrading a shop of ours to Magento 2.3.1, this shop has the particular Amasty Shopby module installed, currently version 2.11.1 (not updated to 2.11.2 yet which is supposed to fix this problem, even though that's probably a workaround?).

I then tested this fix you are proposing, but I'm still seeing the same error:

$ bin/magento setup:di:compile
Compilation was started.
Interceptors generation... 4/7 [================>-----------]  57% 10 secs 124.0 MiB
In Generator.php line 218:

  Source class "\Magento\Elasticsearch6\Model\Adapter\FieldMapper\ProductField" for "Magento\Elasticsearch6\Model\Adapter\FieldMapper\ProductFieldMapper" generation does not exist.


setup:di:compile

I thought we didn't need to upgrade the Amasty module and only apply this fix as a patch to circumvent the problem, but that doesn't seem to be the case?

If you need more info from me for this particular problem, I will gladly provide it :)

@phoenix128
Copy link
Contributor Author

@hostep, @rogyar , this is a WIP PR, the issue is divided in two different phases and I only fixed the first one. I am almost finished.

@phoenix128 phoenix128 force-pushed the issue-21916-elasticsearch6-generation-does-not-exist branch from cd2affa to b06ead6 Compare April 8, 2019 17:03
Magento was not checking concrete class while applying plugins, this commit prevents virtual types to be autogenerated
@phoenix128 phoenix128 force-pushed the issue-21916-elasticsearch6-generation-does-not-exist branch from b06ead6 to fa079c9 Compare April 8, 2019 17:11
@phoenix128 phoenix128 changed the title [WIP] FIX for issue #21916 - Elasticsearch6 generation does not exist FIX for issue #21916 - Elasticsearch6 generation does not exist Apr 8, 2019
@hostep
Copy link
Contributor

hostep commented Apr 8, 2019

Nice one @phoenix128!

I can confirm this fixes the problem with the amasty/shopby module (version 2.11.1).

It also looks like setup:di:compile runs a couple of seconds faster due to this change, so that's also nice.

Just an observation, not sure if this is a problem or not, but before this change, the following files got generated, and after the change this no longer happens (tested on Magento OS 2.3.1 - developer mode):

  • generated/code/Magento/MysqlMq/Model/Driver/Bulk/ExchangeFactory.php
  • generated/code/Magento/Ui/Config/Converter/StorageConfig/Proxy.php
  • generated/code/Magento/Ui/Config/Converter/Buttons/Proxy.php
  • generated/code/Magento/Ui/Config/Converter/Item/Proxy.php
  • generated/code/Magento/Ui/Config/Converter/Actions/Proxy.php

@phoenix128
Copy link
Contributor Author

phoenix128 commented Apr 9, 2019

Hello @hostep,
I know and I expected this, IMO the previous behaviour was wrong.

If you take for example Magento\Ui\Config\Converter\Item\Proxy you will see it is a virtual type defined in Magento/Ui/etc/di.xml.

So, since my patch prevents the code generation of VirtualTypes ending with a generation suffix (Proxy in our case), this should be the expected behaviour.

I would like some Magento architect to confirm this aspect anyway.

@phoenix128
Copy link
Contributor Author

I added test coverage for virtual types code generation.
I also fixed a couple of too broad tests not checking the exit status of generateClass method.

Seems like we also had a small performance boost while DI compiling as a side effect.

@phoenix128 phoenix128 force-pushed the issue-21916-elasticsearch6-generation-does-not-exist branch from f4bda3b to 75684ca Compare April 9, 2019 16:45
@phoenix128 phoenix128 requested a review from maghamed April 9, 2019 16:48
@melnikovi melnikovi self-requested a review April 9, 2019 22:08
@nmalevanec nmalevanec force-pushed the issue-21916-elasticsearch6-generation-does-not-exist branch from c5f587a to b8ab61b Compare April 11, 2019 14:36
@m2-assistant
Copy link

m2-assistant bot commented Apr 15, 2019

Hi @phoenix128, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants