Improvement - Theme hook only_pages by lusimeon · Pull Request #19146 · PrestaShop/PrestaShop

lusimeon · 2020-05-14T20:07:28Z

Questions	Answers
Branch?	develop
Description?	In contrast to except_pages sequences in theme.yml who's not very handy, this PR add the posibility to limit hooks to load only on specified pages thanks to an "only_pages" sequence. except_pages continue to work but if only_pages and except_pages are set on the same hook, only_pages has priority.
Type?	improvement
Category?	FO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #16420.
How to test?	Run tests, or add only_pages sequence in theme.yml

This change is

In contrast to except_pages sequences in theme.yml who's not very handy, this commit add the posibility to limit hooks to load only on specified pages.

prestonBot · 2020-05-14T20:07:31Z

Hello @lusimeon!

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

Progi1984 · 2020-05-15T06:58:24Z

@lusimeon It seems that your PR broke unit tests : https://travis-ci.com/github/PrestaShop/PrestaShop/jobs/334202032#L524

Progi1984

Unit tests are broken

See commit #da85498a9d for more details.

matks · 2020-05-18T16:26:09Z

@lusimeon Nice work !

So you decided to use only_pages data and then compare it to the list of all FO controllers to be able to compute a complete except_pages map, is that right ?

I think if we merge this (and it looks like a nice idea) we need to update the documentation

Matt75

Some suggestions

Matt75 · 2020-05-18T17:07:06Z

+                if (!empty($extra_data['only_pages'])) {
+                    $extra_data['except_pages'] = [];
+
+                    $controllersNames = Dispatcher::getControllersNames(_PS_FRONT_CONTROLLER_DIR_);


Maybe you should add ModuleFrontController too 🤔
Because some module controllers are important like for a blog module or discounted product listing for example

You're right, I will add modules front controllers too.

After few research, at this time except_pages seems to don't works with all module controllers because some of thoses don't have controller_name or php_self (see classes/Hook.php line 858) attributes set (eg: module ps_emailsubscription/controllers/front/verification.php results in module-ps_emailsubscription- key).

I think an issue need to be open to fix module controller name to have something even (maybe #19226) before we can include module controller in this PR.

@Matt75 If you have any suggestions on how to get all modules controllers names? I tried with filenames return by Dispatcher::getModuleControllers() but it doesn't works when checking permissions in Hook::exec().

micka-fdz · 2020-05-20T14:14:51Z

I think it would be a good improvement! Good job @lusimeon

sowbiba

seems good to me, waiting for the modification suggested by @Matt75

matthieu-rolland

Thank you for your contribution @lusimeon, lgtm !

We'll wait your implementation of @Matt75 's suggestion before sending this PR to QA.

kpodemski · 2020-05-27T00:34:24Z

Great idea!

lusimeon · 2020-05-27T18:49:44Z

@lusimeon Nice work !

So you decided to use only_pages data and then compare it to the list of all FO controllers to be able to compute a complete except_pages map, is that right ?

I think if we merge this (and it looks like a nice idea) we need to update the documentation

@matks You're right, I'll do it when this PR will be merged. Am I right?

lusimeon · 2020-07-20T16:09:42Z

Hi, any news on this one? cc @Matt75 @matks

PierreRambaud · 2021-07-16T14:08:24Z

Hi, any news on this one? cc @Matt75 @matks

Little reminder guys :)

PierreRambaud · 2021-11-15T15:18:08Z

Hi, any news on this one? cc @Matt75 @matks

Little reminder guys :)

Ping!

Matt75 · 2021-11-15T15:54:29Z

Hello @lusimeon

Good practices for ModuleFrontController is to be declared by developers in their module class : https://github.com/PrestaShop/PrestaShop/blob/1.7.8.1/classes/module/Module.php#L124
At module installation, PrestaShop will automatically register them to Meta table : https://github.com/PrestaShop/PrestaShop/blob/1.7.8.1/classes/module/Module.php#L2970
So controllers will be available in BO > Shop Parameters > Traffic & SEO to manage URL Rewriting for example.
So you can retrieve list of ModuleFrontController thanks to ps_meta table

About ModuleAdminController, it's required to be registered in ps_tab table

PierreRambaud · 2021-12-28T13:40:10Z

Hi @lusimeon ,

Since we had no news from you for more than 30 days, I'll close this pull request. Feel free to reopen or open another one if you think it's still relevant.

Thanks!

FO: Add an only_pages sequences for hooks in theme.yml

da85498

In contrast to except_pages sequences in theme.yml who's not very handy, this commit add the posibility to limit hooks to load only on specified pages.

lusimeon requested a review from a team as a code owner May 14, 2020 20:07

prestonBot added develop Branch Improvement Type: Improvement labels May 14, 2020

Progi1984 requested changes May 15, 2020

View reviewed changes

Add integration and unit test for only_pages feature

1251548

See commit #da85498a9d for more details.

lusimeon force-pushed the feature/theme-hook-only-pages branch from f01cea3 to 1251548 Compare May 18, 2020 09:47

Progi1984 approved these changes May 18, 2020

View reviewed changes

matks reviewed May 18, 2020

View reviewed changes

Comment thread classes/Dispatcher.php

Matt75 reviewed May 18, 2020

View reviewed changes

sowbiba reviewed May 26, 2020

View reviewed changes

matthieu-rolland reviewed May 26, 2020

View reviewed changes

Progi1984 added the Waiting for author Status: action required, waiting for author feedback label May 27, 2020

PierreRambaud requested a review from matks January 21, 2021 14:38

PierreRambaud added the Waiting for dev Status: action required, waiting for tech feedback label Jan 21, 2021

PierreRambaud removed the Waiting for author Status: action required, waiting for author feedback label Nov 15, 2021

matks added Waiting for author Status: action required, waiting for author feedback and removed Waiting for dev Status: action required, waiting for tech feedback labels Nov 17, 2021

PierreRambaud closed this Dec 28, 2021

Conversation

lusimeon commented May 14, 2020 • edited by eternoendless Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestonBot commented May 14, 2020

Uh oh!

Progi1984 commented May 15, 2020

Uh oh!

Progi1984 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matks commented May 18, 2020

Uh oh!

Matt75 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Matt75 May 18, 2020

Choose a reason for hiding this comment

Uh oh!

lusimeon May 19, 2020

Choose a reason for hiding this comment

Uh oh!

lusimeon May 27, 2020

Choose a reason for hiding this comment

Uh oh!

micka-fdz commented May 20, 2020

Uh oh!

sowbiba left a comment

Choose a reason for hiding this comment

Uh oh!

matthieu-rolland left a comment

Choose a reason for hiding this comment

Uh oh!

kpodemski commented May 27, 2020

Uh oh!

lusimeon commented May 27, 2020

Uh oh!

lusimeon commented Jul 20, 2020

Uh oh!

PierreRambaud commented Jul 16, 2021

Uh oh!

PierreRambaud commented Nov 15, 2021

Uh oh!

Matt75 commented Nov 15, 2021

Uh oh!

PierreRambaud commented Dec 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

lusimeon commented May 14, 2020 •

edited by eternoendless

Loading