Keep existing hooks in the theme config

[jolelievre]

Questions	Answers
Description?	Add placeholders in hook definitions to keep the existing hooks from other modules and avoid overriding them all
Type?	bug fix
BC breaks?	no
Deprecations?	no
Fixed ticket?	~
Sponsor company	~
How to test?	See below

This modification is done because we changed the order of steps during installation PrestaShop/PrestaShop#40457 hich highlighted the fact that the config of the theme was not really used on fresh install To remain iso with the activated modules we update the theme config so that it is less strict and keep hooks from all installed modules

How to test

1. You need to use the 9.1.x branch
2. Use the PR of this theme
3. Install the shop with all modules enabled
4. Go to the list of products
5. Without this fix the Brand block is not present
6. With this PR fix the Brand block is present

[tblivet]

Hi @jolelievre, thanks for the PR. I’m wondering if this is a good idea, because by doing this we assume that installing HB will not make the theme “clean,” since all modules hooked via the configuration are considered OK.

On the other hand, not removing all hooked modules could also be a benefit for users who want to switch to HB without losing all their hooked modules.

So I’m not sure whether it would be better to just add only the following:

displayFooterProduct:
  - productcomments
  - ps_categoryproducts

[tblivet]

@jolelievre I just blocked it to get your thoughts, don’t worry 🙂

[tblivet]

👍

[Hlavtox]

Hmmmmm. Sooo, if I understand correctly.

The installation process was changed and now it unhooks all other modules from these hooks?

That it requires changing the hummingbird theme?

That's not good, because it will break all other themes during installation. (From 3rd party distributions or agency prepared packs.)

And moreover - it seems it's a big BC break if you need to adapt both themes. The code in PrestaShop/PrestaShop#40457 must be fixed IMHO.

[jolelievre]

@Hlavtox yes but the current behaviour was incorrect:

• first it didn't take into account the modules to enable/diable in the theme config
• the theme hooks were also not correctly applied, or at least they were applied when the theme was installed and then ALL the modules were installed thus overriding the actual theme configuration

Now that the theme is installed after the modules its configuration is correctly applied. So the previous installation behaviour was buggy, and fixing it simply highlighted the fact that the theme was too strict.

One final thing that makes us think this is the right approach, when you enable/disable the theme from the BO after the installation the config is correctly applied at that moment. So it means the enabling process of the theme was not consistent between the fresh install and the installation from the BO.

This feature has been here from years we just didn't realize it because we (prestashop core team) mostly test PrestaShop after a fresh install. But actually things worked out out of luck. This modification aims at catching up with a hidden bug.

We hesitated between two approaches:

• either hard code the modules used by hummingbird and explicitely list them
• use the ~ "placeholder" so that the theme is less strict and more open to existing modules

We chose the second approach because this is a native theme used as a reference and we prefer to avoid removing hooks from existing modules for people who simply want to test the theme without losing their configuration. But children themes based on hummingbird can always change their config if they want to restrict the list of hooks more strictly.

This simply catches up with a behaviour that was discovered after fixing a bug

[Hlavtox]

Jo, I dont understand.

Tell me please:

1. If I add some custom module to my installation ZIP and this module used displayFooter for example. Will it stay hooked even without the tilde in the YML?
2. If I am running a shop using theme X, with many external modules, some hooked to displayFooter, will they stay hooked when I install hummingbird, even without the tilde in the XML?

[Hlavtox]

Please, dont remove the block immediately. There were many times where even the core team merged some complete bullshit that had to be reverted. You can spare the few minutes or hours until I have a chance to see you reply. :-)

[jolelievre]

1. If I add some custom module to my installation ZIP and this module used displayFooter for example. Will it stay hooked even without the tilde in the YML?

So far it worked, with the fix on 9.1 it won't work anymore because the theme configuration is the norm, if it forces a restricted list of modules on a hook they replace the existing ones previously added when all modules are installed.

