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

Fix controller override #9042

Merged
merged 2 commits into from Nov 27, 2018

Conversation

Projects
None yet
7 participants
@nenes25
Copy link
Contributor

nenes25 commented May 9, 2018

In complement of my pull request #9041 , the behavior should be corrected for all the controllers ( FO + BO )

Questions Answers
Branch? develop
Description? Remove private vars and methods from controllers to allow their override.
Type? bug fix
Category? BO + FO
BC breaks? no
Deprecations? no
Fixed ticket?
How to test? Override of the modified controllers should works

This change is Reviewable

@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented May 9, 2018

Hello @nenes25!

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

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented May 9, 2018

I'm not in favor of allowing people to override Controllers or help them override more in Controllers, not matter it is in BO or FO.

All legacy BO controllers will be removed during 1.7 release, this is part or our publicly announced migration plan, so you must not rely on BO legacy controllers but only on hooks to alter these pages.

In FO, even if it's a bad idea and if we discourage people to do it since 1.5.x, the behavior will be kept in 1.7.x.

@nenes25

This comment has been minimized.

Copy link
Contributor

nenes25 commented May 10, 2018

I agree that override controllers is a not the best idea, and i use hooks as often as possible.

But it some case it's still usefull and there is no other way than overriding controllers in order to complete our developpement. ( It happens more often for FO )

And in this case as the legacy documentation describe, we should have the possibility to override these controllers without errors linked to private functions or vars.

@kpodemski

This comment has been minimized.

Copy link
Contributor

kpodemski commented May 10, 2018

@mickaelandrieu from one side, i can agree with you, overrides are not a final answer, but untill we have a possibility to completely swap out some back-office features with our own i think that it's good to allow this pull request to go through

btw. @mickaelandrieu - i think that we don't really need to use hooks, i'm a fan of dynamic methods, i'll give you one quick example, recently i was working with back-office on 1.6, on dedicated settings panel for one of my modules, PrestaShop 1.6 handle options very well thanks to HelperOptions but without one little method:
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/controller/AdminController.php#L1473

i wouldn't be able to do what i really need to

i really hope that we'll have these small details in 1.7

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented May 12, 2018

i really hope that we'll have these small details in 1.7

In 1.7, you don't/won't need to override a controller to do this :)

This is the right way to do in a "modern" back office page.

// the hook name depends on page where the form lives

public function hookDisplayAdministrationPageForm(&$hookParams)
{
    $formBuilder = $hookParams['form_builder'];
    $form = $formBuilder->get('a_form_field_with_options'); // can be a select, a collection of input radio or checkboxes...
    // get the current values of options
    $options = $form->getData(); // it's an associative array of course
    // set new options
    $newOptions = [...];
    $form->setData($newOptions);
}

@PrestaShop PrestaShop deleted a comment from prestonBot May 12, 2018

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented May 12, 2018

ping @Quetzacoalt91, we need another point of view here 👍

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented May 14, 2018

Why do you need my point of view for?

If that's regarding the changes of the pull-request, I'd approve them. Of course we're not in favor of overrides, but this is a solution which must work in the case a developer absolutely need them.

@PierreRambaud
Copy link
Contributor

PierreRambaud left a comment

Because override still exists, we need to be consistent.

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Nov 23, 2018

@marionf marionf self-assigned this Nov 26, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 27, 2018

@marionf marionf removed their assignment Nov 27, 2018

@PierreRambaud PierreRambaud merged commit f6734c4 into PrestaShop:develop Nov 27, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment