EU Cookie Compliance breaks AMP Verification

I'll see if we can make the module work better with AMP. Thanks for the report!

At the moment the only way to make EU Cookie Compliance module work with AMP is disable it and use what AMP specification says to use ))
Its forbidden to use third party JS on AMP pages, which makes it impossible to use something except what AMP have in it.
See documentation about amp-user-notification element and try to make it work.

I used something like this:

<amp-user-notification
  id="sliding-popup"
  data-persist-dismissal="false"
  data-show-if-href="{{ data_show_if_href }}"
  data-dismiss-href="{{ data_dismiss_href }}"
  layout="nodisplay"
  class="sliding-popup-top clearfix">
  // popup HTML here.
</amp-user-notification>

Where 'data_show_if_href' and 'data_dismiss_href' are two API endpoints which GETs and POSTs cookies which are used on main non-AMP pages by EUCC module. So if user accepted cookies on main site, he will not see the popup on AMP pages.

Good morning,
Has anyone found a solution to this problem?

Greetings and thanks

Hi,

As a solution for now, you can conditionally remove the unwanted style asset provided by the eu cookie compliance via hook_page_attachments_alter()

Add the following to your .theme file inside your amp theme folder.

/**
 * Implements hook_page_attachments_alter().
 */
function yourtheme_page_attachments_alter(&$attachments) {
  if (isset($attachments['#attached']['html_head'])) {
    $list = $attachments['#attached']['html_head'];
    foreach ($list as $key => $attachment) {
      if (isset($attachment[1]) && $attachment[1] == 'eu-cookie-compliance-css') {
        unset($attachments['#attached']['html_head'][$key]);
      }
    }
  }
}

So what we'll have to do is check if the page is being viewed as AMP, and then remove the inline css?

or you could wrap the attachment within some logic that checks if the current theme is based on an AMP theme. Something like

$activeTheme = \Drupal::service('theme.manager')->getActiveTheme();
$isAmpTheme = FALSE;
foreach ($activeTheme->getBaseThemes() as $theme) {
  if ($theme->getName() == 'amptheme') {
    $isAmpTheme = TRUE;
  }
}
if (!$isAmpTheme) {
  // Add attachments..
}

