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

FO: override CustomerAddressForm->submit method more easily #9417

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
5 participants
@azisyus
Contributor

azisyus commented Aug 7, 2018

now you can override CustomerAddressForm->submit method more easily without trying to hack private variables

Questions Answers
Branch? develop
Description? inheritance CustomerAddressFormCore class and just copy past submit(); method then as you can see it wont work because while taking instance of Address, it takes $this->language->id as parameter but its a private variable so we cant reach it and if you write a module interests here, probably we cant override only submit method and all we can do is copy pasting whole class only for changing 2 line
Type? improvement
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? protected is more accessible then private access modifier so it cant break anything

This change is Reviewable

FO: now you can override CustomerAddressForm->submit method more easi…
…ly without trying to hack private variables
@prestonBot

This comment has been minimized.

Show comment
Hide comment
@prestonBot

prestonBot Aug 7, 2018

Collaborator

Hello @azisyus!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

Collaborator

prestonBot commented Aug 7, 2018

Hello @azisyus!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@azisyus azisyus changed the title from FO: now you can override CustomerAddressForm->submit method more easily to FO: override CustomerAddressForm->submit method more easily Aug 7, 2018

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.5.0 milestone Aug 8, 2018

@Quetzacoalt91

This comment has been minimized.

Show comment
Hide comment
@Quetzacoalt91

Quetzacoalt91 Aug 8, 2018

Member

A QA check is not mandatory here, I'm merging directly.

Member

Quetzacoalt91 commented Aug 8, 2018

A QA check is not mandatory here, I'm merging directly.

@Quetzacoalt91 Quetzacoalt91 merged commit 1bad185 into PrestaShop:develop Aug 8, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

inheritance CustomerAddressFormCore class and just copy past submit()

we cant override only submit method and all we can do is copy pasting whole class only for changing 2 line

Let's be clear on it: we don't expect you to make a module that only copy paste a whole class and alter 2 lines, and modules with overrides are forbidden in PrestaShop Addons Store. If properties are private, this is for a good reason: except $address property, all others properties must not be altered as they may broke the intended behavior.

I will update this class.

Contributor

mickaelandrieu commented Aug 13, 2018

inheritance CustomerAddressFormCore class and just copy past submit()

we cant override only submit method and all we can do is copy pasting whole class only for changing 2 line

Let's be clear on it: we don't expect you to make a module that only copy paste a whole class and alter 2 lines, and modules with overrides are forbidden in PrestaShop Addons Store. If properties are private, this is for a good reason: except $address property, all others properties must not be altered as they may broke the intended behavior.

I will update this class.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

it may be reverted by #9440, please review :)

Contributor

mickaelandrieu commented Aug 13, 2018

it may be reverted by #9440, please review :)

@rGaillard

This comment has been minimized.

Show comment
Hide comment
@rGaillard

rGaillard Aug 13, 2018

Member

@mickaelandrieu it is easier for an agency to make an override to change a behavior than making a module. It is also better for the shop performance....

Member

rGaillard commented Aug 13, 2018

@mickaelandrieu it is easier for an agency to make an override to change a behavior than making a module. It is also better for the shop performance....

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

@rGaillard and this make the maintenance, the upgrade and the reputation of our product bad since years, I know.

We have already announced many times that this practice (overrides in specific folder) is discouraged and we are deprecating it to be able to remove it in the next major.

You may agree or not, but the easiest way to alter a behavior is using an hook: the performance issue isn't worth the upgrade issues (and can be solved).

Contributor

mickaelandrieu commented Aug 13, 2018

@rGaillard and this make the maintenance, the upgrade and the reputation of our product bad since years, I know.

We have already announced many times that this practice (overrides in specific folder) is discouraged and we are deprecating it to be able to remove it in the next major.

You may agree or not, but the easiest way to alter a behavior is using an hook: the performance issue isn't worth the upgrade issues (and can be solved).

@azisyus

This comment has been minimized.

Show comment
Hide comment
@azisyus

azisyus Aug 13, 2018

Contributor

@rGaillard you are right about what you said and its very easy way to change/fix things for small teams/agencies but i really wonder if 2 different people overrides same class what will happen, i can see that module conflict[SOLVED] titled forum topics from here

Contributor

azisyus commented Aug 13, 2018

@rGaillard you are right about what you said and its very easy way to change/fix things for small teams/agencies but i really wonder if 2 different people overrides same class what will happen, i can see that module conflict[SOLVED] titled forum topics from here

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 13, 2018

Contributor

@azisyus this is why we're willing to remove this feature and include more hooks and docs to make shops more stable. I don't say this won't be "as easy" as before, but I can promise we're working on it to ease the work of developers on maintenance and upgrade 👍

Contributor

mickaelandrieu commented Aug 13, 2018

@azisyus this is why we're willing to remove this feature and include more hooks and docs to make shops more stable. I don't say this won't be "as easy" as before, but I can promise we're working on it to ease the work of developers on maintenance and upgrade 👍

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