Problem/Motivation

On a Drupal 10.1 site, when this module is configured to disable certain JavaScript and JS aggregation enabled, certain admin page functionality such as admin toolbar, operations etc. will be broken.

Steps to reproduce

Enable and configure the Google Tag module, adding an a Google Tag ID like "UA-12345-12" to the site (/admin/config/services/google-tag).

On the EU Cookie Compliance settings page:

Add the following to the "Disable JavaScripts" section:

category:modules/contrib/google_tag/js/gtm.js||google_tag/gtm
category:modules/contrib/google_tag/js/gtag.js||google_tag/gtag
modules/contrib/google_tag/js/gtag.ajax.js||google_tag/gtag.ajax

Ensure the "Advanced" > "Exclude paths" textarea is empty, or in particular these paths are no longer present:

/admin
/admin/*
/node/add*
/node/*/*

NB: This step might not be necessary, but it's an easy way to trigger the bug consistently.

Now attempt to view a page in the admin, e.g. the content listing page (/admin/content) or a node with multiple revisions' revision page (/node/%nid/revisions), the page will appear broken.

The following exception is shown in the watchdog logs when Drupal tries to load the aggregated JavaScript:

Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Invalid filename. in Drupal\system\Controller\AssetControllerBase->getGroup() (line 224 of /app/application/web/core/modules/system/src/Controller/AssetControllerBase.php).

It doesn't seem to be theme specific, but it affects Claro and Seven admin themes.

Proposed resolution

Ensure we manipulate the final JS only once the page has been fully built and all assets have been attached, but before aggregation is ran. hook_js_alter() is triggered after the aggregation is generated, so its implement is mostly redundant.

Remaining tasks

Provide issue fork/MR.

User interface changes

N/A

API changes

Introduced a new response event subscriber and a new _eu_cookie_compliance_process_disabled_javascripts() function that acts on response arrays and attachment responses.

Data model changes

N/A

Release notes snippet

TBD

Issue fork eu_cookie_compliance-3412431

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

markusa created an issue. See original summary.

markusa’s picture

markusa commented

When I "View Source" of admin vs. front end pages ..
On admin pages:
I see an aggregated js file, then 2 gtm js scripts NOT aggregated and then a couple more aggregated JS files
On the front end, I don't see the un-aggregated gtm scripts in the source.

So whatever its doing, when the gtm scripts are NOT in the aggregated scripts, the condition occurs.

If I configure the Google Tag module to exclude /admin/* pages, the condition disappears.

What bothers is why on the admin theme only, is there some 3rd component interacting?

kevinquillen’s picture

kevinquillen commented

This only appears to work if you Exclude from Admin Pages in the cookie consent settings.

finn lewis’s picture

finn lewis
Location Oxford
commented

We were seeing exactly the same problem.

Enabling: "Exclude admin pages." and "Don't show the banner for site administrators (including UID 1)." seems to resolve it for us.

kevinquillen’s picture

kevinquillen commented

Yes, this is still a problem. This should be disabled entirely for administrative paths by default.

codebymikey’s picture

codebymikey commented
Version: 8.x-1.24 » 8.x-1.x-dev
Related issues: +#3312879: Disable JavaScript isn't completely working (to prevent cookies before consent), +#3111111: Disable JavaScripts - splitting config string not working

I ran into this issue myself with the google_tag module.

It seems to be because google_tag's hook_page_attachments() implementation is ran after eu_cookie_compliance's.

This means that the disabled_javascripts check doesn't necessarily work as expected.

I notice that we also implement eu_cookie_compliance_page_attachments_alter() but don't use that to manipulate the attached library definitions. Is there any particular reason we don't handle the disabling of all assets in the alter hook?

It might also be worth implementing a hook_module_implements_alter() to ensure that the module acts on the attachments last.

Thoughts are more than welcome!

codebymikey opened merge request !138

codebymikey’s picture

codebymikey commented
Status: Active » Needs review

Updated the logic so that all the duplicated logic is handled by a single function, as well as removed all the disable logic from hook_page_attachments() to hook_page_attachments_alter() so that it can detect dynamic libraries such as the google_tag one that might be added in other hook_page_attachments() calls.

Also provided a MR which implements the fix in an event subscriber rather than through hooks so that the behaviour is more consistent and runs after all the render hooks have been applied.

Usable patch for v1.24

codebymikey’s picture

codebymikey commented
Issue summary: View changes

Updated issue summary.

drcolossos’s picture

drcolossos commented

The comment from #4 worked as expected. For Google Tag Module: It also works if you enable only "Anonymous Users" only tracking

hmdnawaz made their first commit to this issue’s fork.

svenryen’s picture

svenryen commented
Issue tags: +Open Ownership Pledge

atowl made their first commit to this issue’s fork.

atowl’s picture

atowl commented

Hi All,

Is the MR138 good to go? or are we still missing something?
It looks fine to me.

  • atowl committed eddcd7fa on 8.x-1.x authored by codebymikey 
    Issue #3412431: JS Aggregation breaks in 10.1 when disabling other...
atowl’s picture

atowl commented
Status: Needs review » Fixed
Issue tags: -Open Ownership Pledge

Hi All,
Merged and closed as fixed.

Thanks for help on this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.