Skip to content

Replaced usage of ObjectManager with Constructor DI of Factory class #24661

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 26, 2019
Merged

Replaced usage of ObjectManager with Constructor DI of Factory class #24661

merged 1 commit into from
Sep 26, 2019

Conversation

drpayyne
Copy link
Contributor

@drpayyne drpayyne commented Sep 19, 2019 •
edited
Loading

Description

Fixed the improper usage of ObjectManager to get instance of Factory class in the middle of a method, instead of using Constructor Dependency Injection, in Magento_Catalog/Controller/Adminhtml/Category/Add.

  • Removed usage of ObjectManager
  • Injected \Magento\Framework\View\Result\PageFactory through Constructor DI
  • Fixed return type annotation in appropriate PHPDoc Blocks for method and constructor

Fixed Issues

  1. Replacing the usage of ObjectManager in Magento_Catalog Controllers #24646: Replacing the usage of ObjectManager in Magento_Catalog Controllers

Manual testing scenarios

No practical testing, but

  1. Code changes in Magento_Catalog/Controller/Adminhtml/Category/Add as described above

Questions or comments

Bug fix, as usage of ObjectManager directly in code is prohibited.

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)

@drpayyne drpayyne requested a review from akaplya as a code owner September 19, 2019 14:31
@m2-assistant
Copy link

m2-assistant bot commented Sep 19, 2019

Hi @drpayyne. 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 Guide documentation.

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Sep 19, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@drpayyne drpayyne force-pushed the issue-24646 branch 2 times, most recently from fdbf468 to 33733d3 Compare September 19, 2019 15:23
@ihor-sviziev ihor-sviziev self-assigned this Sep 19, 2019
ihor-sviziev
ihor-sviziev requested changes Sep 19, 2019
View reviewed changes
joshuaadickerson
joshuaadickerson suggested changes Sep 19, 2019
View reviewed changes
Copy link

@joshuaadickerson joshuaadickerson left a comment

Choose a reason for hiding this comment

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

Use return types too :)

@drpayyne drpayyne changed the title Replaced usage of Objectmanager with Constructor DI of Factory class Replaced usage of ObjectManager with Constructor DI of Factory class Sep 19, 2019
@ghost
Copy link

ghost commented Sep 20, 2019

@drpayyne unfortunately, only members of the maintainers team are allowed to remove progress related labels to the pull request

ihor-sviziev
ihor-sviziev requested changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

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

Hi @drpayyne,
Please review my comments, once you'll finish - please squash all your changes to single commit & force push them.

Thank you!

@ihor-sviziev ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Progress: needs update and removed Progress: ready for testing Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests labels Sep 20, 2019
@drpayyne drpayyne requested a review from orlangur September 24, 2019 11:39
ihor-sviziev
ihor-sviziev reviewed Sep 24, 2019
View reviewed changes
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-5909 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa engcom-Alfa self-assigned this Sep 25, 2019
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@engcom-Foxtrot engcom-Foxtrot self-assigned this Sep 25, 2019
@engcom-Foxtrot engcom-Foxtrot added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Sep 25, 2019
@magento-engcom-team magento-engcom-team mentioned this pull request Sep 26, 2019
Closed
magento-engcom-team pushed a commit that referenced this pull request Sep 26, 2019
@sidolov
ENGCOM-5909: Replaced usage of ObjectManager with Constructor DI of F…
1e07346 
…actory class #24661
@magento-engcom-team magento-engcom-team merged commit 2b5b739 into magento:2.3-develop Sep 26, 2019
@m2-assistant
Copy link

m2-assistant bot commented Sep 26, 2019

Hi @drpayyne, 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 Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orlangur orlangur orlangur approved these changes

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

+2 more reviewers

@joshua-bn joshua-bn joshua-bn left review comments

@joshuaadickerson joshuaadickerson joshuaadickerson approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@orlangur orlangur

@ihor-sviziev ihor-sviziev

@engcom-Alfa engcom-Alfa

@engcom-Foxtrot engcom-Foxtrot

Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Component: Catalog Partner: EY partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.3
Projects
None yet
Milestone
Release: 2.3.4
Development

Successfully merging this pull request may close these issues.
9 participants
@drpayyne @magento-cicd2 @magento-engcom-team @engcom-Alfa @joshuaadickerson @orlangur @ihor-sviziev @joshua-bn @engcom-Foxtrot