oveleon / contao-cookiebar

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

Contao 5 and Twig compatibility #182

Closed xprojects-de closed 7 months ago

xprojects-de commented 7 months ago

@zoglo @doishub

Hallo ihr beiden,

ich habe mal einen POC gemacht und alles auf kernel.event_listener umgebaut. Das finde ich viel besser wie die Hooks und es ist unabhängig von Twig- oder HTML-Templates.

Wenn es in Zukunft die vielleicht Twig-Events gibt dann kann man sich die ganzen RegExp sparen.

Zwei Dinge müssen wir noch prüfen/anpassen (Vielleicht habt ihr da eine Idee):

Könnt ihr euch ja mal anschauen und Feedback geben ...

Grüße

xprojects-de commented 7 months ago

Nach Rücksprache mit Yanick ist bei jedem SubRequest und auch beim MainRequest immer das PageModel ab Contao5 dabei!

Weiters geht bei der Webseite der Mainrequest immer auf die FE_PAGE!

xprojects-de commented 7 months ago

in think wen also should set

"contao/core-bundle":"^4.13 || ^5.0",
"symfony/http-foundation":"^5.4 || ^6.0",
"symfony/serializer":"^5.4 || ^6.0"

to

"contao/core-bundle":"^5.0",
"symfony/http-foundation":"^5.4 || ^6.0",
"symfony/serializer":"^5.4 || ^6.0"

because i don´t know if the listenerLogik with PageModles work in Contao 4.13!?

zoglo commented 7 months ago

because i don´t know if the listenerLogik with PageModles work in Contao 4.13!?

The same listener logic exists in Contao 4.13 afaik

xprojects-de commented 7 months ago

yes, the listeners do, but I don't know whether the models are present in the request too. We should check that ...

xprojects-de commented 7 months ago

I still had a few adjustments that I made. Now we should look at the adjustments and evaluate whether it is the right way ...

xprojects-de commented 7 months ago

I have now tested all of my standard use cases and they work so far.

Contao 5.2.8 Usecases:

These are the use cases that I always have and they work. Unfortunately, I can't say yet whether everything else works. :-)

xprojects-de commented 7 months ago

I noticed something else...

In the parseCookiebarTemplate hook, the CookiebarModel is passed as a reference and can theoretically be manipulated there. In the previous code, the CookiebarModel was regenerated every time the element/module was parsed and the hook only came at the end with the outputFrontendTemplate. So you could not dynamically manipulate config in the hook for the element/module process.

With the current implementation, the hook comes at the beginning before the elements are parsed and the CookieModel is always the same reference. This would make it possible to manipulate the CookieModel via the hook and this would also affect the parsing of the elements/modules.

That's the question of whether we want that or whether that would be a BC break!?

At the moment it would only affect that:

Cookiebar::validateCookies($this->cookiebarModel)
zoglo commented 7 months ago

With the current implementation, the hook comes at the beginning before the elements are parsed and the CookieModel is always the same reference. This would make it possible to manipulate the CookieModel via the hook and this would also affect the parsing of the elements/modules.

That's the question of whether we want that or whether that would be a BC break!?

IIRC, we do have versions for every cookiebar so the version should change if the CookieModel has been changed to make sure that users get the new version 🤔 /cc @doishub ?

@xprojects-de We really appreciate you working on this. We were discussing your PR this afternoon but didn't have time to review it yet. That's for next week.

fritzmg commented 7 months ago

I am still unsure whether a kernel.response listener is the right approach. If possible please check the performance impact for cases with a lot of content elements on one page.

xprojects-de commented 7 months ago

I once profiled 40 content elements and did each with RequestListeners and pureHooks. No difference in time! Attached are the profiles... (My local server is very slow :-) , dev-mode, etc ...)

cachegrind.out.90651_pureHooks.gz cachegrind.out.90660_listeners.gz

But it would be great if you could test that too. I will also do more tests...

xprojects-de commented 7 months ago

cachegrind.out.95461_pureHooks2.gz cachegrind.out.95464_Listeners1.gz cachegrind.out.95466_Listeners2.gz cachegrind.out.95466_pureHooks1.gz

Here are more profiler results with the same result. Currently, in my opinion it makes no difference whether you use KernelRespoinses or pure Hooks.

I think at one point or another the code can certainly be made even more performant.

I'm looking forward to your results and what you find out... :-)

zoglo commented 7 months ago

As discussed on Slack, we still may find another solution in the future as Twig could offer possibilities. This is currently a working solution that we can work with for now.

It will not be merged into the current 5.x-dev as several people are already using it. It will be moved to a new branch which we will prepare for a new version with all the latest changes as we are almost 2.5 years behind on commits from main.

zoglo commented 7 months ago

Thank you @xprojects-de :tada:

xprojects-de commented 7 months ago

Here you can see what other kernel response listeners are active when rendering a ContentElementFragment and I don't think the cookie bar here is particularly noticeable.

Bildschirmfoto 2024-02-06 um 19 46 51

Bildschirmfoto 2024-02-06 um 20 18 32 Bildschirmfoto 2024-02-06 um 20 18 51

Based on the debug results, I would leave it like that for now.