Prevent undefined id_module error on Modules page

[kpodemski]

Questions	Answers
Branch?	develop
Description?	We've noticed that sometimes after update we encounter undefined notice error in this file, id_module is for some reason not available and to prevent having fatal error we need to check if we can rely on id_module or we should continue in the loop
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?
How to test?	Hard to reproduce but happend two times after upgraded store with developer mode enabled

---

This change is 

[Quetzacoalt91]

Thanks for this PR @kpodemski.
We are covering a notice with that fix, but are probably hiding another issue (more critical?).

All modules registered to a hook must be installed, thus must have an ID. I don't really understand how that case could happen.
What kind of update are you talking about in your description? A PrestaShop upgrade or a module upgrade?

[kpodemski]

PrestaShop upgrade, modules which causes this error are installed, this is why it's weird, however i have not noticed any downsides after this change, everything is working as expected

[mickaelandrieu]

Hmm can we log something in case the module id doesnt exists? I'm never in favor of hidding a possible real issue.

[Quetzacoalt91]

Is there an easy way to add to the log without adding the Logger? Something with @trigger_error()?

[kpodemski]

I don't think that PrestaShop has any other method to log errors than Logger? unless there's something from Symfony, dunno

btw. i'm not sure what is the purpose of LegacyHookSubscriber on Modules page in the first place :)

[Quetzacoalt91]

The Logger is actually a service from Symfony. But I guess this is an extreme change just for that case (furthermore, we are in a static context).
We also have the legacy solution (which I don't approve), which is the PrestaShopLogger, with static methods.

[kpodemski]

Oh, yes, i was writing about PrestaShopLogger, haven't use symfony one yet

You know, from what i saw a lot of modules cause errors, dashtrends, pm_advancedtopmenu and few others, i think that there's some logic issue with entire LegacyHookSubscriber idea on Modules page as those modules are working fine...

[Quetzacoalt91]

Okay, let's make it simple first.
Can you please use the PrestaShopLogger in order to notify the merchant/shop maintainer about the issue?
As we are in an Adapter, that's okay.

[kpodemski]

@Quetzacoalt91 this would log notice on every request, not sure if that's a good idea, after over 6 months of running store on production (a big store) i couldn't find any downsides of having this change in code

[kpodemski]

@Quetzacoalt91

issue was hook with an empty name in database, fix from this PR doesn't work fully, we need to check this earlier:

foreach ($hooks as $hook) {
                if (empty($hook['name'])) {
                    continue;
                }

could you try to add hook into database with an empty name and check on your side if you have this notice with debug mode enabled?

[Quetzacoalt91]

Hi @kpodemski,

I reproduce the issue, but it seems the issue is worse than we could expect.

Added an empty hook, which is stored with the ID 248 in my example.

$ update ps_hook set name = "displayDoge" where id_hook = 248;

Default behavior:

$ update ps_hook set name = "" where id_hook = 248;

Unexpected behavior, all hooks are returned:

[Quetzacoalt91]

Is #13711 a better fix for this issue?

[kpodemski]

@Quetzacoalt91 yes, this seems to fix issue entirely