Skip to content

Use class name as type hint instead of self. #23300

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

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

dcabrejas
Copy link

Description (*)

Use class name as type hint instead of self. self breaks code generator when a preference is defined.

Fixed Issues (if relevant)

  1. Creating a preference for category product indexer breaks setup:di:compile #22769: "Creating a preference for category product indexer breaks setup:di:compile"

Manual testing scenarios (*)

  1. Create a preference for \Magento\Catalog\Model\Indexer\Category\Product\Action\Full in a custom module
  2. Run bin/magento setup:di:compile
  3. There is no PHP fatal error anymore.

Questions or comments

I am going to try and work out a solution to the real problem here which should allow us to still use the self keyword and use preferences/code generation at the same time. That would be in a different PR though.

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Jun 18, 2019

Hi @dcabrejas. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

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

@magento-engcom-team
Copy link
Contributor

Hi @aleron75, thank you for the review.
ENGCOM-5309 has been created to process this Pull Request
✳️ @aleron75, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@magento-engcom-team
Copy link
Contributor

@dcabrejas thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is perfectly valid, interception mechanism must be fixed instead.

@dcabrejas
Copy link
Author

Hi @orlangur

Please see my comment on the PR description about this being just a fix to the issue users have reported. While I agree more thought is required to provide a generic fix that allows code to typehint self as a return parameter. I believe that can be done as a separate PR.

@orlangur
Copy link
Contributor

@dcabrejas sure.

That would be in a different PR though.

Please work on such PR and this change is not needed actually.

@orlangur orlangur closed this Jun 21, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jun 21, 2019

Hi @dcabrejas, 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.

@dcabrejas
Copy link
Author

Hi @sivaschenko

What is your opinion about this? We discussed this at meet Magento and it was decided this should be fixed in this way.

@dcabrejas dcabrejas reopened this Jun 21, 2019
@ghost ghost unassigned orlangur, aleron75 and engcom-Alfa Jun 21, 2019
@orlangur orlangur self-assigned this Jun 21, 2019
@orlangur
Copy link
Contributor

Similar poor attempt of "fix" was performed in #21775 (comment).

Again: indexer code MUST NOT be changed, interception mechanism is what actually needs to be fixed.

@DrewML
Copy link

DrewML commented Jun 21, 2019

Technical disagreements are fine, but suggesting that another contributor's work is a waste of time is completely out of line.

@orlangur
Copy link
Contributor

@DrewML, sorry, I wasn't specific enough and my English may sound not how it was intended to sound to native speaker. I just tried to guide contributor in a right direction to avoid waste of efforts by doing a temporary fix, then a proper one, then reverting initial unneeded change. This can be even a bigger trouble if release occur between such connected pull requests. #23300 (comment) is a good example where we faced with similar changes and elaborated a more universal fix.

I do appreciate every contributor work and trying to help to not waste contributor efforts on changes which could be avoided, this is what I meant by "our wasted time" - we are all on the same boat, there is no distinction between contributors, maintainers or even EngCom team members on this aspect.

Thanks for pointing out, I'll take an additional look into Code of Conduct and do my best to avoid such situations in the future.

@dcabrejas sorry that I didn't pick the right words to convey my point and could decrease your enjoyment of contributing to Magento Open Source. I'm taking part in the Magento Contribution Day today and will reach out to @sivaschenko directly.

@sivaschenko
Copy link
Member

@orlangur we have discussed this issue with @dcabrejas at the Meet Magento UK contribution day. As there is no proved solution for generator fix, and such fix may require a substantial amount of time to implement and tests, we decided to apply a local fix to this file and postpone the generator fix.

In case a pull request will be created, tested and delivered within the current release with the fix described in #23300 (comment) - that solution looks more preferable for me.

@orlangur
Copy link
Contributor

@sivaschenko thanks, seems we are on the same page taking into consideration there is a bunch of time till 2.3.3 contributions freeze. I'll try the patch in action.

@ghost ghost assigned sidolov Aug 14, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sidolov, thank you for the review.
ENGCOM-5624 has been created to process this Pull Request
✳️ @sidolov, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@YevSent
Copy link
Contributor

YevSent commented Aug 15, 2019

As @orlangur mentioned, the fix should be on the code generation level (internal ticket MC-17251) but should not change Magento\Catalog\Model\Indexer\Category\Product\Action\Full as it's a valid class.

Another note, preferences should not be used for classes, only for interfaces.

@m2-assistant
Copy link

m2-assistant bot commented Aug 27, 2019

Hi @dcabrejas, 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.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Aug 27, 2019
@VladimirZaets VladimirZaets added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Catalog Partner: JH partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants