After the upgrade to 1.26 my own scripts started behaving strangely. Tracked the problem down to the fact that they were running twice. Reversing the patch in 3008618 seems to solve the issue.

CommentFileSizeAuthor
#29 eu_cookie_compliance-jquey-scripts-in-drupal-attachBehaviors-are-being-run-twice-3038237-7.x-29.patch5.62 KBsvenryen
#17 loaded-scripts-running-twice.patch1.28 KBsaschahannes
#15 attachbehaviour-3038237-15.patch1.19 KBspokje
#13 attachbehaviour-3038237-13.patch1.19 KBspokje
#11 attachbehaviour-3038237-11.patch1.08 KBdanyg

Comments

benqwerty created an issue. See original summary.

svenryen’s picture

svenryen commented
Status: Active » Postponed (maintainer needs more info)

This bug report is a bit vague. Are you able to provide steps to reproduce? Does it also occur on a test site with only one script being handled? Can you also include the output of 'drush vget eu_cookie_compliance'?

danyg’s picture

danyg commented

I can confirm the the original notion. After removing the changes came #3008618 from (Drupal.attachBehaviors();), the js errors disappeared again from console.log (for me the slick.js threw errors)

benqwerty’s picture

benqwerty commented
Status: Postponed (maintainer needs more info) » Needs work

I've had the issue on at least two sites and I've tested a clean site and it does the same. To reproduce:
Upgrade EU Cookie Compliance to 1.26
Add a plain script with console.log("Script run");
Add the script to the theme's info file (or from template.php using drupal_add_js)
Load the page and the console shows "Script run" twice
Revert changes from 3008618 and it will only run once

drush vget eu_cookie_compliance gives:

eu_cookie_compliance:
  popup_enabled: 1
  method: default
  info_template: new
  disabled_javascripts: ''
  whitelisted_cookies: ''
  consent_storage_method: do_not_store
  popup_clicking_confirmation: 1
  popup_info:
    value: 'We use cookies on this site to enhance your user experience. By clicking any link on this page you are giving your consent for us to set cookies.'
    format: full_html
  use_mobile_message: 0
  mobile_popup_info:
    value: ''
    format: full_html_paragraphs
  mobile_breakpoint: '768'
  popup_agree_button_message: 'OK, I agree'
  show_disagree_button: 1
  popup_disagree_button_message: 'No, give me more info'
  disagree_button_label: 'No, thanks'
  withdraw_enabled: 0
  withdraw_message:
    value: '<h2>We use cookies on this site to enhance your user experience</h2><p>You have given your consent for us to set cookies.</p>'
    format: plain_text
  withdraw_tab_button_label: 'Privacy settings'
  withdraw_action_button_label: 'Withdraw consent'
  popup_agreed_enabled: 0
  popup_hide_agreed: 0
  popup_agreed:
    value: '<h2>Thank you for accepting cookies</h2><p>You can now hide this message or find out more about cookies.</p>'
    format: filtered_html
  popup_find_more_button_message: 'More info'
  popup_hide_button_message: Hide
  popup_link: privacy
  popup_link_new_window: 0
  popup_position: false
  use_bare_css: 0
  popup_text_hex: ffffff
  popup_bg_hex: '555555'
  popup_height: ''
  popup_width: 100%
  fixed_top_position: 0
  popup_delay: '1000'
  disagree_do_not_show_popup: 0
  reload_page: 0
  popup_scrolling_confirmation: 0
  cookie_name: ''
  domains_option: '1'
  domains_list: ''
  exclude_paths: ''
  exclude_admin_pages: 0
  exclude_uid_1: 0
  script_scope: footer
  better_support_for_screen_readers: 0
  cookie_session: 0
eu_cookie_compliance_cookie_lifetime: '100'
eu_cookie_compliance_domain: ''
svenryen’s picture

svenryen commented

I'll have to investigate this a bit, since this was originally requested by somebody who's site we'll likely break if we just revert 3008618, as well as potentially other sites that matches their use case.

Would it be a suitable remedy to include a checkbox to "Add Drupal.attachBehaviors" in association with the script blocking feature, so that those who need it can enable the feature and have that appended to the inline script?

toddses’s picture

toddses commented

This also seems to be an issue with the latest drupal 8 release (8.x-1.4). Scripts that previously were using Drupal Behaviors to take some action based on the context being 'document' are now executing twice.

