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

Ease CustomerAddressForm customization #9440

Merged
merged 1 commit into from Aug 24, 2018

Conversation

Projects
None yet
6 participants
@mickaelandrieu
Contributor

mickaelandrieu commented Aug 13, 2018

Questions Answers
Branch? develop
Description? Demonstrates how native classes may be open for extension but closed for modification.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? At some point, @azisyus may share his needs.

This change is Reviewable

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

Hello @azisyus,

can you confirm this update is enough to allow you to change the behavior of the submit action?

Regards,

Contributor

mickaelandrieu commented Aug 14, 2018

Hello @azisyus,

can you confirm this update is enough to allow you to change the behavior of the submit action?

Regards,

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

ping @azisyus,

would you mind to tell us if it helps you or not?

In all cases, I'll remove public access to the form properties.

Mickaël

Contributor

mickaelandrieu commented Aug 16, 2018

ping @azisyus,

would you mind to tell us if it helps you or not?

In all cases, I'll remove public access to the form properties.

Mickaël

@azisyus

This comment has been minimized.

Show comment
Hide comment
@azisyus

azisyus Aug 17, 2018

Contributor

@mickaelandrieu sorry for delay, as you said it helps with hook and new get set methods, adding get set methods instead of changing access modifiers probably always better solution and i think this PR proves necessity of hooks because we cant override everything but also overrides fixes things fast and makes happy customer as you can understand we cant wait for new versions to fix our personal needs and also thats why you should remove override feature otherwise many php guy will use this feature hardly while writing module, i was using because i saw it in your documents, first of all you need to remove from docs then it will slowly disappear and when the times comes you just remove the overrides and everything will be fine.

Contributor

azisyus commented Aug 17, 2018

@mickaelandrieu sorry for delay, as you said it helps with hook and new get set methods, adding get set methods instead of changing access modifiers probably always better solution and i think this PR proves necessity of hooks because we cant override everything but also overrides fixes things fast and makes happy customer as you can understand we cant wait for new versions to fix our personal needs and also thats why you should remove override feature otherwise many php guy will use this feature hardly while writing module, i was using because i saw it in your documents, first of all you need to remove from docs then it will slowly disappear and when the times comes you just remove the overrides and everything will be fine.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

i was using because i saw it in your documents, first of all you need to remove from docs then it will slowly disappear and when the times comes you just remove the overrides and everything will be fine.

👍 you can count on me to remove every mention of overrides in the documentation ;) thanks!

Contributor

mickaelandrieu commented Aug 22, 2018

i was using because i saw it in your documents, first of all you need to remove from docs then it will slowly disappear and when the times comes you just remove the overrides and everything will be fine.

👍 you can count on me to remove every mention of overrides in the documentation ;) thanks!

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 22, 2018

Contributor

PR rebased (cs fixes)

Contributor

mickaelandrieu commented Aug 22, 2018

PR rebased (cs fixes)

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud
Contributor

PierreRambaud commented Aug 22, 2018

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 23, 2018

Member

You already had my blessing. :)

Member

Quetzacoalt91 commented Aug 23, 2018

You already had my blessing. :)

@marionf marionf assigned marionf and unassigned marionf Aug 23, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 23, 2018

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Aug 24, 2018

@PierreRambaud PierreRambaud merged commit 13aca3a into PrestaShop:develop Aug 24, 2018

1 of 2 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the mickaelandrieu:ease-customer-address-form-customization branch Aug 24, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud
Contributor

PierreRambaud commented Aug 24, 2018

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