2. If I am running a shop using theme X, with many external modules, some hooked to displayFooter, will they stay hooked when I install hummingbird, even without the tilde in the XML?
   Currently AND with the fix no, when the theme is installed its config is applied no matter which modules were previously installed and hooked, only the modules from the theme's wonfig are kept. This is actually the behaviour present for years, but I think the switched steps in the installer actually hide this behaviour.

Initially the fix was so that the modules_to_disable config was correctly applied during fresh install, and it simply highlighted this existing behaviour. So that's what I'm saying the prupose here is to uniformize the behaviour between fresh install and theme install/enable.

[jolelievre]

And you can avoid using "bullshit" to define our actions, "mistake" is much more acceptable 😉

[Hlavtox]

So, I have a tracking module hooked to displayFooter, I change the theme and you unhook it?

So, ps_googleanalytics will stop working because its not in the theme yml?

I think we can revert the core PR :-)

[tblivet]

Hi @Hlavtox , just to make things clear about this PR:

With this fix, if you change the theme to Hummingbird, the currently hooked active modules will remain unchanged, and the modules defined in the configuration will be hooked if that’s not already the case.

So, for example, if you use the “MyTheme” theme with ps_googleanalytics hooked to displayFooter, switching to the Hummingbird theme should keep ps_googleanalytics hooked to displayFooter.

[tblivet]

Here is a small discussion regarding this subject:
#819

[Hlavtox]

@tblivet With this fix and tildes maybe, but what about thousands of themes on addons? Will you change it for them? :-)

This is a big BC break, we will block release of 9.1 if it's not reverted. Even if it fixes a bug, it's still a BC break that will break logic for all themes.

I am astonished that you don't see how wrong it is.

The theme should hook all modules specified in the YML,
ignore the rest,
ignore modules not present on disk.
The same with modules to enable and disable.

[jolelievre]

Sorry for the late answer, I didn't have time for this topic these last few days.

The theme should hook all modules specified in the YML,
ignore the rest,
ignore modules not present on disk.
The same with modules to enable and disable.

That's actually what we are aiming for, but let me clarify things

Behaviour of enabling theme

When a theme is enabled it does a few things depending on its configuration:

1. global_settings.hooks.custom_hooks create new custom hooks
2. global_settings.configuration update some configuration values
3. global_settings.modules.to_disable disable some modules
4. global_settings.modules.to_enable + global_settings.hooks.modules_to_hook enable some modules
5. global_settings.modules.to_reset reset some modules
6. global_settings.image_types install image types
7. global_settings.hooks.modules_to_hook hook some modules to specific hooks

This is the expected behaviour when you enable a theme, and it is correctly applied when you perform the enable action from the back office, or when you upload and install a new theme.

Especially for our discussion, the step 7. is based on the theme configuration we can see the details of this operation here and here, and what it does is:

• it forces the hooks from the theme, any hook that is present in the theme's configuration will be overwritten meaning if it contains ps_languageselector, ps_currencyselector then only these two modules will be hooked, any other module that hooked itself during its installation will be unhooked
• the hooks that are not in the module configuration are left untouched
• in case you want to keep the existing hooks you can use the ~ value (equivalent to null value in PHP) which is like a placeholder of the existing hooks, so you can decide to place it first, at the end or even between two modules But this value has disappeared from the classic theme, and quite logically from the hummingbord theme as well It was used in the old start theme, I don't know why it was not used anymore but my guess is that it may have been a little forgotten over time

This behaviour has been in the core for years, and we absolutely didn't modify it

Installation process so far

Now met's dig into the installation process, up until now it executed the steps in this order (I ignore other irrelevant steps for this discussion):

1. install the theme (so the theme config is applied, including modules to enable/disable and hooks to update based on its config, as explained above)
2. install all the native modules (provided you kept the select all option of course), or install the list of modules (if you decided to uncheck some modules)

But here is the catch, since modules are installed after the theme, they are all installed (regardless of the theme configuration), and all their hooks are installed as well. This means the configuration from the theme (especially global_settings.modules.to_disable and global_settings.hooks.modules_to_hook) is completely ignored and overwrittent.