toddses’s picture

toddses commented
svenryen’s picture

svenryen commented

Yes, it's correct to say nothing has been done about this issue in version 1.4.

toddses’s picture

toddses commented

This wasn't an issue in 1.2, but we pushed the 1.3 and 1.4 updates today and it started. We actually have a site running the old version and an environment with the new version. Not sure if this is helpful at this point, but just noting.

svenryen’s picture

svenryen commented

It has been identified as being related to #3008618: attachBehaviors after loading blacklisted scripts but I haven't had time to look into it. You can revert that commit to resolve the issue if you're affected. I need to examine this in detail and check with the reporter of that issue why it was necessary for him to make that change.

danyg’s picture

danyg commented
Status: Needs work » Needs review
StatusFileSize
new attachbehaviour-3038237-11.patch1.08 KB

Here is a patch which can fix this issue.
If we don't want to load extra script element we don't need to call Drupal.attachBehaviors();

I hope it will be good approach.

svenryen’s picture

svenryen commented

Thanks for the patch. It will need some testing before we can commit it to the branch. I'm not 100% convinced we solve all cases with the patch in #11, there may be some edge cases where attachBehaviors() shouldn't run even though we have content in $load_disabled_scripts.

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new attachbehaviour-3038237-13.patch1.19 KB

I'll have to investigate this a bit, since this was originally requested by somebody who's site we'll likely break if we just revert 3008618, as well as potentially other sites that matches their use case.

I fear _a lot_ more site are breaking _because_ of #3008618, I myself have around 20 so far.

Nothing worse than complaining without putting in some effort to move forward.
Patch in #11 worked for me, although the patch isn't really in the normal format.

Let's start by uploading #11 in the normal patch format, so more people can easily use to.
(All credits to @danyg)

I will have a look to see if the patch covers both #3008618 and edge cases next.

spokje’s picture

spokje
Primary language Dutch
commented
Title: Changes in issue 3008618 causes scripts to run twice » jQuery scripts in Drupal.attachBehaviors() are being run twice

Changed title hoping to make it more clear

spokje’s picture

spokje
Primary language Dutch
commented
StatusFileSize
new attachbehaviour-3038237-15.patch1.19 KB

Fixed newline mistake in #13

ludo.r’s picture

ludo.r
Location Brussels
commented

I confirm patch #15 solves the issue.

saschahannes’s picture

saschahannes commented
StatusFileSize
new loaded-scripts-running-twice.patch1.28 KB

Patch for drupal 8 version: https://www.drupal.org/files/issues/2019-03-08/loaded-scripts-running-tw...

Status: Needs review » Needs work

The last submitted patch, 17: loaded-scripts-running-twice.patch, failed testing. View results

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Put patch similar to #15 for 8.x version of this module in related issue. (https://www.drupal.org/project/eu_cookie_compliance/issues/3038661).

Let's keep D7 and D8 separated to prevent confusion. :)

I'm working on tests for the D8 patch. I won't be working on tests for this D7 version.

Putting this back to Needs Review, with a "It works for me" in #16.

agileadam’s picture

agileadam
Location Maine, USA
commented

I too can confirm that the patch in #15 fixes the issue in Drupal 7.64 with EU Cookie Compliance 7.x-1.27.

millerrs’s picture

millerrs commented

Patch #15 works for me.

spokje’s picture

spokje
Primary language Dutch
commented

If anyone could mark this as RTBC we could try to get this into the next release.

@svenryen Would you consider getting out a new release for this?
In my mind it's big enough to do so.

Because the twofold applying of javascripts in Drupal.attachBehaviors(), some sites are breaking, but not in a immediately noticeable way, so I expect a lot more people finding out and needing this patch in the next few days.

Examples:

I have a site that attaches a toggle open/close through attachBehaviors on a onClick event.
Since it's currently applied twice, clicking that element now opens and immediately closes the element again.
Not something you notice straight away, but eventually it will turn up in a bug report.

danyg’s picture

danyg commented

@Spokje: thanks for correcting my patch, I don't understand why PHPStorm did this format.
I'm still checking the issues for another sites I've made and I try to put the issue to RTBC

spokje’s picture

spokje
Primary language Dutch
commented

@danyg Thanks :)

I'm a PHPStorm user myself and had the same problem with patches, so I still do them on the cmd-line.

