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

[BUGFIX] Made method public so a plugin is possible. #11878

Conversation

ghost
Copy link

@ghost ghost commented Oct 30, 2017

Customer isConfirmationRequired method exists in two places. In the customer Object and in the accountManagement object

The method in the customer object is public. Which makes it possible to plugin it.
The method in the accountManagement object is protected. Which makes it impossible to plugin it.

This PR only makes the method in isConfirmationRequired in AccountManagement class public.

  • This makes it possible to develop custom business logic to decide if confirmation is required yes/no for certain customers.
  • This makes it possible to implement a soft confirmation

Fixed Issues (if relevant)

  • Can plugin the isConfirmationRequired method in AccountManagement

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@orlangur orlangur self-assigned this Oct 30, 2017
@orlangur orlangur added this to the October 2017 milestone Oct 30, 2017
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@omiroshnichenko omiroshnichenko self-assigned this Nov 24, 2017
@omiroshnichenko
Copy link
Contributor

omiroshnichenko commented Nov 24, 2017

Hi @dheesbeen. Change of method from protected to public leads to backward compatibility issue. To solve you problem you can create new class that will contain isConfirmationRequired method, that will be public, inject this class into AccountManagement and Customer classes, replace all places where old methods used and make deprecation of old methods.

@orlangur
Copy link
Contributor

inject this class into AccountManagement and Customer classes, replace all places where old methods used and make deprecation of old methods.

... or just use this new class inside of isConfirmationRequired methods

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…us-2.2-patch-customer-accountmanagement-is-confirmation-required
@omiroshnichenko omiroshnichenko force-pushed the experius-2.2-patch-customer-accountmanagement-is-confirmation-required branch 4 times, most recently from b43de35 to 7588df9 Compare December 5, 2017 13:47

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@omiroshnichenko omiroshnichenko force-pushed the experius-2.2-patch-customer-accountmanagement-is-confirmation-required branch from 7588df9 to 6227a14 Compare December 5, 2017 14:11
@omiroshnichenko
Copy link
Contributor

Hi, @dheesbeen. What is a use case to make plugin on isConfirmationRequired method? What goal you want to achieve?

@orlangur
Copy link
Contributor

orlangur commented Dec 5, 2017

@omiroshnichenko

This makes it possible to develop custom business logic to decide if confirmation is required yes/no for certain customers.

It seems pretty clear to me. Logic like

if (customer corresponds to some conditions) {
    skip confirmation;
}

Probably it's possible with existing code by some setSkipConfirmation?

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…us-2.2-patch-customer-accountmanagement-is-confirmation-required

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
…us-2.2-patch-customer-accountmanagement-is-confirmation-required
@magento-team magento-team merged commit 880dd9f into magento:2.2-develop Dec 22, 2017
magento-team pushed a commit that referenced this pull request Dec 22, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
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.

None yet

4 participants