if ($theme->getName() == 'amptheme') {

This seems to only work with https://www.drupal.org/project/amptheme
What about custom AMP implementations?

True, you could check if the module exists and is enabled first.

Is there any patch or other solutions for this?
Our site's AMP pages were all affected and finally tracked to this module as the cause.
We have disable the module for now while allowing the errors to clear in Google Console to restore our impression stats.
There is an option to exclude path in the advanced settings tab but inserting */?amp* doesn't seem to have any effects.

@Eurekaweb: What does the amp paths look like? Do you have an example?

When AMP pages are available the typical path would be the url?amp

In that case, can you use XDebug to check that your path is actually being excluded.

If you entered literally "*/?amp*" as an option to exclude, I don't think it would match a URL being http(s)://domain.com/page?amp

It would however match "http(s)://domain.com/page/?amp"

(notice the trailing slash to make your pattern match.)

Sorry for the late response. Missed your reply.

The amp url typically results in this format:
http(s)://domain.com/page?amp

So the when "*/?amp*" is setup to exclude resulting "http(s)://domain.com/page/?amp" would not work to exclude.

Any suggestions on how to accomplish the exclusion or any other work arounds?

Thanks

I hear you, but I haven't had time to look into the issue.

Meanwhile, can you change your pattern into *?amp and see how that works?
Even if it did work, your pattern "*/?amp*" doesn't match "http(s)://domain.com/page?amp", though it seems to match "http(s)://domain.com/page/?amp"

(Notice, again, the slash in your pattern)

I can confirm that adding '*?amp' as the option to exclude didn't work. I am after this feature as well, so request if anyone can take this up.

For me even uninstalling the module (using admin interface, and even purging configs with easy_install and even after deleting the module folder) doesn't help at all.

Somehow there's more than 100 entries containing "eu_cookie_compliance" in database. Not sure if it's designed to be like this or I did something wrong.

Any help to safely purge database from those entries?
(Blunt force like simply manually removing all them doesn't work as intended, tried already. Good that I make backups all the time)

pick_d, are you using D7 or D8?

And can you let me know how your report is related to problems verifying the site under AMP?

Upgrading this to major, because each update of the module breaks amp pages for all my sites.

Do we have a roadmap for this?

I hear you. If all goes according to plan, all bug reports will be cleared within a month. That's assuming no single issue requires several days to resolve.

Patches are always welcome.

If somebody could help summarize a list of all changes that needs be done in order for the module to support AMP, that could really help.

Do I understand it correctly, that if AMP is detected, we want to just skip adding any js or css to the page?
Is there anything we could do to replace functionality under AMP?

From what I understand, we can leverage amp-user-notification.

I have to admit I haven't worked with AMP implementations, so I'd have to read up a bit.

@scenryen
Currently just skipping the style tags in the html fixes the problem. Specifically

415:421 in eu_cookie_compliance.module

$attachments['#attached']['html_head'][] = [
        [
          '#tag' => 'style',
          '#value' => $data['css'],
        ],
        'eu-cookie-compliance-css',
      ];

So then the additional requirement from #4 is wrong.

Its forbidden to use third party JS on AMP pages

I would assume this means we should also omit JS, but that's maybe handled by an amp-module?

yes it is most likely that the js is picked up by the AMP library

What is the current state on this topic?

For the list of all changes i think we need:
- A proper documentation of how to do things:
+ Disable JS and CSS.
We are using #6 and added eu_cookie_compliance/eu_cookie_compliance: false to the my_theme.info.yml
+ Explaining which additional libraries should be included. amp-user-notification and amp-analytics
+ Explain how to create the right notification html and where to place it.
- Create a api to see if there was givven a consent (GET) and to publish a consent (POST)

I think the way @tarasich in comment #4 tried to do it is the best. Because if you would leave out the api part, then the user would have to accept again, when you leave the amp site and go to your main, which isn't probably in amp.

So the issue is that we sometimes handle user data under AMP even though no js files are loaded? We're displaying the banner using js, so based on this issue I'm not really sure how we can do that on AMP, let alone set cookies to indicate the user wants to be tracked.

Is an acceptable solution to simply "disable" the module output when AMP is being used?

Hello
Please let me clarify something important. AMP pages are served from Google.
AMP pages accessibility by our site users with /?amp urls IS possible but this is NOT the purpose of AMP pages.
Yes, google reads a non AMP page, then, this page informs Google bot that there is an AMP alternate page and then Google makes the AMP page available through a URL like:
https://news.google.com/articles/CBMidWh0dHA6Ly9hbmFnbm9zdGlzLm9yxxxxxxxxxxxxxxxxxxxxxxxx
The important here is that we DO NOT need cookies conset from our users because when they are in our AMP page, actually they are inside Google's domain.
In my opinion, this means that EU Cookie Compliance module should not load at all in AMP pages.

Hello,
I had the exact same problem today with amp and a website with eu cookie.
I made a patch to remove all code from eu_cookie_compliance_preprocess_page() in the .module file when the current theme in amptheme or a theme based on amptheme.
It's perhaps not the greatest patch but I don't need eu_cookie on the amp theme, I'll add the amp code for this.
And the error was a major issue with my website at the moment.

Hope it can helps if someone has the same issue.

Thanks for your patch @mugensama.

Do you know if most sites use this theme for their amp purpose?

+++ web/modules/contrib/eu_cookie_compliance/eu_cookie_compliance.module	(date 1621592700219)
@@ -54,206 +54,213 @@
+function eu_cookie_compliance_preprocess_page(&$variables)

Can you please tidy up your patch as per Drupal coding standards? The opening curly bracket should be on the same line as the function name. https://www.drupal.org/docs/develop/standards is a helpful guide, so is running CodeSniffer.

Also, we don't need the rather long else branch, since if the 'if' condition is hit, the function will return.

Thanks @svenryen, i'll try update the patch asap

I think the amptheme should be the default theme for amp purpose (because of all the changes already done to templates), and creating a child theme base on this feels like it's the best practice for custom amp pages.

Thanks for your patch @mugensama.

I just faced the same issue while I'm validating my AMP pages.

Just I have refactored the patch as per Drupal coding standards.

The patch on #32 for version 8.x-1.x-dev

This patch is only for version: '8.x-1.14' until the patch #32 patch committed in the next release.

Patch looks good. I tested the generated code using Bartik AMP against the validator at https://search.google.com/test/amp and although I got a number of unrelated errors, I didn't get any error on inline css.

For clarity, I updated the patch with an additional () around the if statement, since my IDE complained it was ambiguous.

if ((!empty($parents_theme) && array_key_exists('amptheme', $parents_theme)) || $themeName === "amptheme") {

I try this new patch #36 to what I had before as a manual patch but both can't be applied. anymore :-(
I have $themeName === "bartik_amp"

diff --git a/eu_cookie_compliance.module b/eu_cookie_compliance.module
index 3b4d59b..1eca4ce 100644
--- a/eu_cookie_compliance.module
+++ b/eu_cookie_compliance.module
@@ -54,6 +54,15 @@ function eu_cookie_compliance_help($route_name, RouteMatchInterface $route_match
  * Implements hook_page_attachments().
  */
 function eu_cookie_compliance_page_attachments(&$variables) {
+  // Make the module work better with AMP.
+  $activeTheme = \Drupal::service('theme.manager')->getActiveTheme();
+  $themeName = $activeTheme->getName();
+  $parents_theme = $activeTheme->getBaseThemeExtensions();
+
+  if (!empty($parents_theme) && array_key_exists('amptheme', $parents_theme) || $themeName === "amptheme" || $themeName === "bartik_amp") {
+    return;
+  }
+
   $config = \Drupal::config('eu_cookie_compliance.settings');
 
   // Check Add/Remove domains.

@Rar9 are you applying the patch to the latest dev-version from git?

Is bartik_amp not a sub-theme of "amptheme"?

@svenryan Yes, I used the bartic_amp amp subtheme for my amp pages in conjuction with

drupal/amp 3.5.0 Google AMP integration
drupal/amptheme dev-3.x 7d65ca8 The AMP Base theme converts core templates to use AMP HTML.

Both patches current the wont apply :-(

@Rar9. We're working on getting a new version out. Until then feel free to run the -dev version.