Add explicit configuration in hooks for missing native modules by jolelievre · Pull Request #904 · PrestaShop/hummingbird

jolelievre · 2026-01-19T15:45:19Z

Questions	Answers
Description?	Add explicit configuration in hooks for missing native modules, now that we validated that the `~` works as expected it's safer to update the configuration with actually expected hooks But we keep the `~` so this reference module doesn't mess with the shop configuration for tests
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	~
Sponsor company	~
How to test?	Not sure it needs to be tested since we already checked with QA the `~` value, this modification only is useful when you switch from/to tier party themes

Hlavtox

Same as classic PR, the hooks are not correct I think.

tblivet

Thanks @jolelievre 👍 these are my comments.
@Hlavtox, do you agree with this?

tblivet · 2026-01-19T16:54:54Z

        - ~
      displayContactContent:
        - contactform
+        - ps_contactinfo


ps_contactinfo This one should be removed.

Yes, ps_contactinfo should not be here.

tblivet · 2026-01-19T17:04:26Z

-        - ps_customtext
        - ps_featuredproducts
        - ps_banner
+        - ps_customtext


ps_customtext Should stay just after the image slider.

tblivet · 2026-01-19T17:06:48Z

        - ps_linklist
        - ps_customeraccountlinks
        - ps_contactinfo
+        - ps_socialfollow


ps_socialfollow This one should be removed.

tblivet · 2026-01-19T17:09:03Z

        - productcomments
+        - ps_categoryproducts
+        - ps_crossselling
+        - ps_googleanalytics


ps_googleanalytics I think we don't need this one.

Its used for tracking I think, but why do we need a non theme module configured here? I dont get it @jolelievre

tblivet · 2026-01-19T17:12:28Z

      displayLeftColumn:
        - ps_categorytree
        - ps_facetedsearch
+        - ps_brandlist


ps_brandlist in my opinion, this should be moved to modules to disabled option.

tblivet · 2026-01-19T17:12:40Z

        - ps_facetedsearch
+        - ps_brandlist
+        - ps_contactinfo
+        - ps_supplierlist


ps_supplierlist in my opinion, this should be moved to modules to disabled option.

tblivet · 2026-01-19T17:13:11Z

        - ps_categorytree
        - ps_facetedsearch
+        - ps_brandlist
+        - ps_contactinfo


ps_contactinfo This one should be removed.

Yes, this should not be there.

tblivet · 2026-01-19T17:14:12Z

        - ps_sharebuttons
        - productcomments
+        - ps_emailalerts
+        - ps_viewedproduct


ps_viewedproduct This one should be removed.

This is kinda borderline - the hook is required by this module to be there, it tracks the views there, check the code there.

But, does this entry belong to a theme configuration, if it's not visible? Maybe? Or not?

I don't really understand the logic of this whole config thing. Yes, a theme should look somehow when it's installed, but it's the module that says what hooks should be where. 🤔 We decide to change hooks in the next module version and we will adapt the theme YML? Hm hm hm.

ps_viewedproduct is already hooked into displayFooterProduct, which is why I think it isn’t needed here. Moreover, this hook isn’t suitable for displaying a product list because it doesn’t take the full width.

@tblivet It's there not to display, but to $this->addViewedProduct($this->currentProductId);.

tblivet · 2026-01-19T17:23:26Z

      displayTop:
        - ps_mainmenu
        - ps_searchbar
+        - pagesnotfound


pagesnotfound I think we don't need this one.

jolelievre · 2026-01-19T23:31:05Z

@tblivet @Hlavtox can you give more details to why you want to remove some of the modules please?

FYI I didn't add anything, I simply checked the native modules that register to the same hooks defined by the theme Which I didin't add anything, the added modules in this PR are purely a reflection of what was already happening when we installed all the native modules during a fresh install

Hlavtox · 2026-01-20T07:39:40Z

@jolelievre I get it, but the fact that module registers somewhere, doesn't make it a theme module.

Theme YML should contain modules that NEED to be hooked so the theme works and functions properly.

It should not mess up with statistics module, analytics modules etc., they have nothing to do with a theme.

tblivet · 2026-01-20T08:43:30Z

Hi @jolelievre and @Hlavtox,
I share the same opinion as @Hlavtox only modules that display content on the front office should appear in this config.

@jolelievre, if I’m right, for example, the ps_contactinfo module is hooked on two hooks that are displayed on the same page, which causes unnecessary duplication and isn’t great in terms of design. However, if I understand correctly, you’re just rewriting the hooks the module is attached to by default, so even if we remove the line, I don’t think this will actually fix the the duplication issue 😕

So maybe we should take some time to update the config file to explicitly list all the modules that need to be hooked and remove the - ~ option. Alternatively, for more flexibility, what do you think about adding a modules_to_unhook option in the config file?

Hlavtox · 2026-01-20T09:00:30Z

@tblivet - ~ must stay, or it will unhook all other modules from that hook on theme activaton, which is 99% of time an unwated behavior.

tblivet · 2026-01-20T09:12:45Z

@Hlavtox ok 👍 and what about modules_to_unhook that doesn’t exist yet, right?

tblivet · 2026-01-20T09:43:11Z

@jolelievre @Hlavtox, without the ability to fine-tune things (for example via a modules_to_unhook option), we will end up with situations like this:

2 social links instead of a single correct one (the expected one is the version on the white background).

