Problem/Motivation

The documentation for this module states that it will "automatically add the required Javascript [tracking code] to every page generated by Drupal." However, content security policies generally do not allow inline scripts to be included. Console errors look something like: "Content Security Policy: The page’s settings blocked the loading of a resource at inline (“script-src”)."

Steps to reproduce

Install the Google Analytics module as described. Ensure that your CSP does not include 'unsafe-inline' for the script-src directive.

Proposed resolution

Following the approach taken to patch the Drupal 7 version of this module (#3170031), move the generated script into a separate file.

Remaining tasks

Do it!

User interface changes

Possibly add this feature as an option, rather than as a default implementation change.

API changes

Not anticipating any...

Data model changes

Likely just a single added field to the module configuration.

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:

Comments

beunerd created an issue. See original summary.

beunerd’s picture

Status: Active » Needs review
StatusFileSize
new7.58 KB

Here's a patch that attaches a separate JS file by default, but can be toggled to insert inline if preferred.

Status: Needs review » Needs work

The last submitted patch, 3: 3276807-include-google-analytics--3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

beunerd’s picture

Assigned: beunerd » Unassigned
Status: Needs work » Needs review

The patch in #3 keeps failing tests (against Drupal 9.3 and 9.2), but none appear to be related/caused by these updates... or am I missing something? Edit: Yeah, I was missing something.

beunerd’s picture

StatusFileSize
new6.92 KB

Now including the async gtag script

Status: Needs review » Needs work

The last submitted patch, 6: 3276807-include-google-analytics--6.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

beunerd’s picture

StatusFileSize
new33.25 KB

Addressing tests.

beunerd’s picture

StatusFileSize
new33.25 KB

Renaming the patch file so it's consistent with the comment #.

beunerd’s picture

Status: Needs work » Needs review

Marking this as needs review.

nicholass’s picture

Just noting for others, there is another module which is compatible with the CSP module called.
https://www.drupal.org/project/ga

I think it solves the problem a little differently

beunerd’s picture

StatusFileSize
new33.47 KB

Here's an updated patch.

beunerd’s picture

StatusFileSize
new35.8 KB

Changed the relative path from using `realpath` to the `file_assets_path` to ensure it can be accessed from the resulting markup / HTML.

Status: Needs review » Needs work

The last submitted patch, 13: 3276807-include-google-analytics--13.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

beunerd’s picture

StatusFileSize
new35.81 KB

Missed a slash at the beginning of the relative path.

ricovandevin’s picture

Status: Needs work » Needs review

Changed status to Needs review to trigger tests.

Status: Needs review » Needs work

The last submitted patch, 15: 3276807-include-google-analytics--15.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

r.van.doorn’s picture

StatusFileSize
new31.63 KB

The patch from #15 isn't working for me. The file_assets_path settings that is used for the relative path is empty in my project. I don't like the idea of adding a hardcoded filepath to my own settings file so I've added a fallback to use the public path if the assets scheme doesn't exist.

I also fixed some coding standard issues I ran into while figuring out why the relative path wasn't working.

beunerd’s picture

Status: Needs work » Needs review

Triggering tests

brandon_’s picture

The patch in #18 seems to be working for me as well. Thanks very much.

Hopefully this can get picked up, as it seems to be the best way for GA4 on a site with a strong CSP.

brandon_’s picture

After updating to Drupal 10.1, the patch in #18 stopped working for me. When the url was being built, it didn't properly parse out the assets folder, so I ended up with pages linking to src="//js/google-analytics-XX".

In my case, I don't have an assets folder specified, but with the logic in the patch, it comes back as existing and uses the same value as the public folder. I found that a small change from $settings->get('file_assets_path') to AssetsStream::basePath() made the difference for me.

Otherwise the rest of the changes from r.van.doorn work great. I'm uploading my patch which is basically all of r.van.doorn's work with two lines changed, as well as an interdiff. This works for me on D10.1, and still seems to work as expected on D9.5 as well.

eelkeblok’s picture

Status: Needs review » Needs work

There is a merge request, can that be updated based on the latest changes? Patch workflow is not going to stick around for the long haul, best get to grips with the MR workflow. Also, a quick glance at the patch told me that there are some unrelated code changes in the patch. It is best to limit code changes for an issue to the changes required to solve the issue at hand; it limits the chance of merge conflicts and reduces the number of lines to review.

eelkeblok’s picture

I updated the MR with the latest changes from #21 and merged in 4.x in the process. I'm looking into the implications of using the AssetsStream wrapper directly... I don't think this will work in the current form with Drupal 9.3, which is currently the oldest core version this module claims to support.

eelkeblok’s picture

Just noticed the deprecated notice on the module status and found more information in the documentation (maybe a bit more information wouldn't hurt on the project page either).

Anyway, I rolled back some unrelated changes, which hopefully makes reviewing this a little easier. However, with tests out due to the move to GitLab, I have little faith in this actually ever making the finish line...

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

japerry’s picture

Unfortunately I do not believe such a large change will occur in this module, due to its EOL status. Working to update the documentation so that users will goto the Google Tag module instead. Fortunately the Google Tag module incorporates the goals of this patch, so please try it out!

eelkeblok’s picture

Thanks. The customer was already in the process of moving to GTM, so this was a nice extra nudge :)