oveleon / contao-cookiebar

Cookie bar for the Contao Open Source CMS
GNU Affero General Public License v3.0
58 stars 25 forks source link

bugfix globals $objPage in parseTemplateHook #197

Closed xprojects-de closed 7 months ago

xprojects-de commented 7 months ago

see https://github.com/oveleon/contao-cookiebar/issues/195#issuecomment-1997458246

@zoglo @fritzmg @doishub

We have the problem that at the time of parsing the CookiebarTemplate "global $objPage" (and maybe some other Globals set by FrontendIndex) is not set and therefore HookListener for parseTemplate may have a problem.

There are three (four) solutions:

  1. set the global $objPage
  2. Use the parseLayout-Listener to parse the template
  3. let it as it is and do not support global :-) (4. temporally deactivate HookListeners when parsing Cookiebar-Template )

My preferred solution is of course 3 :-) , but since Contao still supports it, I would set the global $objPage. Maybe some other $GLOBALS set by FrontendIndex are still not avalible, but this is only for the parssing of the cookiebar-Template ...

The PR-Draft includes both solutions for 1) and 3). One has to be removed ...

What do you think?

fritzmg commented 7 months ago

I don't quite understand the problem - what is it that you want to do?

doishub commented 7 months ago

I'm not that deep into the issue yet, but what if we just use $request->attributes->get('pageModel'), as Fritz has already written?

xprojects-de commented 7 months ago

Again, we are right and we don't use global $objPage! :-)

But on https://github.com/oveleon/contao-cookiebar/issues/195 the Extension "contao-themes-net/theme-components-bundle" has a parseFrontendTemplate-Listener (see https://github.com/contao-themes-net/theme-components-bundle/blob/27d4db24f48eb8cd5f2743015e994873cf936dfe/src/EventListener/ParseFrontendTemplateHook.php#L30) and this Listener use global $objPage!

I know that is not good but this Listener is triggered also when we parse our CookiebarTemplate and at the point we parse it, the global $objPage is not set! This is not about our implementation, but rather that we support legacy implementation of other extensions!

zoglo commented 7 months ago

@xprojects-de shouldn't this be something that should be fixed by contao-themes-net? I thought they provide their own consent-tool as well

fritzmg commented 7 months ago

Ah, I see the problem now, I misunderstood the original problem. You are rendering a FrontendTemplate in a kernel.request event listener. This is probably not a good idea. Is there any particular reason why you are doing this in a kernel.request event listener?

xprojects-de commented 7 months ago

We actually want to do it without the hooks so that we are safe with Twig because not all hooks are called consistently with fragments at the moment. So the question now arises as to whether the geneartePage hook is "Fragment-Save" (which I believe) :-) and we should use it or should we rather set the "global $objPage".

EDIT: The other question is whether it is good to render a template in the generatePage hook!? :-)

fritzmg commented 7 months ago

The other question is whether it is good to render a template in the generatePage hook!? :-)

Rendering a FrontendTemplate within the regular Contao rendering context is fine.

xprojects-de commented 7 months ago

So maybe we can use the generatePage-Hook till globals are no longer supported and then switch back to previous implementaion ...

fritzmg commented 7 months ago

then switch back to previous implementaion ...

The current implementation should not be used. You are rendering a FrontendTemplate in a kernel.request event listener. This will cause various hooks to be executed at a point in time that is unexpected by any listeneres of these hooks.

xprojects-de commented 7 months ago

Ok i see and understand! So i think it's good way to do it in generatePage Hook. And if the fe_Page becomes Twig maybe the Hook is still there or any other Hook is present 👍

zoglo commented 7 months ago

Thank you a lot @xprojects-de!