This means it has two drawbacks:

• the behaviour of enabling a theme during install is completely different from the same action after the install from the BO resulting in a big inconsistency
• you cannot fine tune your installation process and the installed/hooked modules during the installation process, you can put as much effort in your theme config as you want it will be ignored in the end, this is true for the Core default build AND if you prepapre your own custom build

What we changed in the installation process

While working on the integration of hummingbird 2.0 we encoutered a problem, the theme is not compatible with blockwishlist for now so it is configured to disable it. But this didn't work because the blockwishlist module is installed after the theme is enabled anyway.

So to fix this we decided to switch the two steps, this way the theme configuration is correctly applied AFTER the modules are installed and this fixes (at least three things):

• the modules are correctly disabled if they need to
• the hooks are correctly applied based on the configuration
• the behaviour is consistent with the enable action from the BO

But while we worked on fixing the UI tests we noticed that some modules are not present anymore on the FO, and that's logic because they were not hooked anymore since they are not in the theme configuration. This is true with classic and this is tru with hummingbird (event the 1.0 version).

We can't be 100% sure but we strongly think that the fact that the two steps were in this order (first theme then modules) actually made developers forget over the years the importance of the theme configuration. Some modules are expected to be enabled after the install in the core scope, and they were.. but out of luck because regardless of the theme the native modules were forced to be installed and hooked anyway.

It means the theme configuration was lacking some modules in their hook definitions. So we had two possibilities to fix this:

• hard-code the list of the expected modules based on a search in all the native modules, and add all the modules matchfing the theme's hooks to keep the same behaviour as when they were installed after the theme
• or use the (probably, at least by the current core theme) forgotten ~ value in the HB and Classic theme, this way all the installed modules during installation keep their hook, and the theme is only adding hooks that would be missing

We favored the latter solution because HB and Classic are the theme provided by PrestaShop as default examples and as such we think they shouldn't be too strict and avoid removing some existing hooks, especially hooks from modules we can't know since they are not in the Core scope. It also has the advantage of pushing this ~ feature forward in case other people had forgotten about it.

Regarding the BC

We do admit this change of order in the steps has an impact, and yes it changes the way modules are installed and hooked during a fresh install. But that's only during a fresh install, we didin't change anything in the theme enable behaviour. On the contrary we considered it was more logical and more consistent to have a similar behaviour during fresh install and action from the BO.

Even without our modification weh you enable the classic and HB themes you erase the present hooks. It's just that we rarely tested other themes this way and so we didn't put enough effort in improving/completing the theme's configuration.

We also thought that this change mostly has impact only during the installation of the shop, so mostly for native modules since external modules are usually added after the initial installation. The only impacted person are the persons that actually create their own build, so in our opinion the impact is limited. It doesn't mean nobody creates their own build, as you correctly pointed out, it's just that it's not the majoritory use case. Meaning the majority of users won't be impacted by this change.

Possible workaround

Now for people who do create their own build they will be impacted by this change of order, that's true. But it's not something they can't overcome. The solution being that they can change the theme's configuration by defining precisely the modules and hooks they expect, or by allowing existing ones using the ~ value.

And since this behaviour was correctly applied from the BO the themes should already take this into account anyway if you want to guaranty the expected behaviour when you install your theme after the initial installation, if you didn't then it means the theme only works half of the time in a way.

Actually, fixing this order of steps offers more possibility during the fresh install than it did previously because we now respect the actual theme configuration you can decide if you want to mlimit modules or accept more. This was not possible until then because the configuration was ignored.

That's why we consider this fix is relevant, useful and even required despite the impact it can have in some use cases.

[Hlavtox]

@jolelievre Hello Jo, I understand everything.

This behaviour has been in the core for years, and we absolutely didn't modify it

I didn't know, in that case, we can consider PrestaShop/PrestaShop#40457 a valid approach that unifies it.

Let's move on.

---

So, let's focus on the issue we have - if - ~ is not present, all modules not specified in the YML are unhooked - which is 99% of times an unwanted behavior.

