codefog / contao-cookiebar

Display the information about cookies on your Contao website
MIT License
26 stars 9 forks source link

Prevent cookie bar caching while page cache is enabled in Contao 4 #34

Closed fatcrobat closed 6 years ago

fatcrobat commented 6 years ago

@qzminski

Because the hook $GLOBALS['TL_HOOKS']['getCacheKey'] is no longer supported in contao 4, when page cache is enabled, the cookie bar will be rendered all time, whether or not the user accepted the cookie message by clicking the button.

To maintain page cache within contao 4 i created an uncached inserttag {{cookie_bar|uncached}} that will be added within the outputFrontendTemplate hook and afterward replaced by the replaceInsertTags hook, ignoring page cache for the cookie bar itself.

The uncached inserttag flag works in both contao 3 and 4, so i dropped the modifyCacheKey hook.

qzminski commented 6 years ago

Would it not be better to use the kernel.request event somehow?

fatcrobat commented 6 years ago

You propably could, but this fix addresses both contao 3 and 4 without any independent version fixes required.

qzminski commented 6 years ago

I would like to merge this PR but can your bring back the previous code style? You have changed the indentation tabs to spaces which makes it difficult to check at first glance what has been exactly changed.

fatcrobat commented 6 years ago

@qzminski Hopefully restored the previous code style.

One more question:

In CookieBar::addCookiebarScripts() you check that isCookiebarEnabled before you invoke css. We use css aggregation with sass on our local machines and put the compiled css on the server. If we have read the cookie notice and disabled cookie bar on our local machines, the cookie css wont be compiled in our aggregated css file. Do you have any problems with removing the isCookiebarEnabled inside CookieBar::addCookiebarScripts()?

qzminski commented 6 years ago

I have merged the PR, thanks!

In CookieBar::addCookiebarScripts() you check that isCookiebarEnabled before you invoke css...

In that case you should likely override that hook and provide your own logic. This is an edge case and I don't see a way to change this without a BC break.

fatcrobat commented 6 years ago

I totally understand, thanks for the merge.