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

12180 Remove unnecessary use operator for Context, causes 503 error i… #12220

Merged
merged 1 commit into from
Dec 11, 2017
Merged

12180 Remove unnecessary use operator for Context, causes 503 error i… #12220

merged 1 commit into from
Dec 11, 2017

Conversation

chris-pook
Copy link
Contributor

…n account address book.

Removing an unnecessary "use" to prevent a 503 error in account address book page.

Description

Remove the unnecessary "use".

Fixed Issues (if relevant)

  1. M2.2.1 Unable to open Address book after account creation #12180: M2.2.1 Unable to open Address book after account creation

Manual testing scenarios

  • Set up a Magento v2.2.1 environment (prod mode + composer dump-autoload -o)
  • Create an account as a customer.
  • Immediately after creating your customer, try to open your address book.

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Nov 13, 2017

CLA assistant check
All committers have signed the CLA.

@orlangur
Copy link
Contributor

@chris-pook could you please sign CLA?

@chris-pook
Copy link
Contributor Author

CLA signed. Thanks

@@ -8,7 +8,6 @@
use Magento\Customer\Api\AddressMetadataInterface;
use Magento\Customer\Api\AddressMetadataManagementInterface;
use Magento\Customer\Model\Metadata\AttributeResolver;
use Magento\Framework\App\Helper\Context;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @chris-pook, please check a872f40#diff-fe978939bb978f6074e10338e0b14791

I believe the fix is not correct, helper context is supposed to be injected, not customer model context.

At least parent class expects Magento\Framework\App\Helper\Context.

Could you please replace import with Magento\Framework\App\Helper\Context as HelperContext instead and change other places in class accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @orlangur , you've lost me.. Customer\Model\AttributeChecker (where my change is) has no parent class. This change I have made is in Customer\Model\AttributeChecker where there is a use statement for Magento\Framework\App\Helper\Context which is never used in the class. I could alias this to HelperContext but it is not used so better to remove I think? Unless I'm missing something here?

Copy link
Contributor

@orlangur orlangur Dec 5, 2017

Choose a reason for hiding this comment

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

@chris-pook, my bad, I was opening AttributeChecker.php in my IDE and didn't notice I'm checking helper and not model with the same name.

Looks good to me now after your explanation :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good stuff! Thanks

@orlangur
Copy link
Contributor

orlangur commented Dec 4, 2017

Hello @chris-pook, please check my comment #12220 (comment).

@magento-team magento-team merged commit 1afffb3 into magento:2.2-develop Dec 11, 2017
magento-team pushed a commit that referenced this pull request Dec 11, 2017
magento-team pushed a commit that referenced this pull request Dec 11, 2017
[EngCom] Public Pull Requests - 2.2-develop
 - MAGETWO-85311: Added namespace to product videos fotorama events #12469 #991
 - MAGETWO-85300: 8437: Silent error when an email template is not found #970
 - MAGETWO-85293: 12613: Verbiage Update Required: Product Image Watermark size Validation Message. #985
 - MAGETWO-85286: 8176: LinkManagement::getChildren() does not include product visibility. #986
 - MAGETWO-85285: 12482: Sitemap image links in MultiStore #935
 - MAGETWO-84955: Set Current Store from Store Code if isUseStoreInUrl #12529
 - MAGETWO-84764: NewRelic: Disables Module Deployments, Creates new Deploy Marker Command #12477
 - MAGETWO-84439: 12180 Remove unnecessary use operator for Context, causes 503 error i… #12220
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