2 contact info sections on the contact page, where only the one in the left column is properly styled for the theme

Hlavtox · 2026-01-20T10:32:58Z

@tblivet This is an issue with the ps_contactinfo module. It should not be hooked to displayContactContent, no idea why it's there in the module settings.

jolelievre · 2026-01-20T10:49:27Z

@tblivet @Hlavtox

I propose closing this PR because I don't have time to argue for hours about this 😅
It's not at all a rage quit or against you, this was just a suggestion to have an exhaustive configuration

Wether you like it or not the modules will be hooked in these places during a fresh install anyway, but I agree with you that if they've been unhooked maybe it's not relevant to systematically rehook them when you switch to the theme (which is the only difference this PR brings).

So let's keep the ~ as is, it's probably enough

jolelievre · 2026-01-20T10:51:16Z

Regarding the modules_to_unhook proposition why not
But it's a new feature and we (I mean the core engine team) don't have time to add it these days

tblivet · 2026-01-20T10:56:16Z

@Hlavtox ok, but regarding the ps_contactinfo issue, it’s unfortunate that we can’t act on unhooking modules using the theme config, whereas we can control where modules are hooked. I think that’s where the config.yml is useful.

No problem @jolelievre i understand, sorry, but in fact, this is a real subject 👍 I think modules_to_unhook is a way to give us more control over the theme and avoid unwanted design issues.

Hlavtox · 2026-01-20T11:14:23Z

@jolelievre No worries Jo, everything is calm. It's just that originally good idea revealed million questions and nonsense we didn't know about. :-D

tblivet · 2026-01-20T17:32:05Z

@jolelievre as you mentioned before, I’ve closed the PR for now because it will need further discussion.

jolelievre · 2026-01-20T19:01:49Z

@tblivet if you don't want to have ps_contactinfo on the displayContactContent then we can drop the ~ for this specific case and add a comment in the theme config to explain why maybe?

Hlavtox · 2026-01-20T21:26:17Z

@jolelievre I think the correct solution would be to remove this hooking on module install. :-)

jolelievre · 2026-01-21T07:20:37Z

Why? if the module wants to hook there why would we remove it from the module just to please one theme?

Hlavtox · 2026-01-21T07:33:11Z

The module shouldn't want to hook there, it's some mistake introduced here - PrestaShop/ps_contactinfo#49.

Another proof - https://github.com/PrestaShop/classic-theme/pull/52/files

jolelievre · 2026-01-21T08:18:51Z

Oh! nice digging

Let's drop it then 👍

jolelievre · 2026-01-21T09:45:36Z

@Hlavtox PrestaShop/ps_contactinfo#65

Add explicit configuration in hooks for missing native modules

3e3bc15

ps-jarvis added this to PR Dashboard Jan 19, 2026

github-project-automation Bot moved this to Ready for review in PR Dashboard Jan 19, 2026

jolelievre requested a review from tblivet January 19, 2026 15:45

jolelievre added this to the v2.0.0 milestone Jan 19, 2026

Touxten approved these changes Jan 19, 2026

View reviewed changes

mattgoud approved these changes Jan 19, 2026

View reviewed changes

ps-jarvis added the Waiting for QA label Jan 19, 2026

ps-jarvis moved this from Ready for review to To be tested in PR Dashboard Jan 19, 2026

Hlavtox requested changes Jan 19, 2026

View reviewed changes

ps-jarvis added the Waiting for Author label Jan 19, 2026

tblivet requested changes Jan 19, 2026

View reviewed changes

jolelievre mentioned this pull request Jan 19, 2026

Add explicit configuration in hooks for missing native modules PrestaShop/classic-theme#190

Closed

tblivet closed this Jan 20, 2026

github-project-automation Bot moved this from To be tested to Closed in PR Dashboard Jan 20, 2026

Touxten removed Waiting for QA Waiting for Author labels Jan 20, 2026

jolelievre deleted the native-hooks branch January 21, 2026 07:20

jolelievre mentioned this pull request Jan 21, 2026

Remove displayContactContent hook PrestaShop/ps_contactinfo#65

Open

Uh oh!

Conversation

jolelievre commented Jan 19, 2026

Uh oh!

Hlavtox left a comment

Choose a reason for hiding this comment

Uh oh!

tblivet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolelievre commented Jan 19, 2026

Uh oh!

Hlavtox commented Jan 20, 2026

Uh oh!

tblivet commented Jan 20, 2026

Uh oh!

Hlavtox commented Jan 20, 2026

Uh oh!

tblivet commented Jan 20, 2026

Uh oh!

tblivet commented Jan 20, 2026

Uh oh!

Hlavtox commented Jan 20, 2026

Uh oh!

jolelievre commented Jan 20, 2026

Uh oh!

jolelievre commented Jan 20, 2026

Uh oh!

tblivet commented Jan 20, 2026

Uh oh!

Hlavtox commented Jan 20, 2026

Uh oh!

tblivet commented Jan 20, 2026

Uh oh!

jolelievre commented Jan 20, 2026

Uh oh!

Hlavtox commented Jan 20, 2026

Uh oh!

jolelievre commented Jan 21, 2026

Uh oh!

Hlavtox commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jolelievre commented Jan 21, 2026

Uh oh!

jolelievre commented Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Hlavtox commented Jan 21, 2026 •

edited

Loading