Since classic and humminbird are an example, and you are adding - ~ into every hook in the YML, you will keep all the modules hooked on these hooks.

So we may as well just modify the core to ignore the - ~ completely and keep the modules hooked all the time. :-)

You will have the same end result. You will not need to modify any theme. You will improve the behavior everywhere.

---

Possible downsides? When switching to a 3rd party theme that has it's own fork of leo_languageselector for example, and the theme does not have ps_languageselector in to_disable you could have two languageselectors in the frontoffice displayed.

But, I think it's far less evil than if the core silently unhooks all of the modules, even native ones, that are on certain hooks, and for example, your marketing scripts stop working silently.

[jolelievre]

I disagree with this

So we may as well just modify the core to ignore the - ~ completely and keep the modules hooked all the time. :-)
You will have the same end result. You will not need to modify any theme. You will improve the behavior everywhere.

If we do this we will have the same behaviour during the installation process as we did so far, yes. But that's assuming the buggy behaviour should be the correct behaviour. The famous "it's not a bug it's a feature". And if we do that we change the current behaviour of enabling the theme from the BO post install, which does unhook modules if they are not in the configuration, as it has been doing for years only maybe you/we didin't realize it.

Changing this behaviour seems like a bad idea to me, because the it will indeed be a breaking change and one that has no workaround that you can easily go around. On the other side the change we made on install only can easily be overcome if you fix your theme configuration which is not that big of a fix to do.

And to me it really is a fix that should be done on the theme because the theme's config is explicit: as long as you don't put the ~ you are saying I want those modules only and it's completely relevant to want only those. However considering the default behaviour is "ok I keep everything here" is a biased view, it may be your case but it doesn't mean it's the case for everyone.

As you said yourself, usually you want to disable some native modules and only keep your modules that are adapted for your theme, well the current behaviour does exactly this IMHO. Which is why we fixed the install process to also have this behaviour.

TLDR; the current behaviour (wether you use ~ or not) is more versatile and offers more possibilities and fine tuning than forcing all modules, which is why I think it should be kept as the correct behaviour both in the BO and in the install.

[jolelievre]

But, I think it's far less evil than if the core silently unhooks all of the modules, even native ones, that are on certain hooks, and for example, your marketing scripts stop working silently.

Just to be clear the only impacted modules are the ones present in the theme configuration, so most of the time it should be display hooks, so for example ps_googleanalytics based on displayHeader is not impacted because this hook is not in the theme configuration. I'm not saying it's impossible that a theme removes a marketing hook, but taking this into account is important: most of the hooks (that are not in the theme configuration) are not impacted at all.

It mostly means theme developers should be careful with their hook configuration and remember this fact, if you restrict a hook to a set of modules you need to do it carefully and you need to be sure of it. If you're not you can alwys use the ~ field to limit the impact of your theme.

But if you have a very specific header that only works with 2 specific modules then it makes sense that you want to restrict only to those ones.

And either way, the developer/merchant can always reste/reinstall the modules after the theme is enabled or hook them manually from the BO so there's always possibilities to adapt to specific use cases.

[Hlavtox]

As we discussed internally with @jolelievre and @kpodemski, let's add the tildes.

But, I still think that the best way would be to just hardcode this behavior - do not unhook other modules.

[jolelievre]

But, I still think that the best way would be to just hardcode this behavior - do not unhook other modules.

That's what we do with the tilde actually, force some modules on top of the hook list and keep the already hooked ones If we hard code the list then all other modules (including third parties) would be unhooked It's legitimate for a third party module but for the open source one it makes sense to keep installed modules as is and not mess with their hooks

[ingridusta]

Hi @jolelievre,

I tested your pr with only this PR (hummingbird theme) and also with both PR (hummingbird and classic themes), and it works perfectly :

All modules impacted by this PR are well displayed on the FO :
psbrandlist :

psemailalert :

ps_special :

ps_categoryproducts :

ps_viewedproduct :

It's QA approved !! ✅