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

Various improvements #64

Merged
merged 11 commits into from Apr 9, 2019

Conversation

@eternoendless
Copy link
Member

eternoendless commented Mar 22, 2019

Main improvements:

  • It no longer downloads tons of useless css files (see ee4d784)
  • Small performance improvements thanks to removal of redundant logic (see 6a10cac)

Fixes PrestaShop/PrestaShop#13017
Fixes PrestaShop/PrestaShop#12848

eternoendless added 10 commits Mar 21, 2019
This variable contained the list of all advices including premium and except addons. It was initialized in the hook event, then passed along to AdminGamificationController, who then used it as whitelist for filtering a list of advices of its own. This controller-generated list, before being filtered, will have either _only_ premium advices or all advices _except_ premium and addons. Following this, we can conclude that controller-generated list will always be a fully contained subset of the whitelist, therefore making the whitelist redundant and useless.
public static function getValidatedPremiumByIdTab($id_tab)
public static function getValidatedPremiumOnlyByIdTab($id_tab)

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Mar 27, 2019

Member

isn't a BC break?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Mar 28, 2019

Author Member

I don't think it's a problem if we release a major version. In any case, I don't think anyone's using this class other than the module itself 😁

Copy link
Member

mickaelandrieu left a comment

Looks good to me but can be improved removing the BC break (let's deprecate the old public function instead and rely on the new ones?)

Also, this need to be cs fixed :)

public static function getAddonsAdviceByIdTab($id_tab)
public static function getValidatedAddonsOnlyByIdTab($id_tab)

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Mar 27, 2019

Member

isn't a BC break?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Mar 28, 2019

Author Member

Not a problem if we release a major version

This comment has been minimized.

Copy link
@jolelievre

jolelievre Mar 28, 2019

Agreed, in a module I think we're safe ^^

composer.json Show resolved Hide resolved
Copy link

jolelievre left a comment

@eternoendless little warning on the use of ajaxProcess

echo json_encode($return);
exit;

This comment has been minimized.

Copy link
@jolelievre

jolelievre Mar 27, 2019

I think we should avoid using exit in ajax methods

This comment has been minimized.

Copy link
@jolelievre

jolelievre Mar 27, 2019

The cleanest way I found to return a json response in modules was:

        header('Content-Type: application/json');
        $this->ajaxRender(Tools::jsonEncode(array(
            'address_form' => $this->render(
                'customer/_partials/address-form',
                $addressForm->getTemplateVariables()
            ),
        )));

It could be nice to have an ajaxRenderJson(arra $data) method
BTW why did you replace Tools::jsonEncode? Although I rarely do, should we avoid using it?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Mar 28, 2019

Author Member

There was a die before already, which I agree is very bad practice. There's no AjaxRender() in ModuleControllers unfortunately.

Tools::jsonEncode should be deprecated as it's no longer useful for PHP >= 5.5.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Mar 28, 2019

ajaxRender is in ControllerCore so it should be available in ModuleController as well, however it is only available since 175..

This comment has been minimized.

Copy link
@eternoendless

eternoendless Mar 28, 2019

Author Member

My mistake, I forgot. That's the reason I didn't use it in the first place, because this module must be compatible with 1.7.0+ (at least in theory it is)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Apr 8, 2019

Ok, but do we really need to keep this exit ?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Apr 8, 2019

Author Member

We do if we don't want to change the current behavior

gamification.php Show resolved Hide resolved
@marionf marionf added QA approved and removed Waiting for QA labels Apr 9, 2019
@eternoendless eternoendless dismissed mickaelandrieu’s stale review Apr 9, 2019

Comments addressed for

@eternoendless

This comment has been minimized.

Copy link
Member Author

eternoendless commented Apr 9, 2019

Thanks guys!

@eternoendless eternoendless merged commit c9a5877 into PrestaShop:dev Apr 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@eternoendless eternoendless deleted the eternoendless:fix-things branch Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.