Would be very nice to get this RTBC. Even nicer if we get this in a release ASAP, the team I work in manage a lot of sites and we have to decide if we go with the patch when updating because of the SA, or hold off a bit longer for an official release (preferably the latter of course).

The code looks 99,9% bulletproof to me, and I'm working on some PHPUnit tests in the related D8-version of this issue to prove that for the full 100%.

This module having no tests in place at all is not helping me (let alone the fact that my PHPUnit testing knowledge in Drupal is minimal to none...), but I hope/think I can get it done this weekend.
(Famous Last Words)

danyg’s picture

danyg commented
Status: Needs review » Reviewed & tested by the community

@Spokje: damn, you're right. I forgot that issue was existed for me already and then I created patch by command line. Ahh :)

So, I've tested the patch on another site than for which I made the patch yesterday and I can confirm that it fixes the duplicate call problem. I put it to RBTC
(And of course it works with version 1.27 too)

spokje’s picture

spokje
Primary language Dutch
commented
Version: 7.x-1.26 » 7.x-1.x-dev

@dannyg Thanks, I've put the version to 7.x-1.x-dev since that's the version I've created the patch against.

It will also apply cleanly to 7.x-1.27:

$ patch -p1 < /path/to/attachbehaviour-3038237-15.patch
patching file eu_cookie_compliance.module
spokje’s picture

spokje
Primary language Dutch
commented

Found out some more about what's going on for the D8 version of this module here: https://www.drupal.org/project/eu_cookie_compliance/issues/3038661#comme...

I _didn't_ test this for the D7 version (and I won't), but since the modules are quite alike for this I _think_ the same thing is happening.

svenryen’s picture

svenryen commented

I've resolved this issue in https://www.drupal.org/project/eu_cookie_compliance/issues/3038661#comme... and I'm working on a similar fix for D7.

svenryen’s picture

svenryen commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new eu_cookie_compliance-jquey-scripts-in-drupal-attachBehaviors-are-being-run-twice-3038237-7.x-29.patch5.62 KB

I've spent a few hours on the issue today, and came up with a proper solution that runs the attach code for only the behaviors of the scripts that you choose to blacklist. I should have validated the request in #3008618: attachBehaviors after loading blacklisted scripts a bit closer before just adding the feature.

The patch introduces additional parameters that can be entered in the Blacklist scripts text area.

Scripts that attach to Drupal.behaviors are supported. To indicate a behavior that needs to be loaded on consent, append the behavior name after the script with a | (vertical bar). Example: modules/custom/custom_module/js/custom.js|customModule. Note that the Drupal behavior name is optional, but may be required to achieve your objective.

Can somebody take the attached patch for a spin and confirm it solves the issue with an RTBC?

I do wish to provide the functionality that was originally requested, and the patch broadens the reach of the blacklisting feature. It also ensures that no script runs twice, not before during and after the consent has been given.

  • svenryen committed b3e50b4 on 7.x-2.x 
    Issue #3038237 by Spokje, svenryen, danyg, SaschaHannes, toddses,...
svenryen’s picture

svenryen commented

The 8.x issue was RTBC in https://www.drupal.org/project/eu_cookie_compliance/issues/3038661#comme... and since this is merely a port of the 8.x code, I'll risk that this works (I did test it fairly, and the code that caused the issue has been removed).

I've committed this patch to -dev and I'll tag a release after checking if there are other critical issues that should go in the release.

svenryen’s picture

svenryen commented
Status: Needs review » Fixed

  • svenryen committed b3e50b4 on 7.x-1.x 
    Issue #3038237 by Spokje, svenryen, danyg, SaschaHannes, toddses,...
  • svenryen committed ec666e2 on 7.x-1.x
    Merge branch '7.x-2.x' into 7.x-1.x

* 7.x-2.x:
Issue #3038237 by Spokje...
svenryen’s picture

svenryen commented

Tagged and released. Thanks for everybody that helped nail this one.

danyg’s picture

danyg commented

I've checked the latest patch on a D7 site, it also fixes the duplicated calls for other JS commands. On this site I use "Consent by default. Don't provide any option to opt out." method, so the disabled javascripts doesn't affect to the tested site.
But it fixes the original problem of this issue.
Thanks to you, guys!

benqwerty’s picture

benqwerty commented

I’ve installed version 1.28 and the problem that I initially logged is fixed. Thank you to everyone who contributed to this!

Status: Fixed » Closed (fixed)

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