Comments

Uv516 created an issue. See original summary.

svenryen’s picture

Actually - the old "No I want to find out more" didn't block any cookies. It simply opens the privacy policy.
If you want a Disagree button, can I suggest the "Opt in" or "Opt out" methods?

I'm considering including a Legacy template for those who still want to use "Consent by default" and liked having the two buttons in the banner.

sachariew’s picture

Yes, please add a template or another way to get the previous layout and functionality when we use consent by default. Thanks!

cobadger’s picture

Status: Active » Needs review
StatusFileSize
new5.89 KB

I found this change to be abrupt, having just introduced this module to a couple of clients using consent by default setting.

@svenryen, patch attached with logic to use different info template for consent by default. Is this along the lines of what you had in mind?

svenryen’s picture

Status: Needs review » Needs work

Thanks for the patch. It would be great if it can be a choice, in case somebody wants to use the new template.

sachariew’s picture

Hi, could we get something like this for D8 ? I am still at 8.x-1.0 because of this change.

cobadger’s picture

@svenryen, to clarify: would this choice be for users who have chosen 'consent by default', allowing them to choose whether or not to use the new template?

svenryen’s picture

Yes, correct. We need a setting that's only available for those who select 'consent by default'.

Perhaps a checkbox saying "Use legacy template for cookie information banner" at the bottom of the cookie information banner pane?

It should not be possible to select this option when you're using any of the other methods. Let's hide it in those cases.

cobadger’s picture

Thanks for the clarification. New patch attached.

cobadger’s picture

Status: Needs work » Needs review
StatusFileSize
new5.69 KB

Revised patch attached with correct logic for the config form's default value for the template selection form item.

adamps’s picture

Status: Needs review » Needs work

Same here need the second button back to link to the privacy policy please.

@COBadger Thanks for the patch - it looks like a good start but I had some problems.

  • I think it is missing the new template:-)
  • I got some warnings when the new "info banner template" field was not yet set to any value.
adamps’s picture

Category: Feature request » Bug report
Priority: Normal » Major

Changing category/priority as this seems likely to break a number of sites, so it would be great if we could get a commit and new version soon, many thanks.

  • The earlier versions of the module have a public security vulnerability
  • New versions with 'default' mode are missing the second button to link to the privacy policy which is potentially a legal requirement
  • New versions with other modes automatically block cookies which can render the site non-functional, e.g. can't log in

I wonder, do we actually need a new setting? There is already an option to configure whether to show the second button, so can't we just stick with that? Whilst I agree that in default mode it doesn't make sense to have a "disagree" button, I certainly don't see it a 'legacy' variant to have a second button which could be labeled "more info" or "view policy" - the legislation seems to place a strong emphasis on communicating the policy to site visitors.

I think this affects D7 and D8.

cobadger’s picture

@AdamPS thanks for your feedback on the earlier patch; updated patch attached.

I'm in favor of either keeping the legacy template as the only available template or at least allowing an admin user to choose it, as the structure of the new template moves a button into a different div and changes the styling.

Updated patch attached.

cobadger’s picture

Status: Needs work » Needs review

I do not have a Drupal 8 patch for this issue. Changing status to 'needs review' for the Drupal 7 patch.

adamps’s picture

Category: Bug report » Feature request
Priority: Major » Normal

Thanks @COBadger.

Apologies whilst rushing to apply security patches last week whilst travelling I had misunderstood. As per the issue summary, although the button is missing, there is a link instead. Hence resetting category/status.

Now that I've had a look at the new way, I've decided to stick with it, so I'm afraid I'm not going to be able to test the new patch. However I took a quick glance and it seems to be heading in the right direction.

Just a thought, but presumably anyone who wants the button back without using a patch can simply override eu-cookie-compliance-popup-info.tpl.php in the standard way. We could even attach the necessary template file and instructions here for the issue followers. It could save having yet another admin UI option when there are already many, and it would save the work still needed to create a D8 version of the patch.

svenryen’s picture

I think we need an option for this in the interface. There are still 17,105 sites at v1.14 and 20,411 at v1.23 who might at some point update the module, and I can see how they will be confused when the template changes.

If those who upgrade can get a "Legacy" template by default, we can at least prevent some people from panicking and opening issues, and those who just want to upgrade to get the security patch can do so without having to update their css.

The problem is most noticeable for those that applied custom styling to the "More info" button (some people misleadingly call it Disagree button) and that formatting spills over to the new link.

adamps’s picture

@svenryen sure that's a good point and the current patch is getting close

If those who upgrade can get a "Legacy" template by default

But I think the patch might not do this part

cobadger’s picture

@AdamPS / @svenryen revised patch, including db update to set 'legacy' template by default.

svenryen’s picture

@AdamPS. I was going to suggest we sneak it into one of the existing update hooks. This way we won't revert the behavior for those who already upgraded, and those who upgrade in the future will get the legacy template applied.

This seems to be the least of all evils.

I can't really think of any other way that will work.

svenryen’s picture

Status: Needs review » Needs work
+++ b/eu_cookie_compliance.install
@@ -366,3 +366,14 @@ function eu_cookie_compliance_update_7008() {
+function eu_cookie_compliance_update_7009() {

This update hook doesn't check which method the user currently is at. If they're using "Opt in" or "Opt out" it appears that their configuration will also revert to Legacy.

cobadger’s picture

Status: Needs work » Needs review

Thanks @svenryen for the feedback. We're not setting a variable to hold which template to use for the 'opt in' and 'opt out' options, only for the 'consent by default' option - so there's no risk that the wrong template will be used for 'opt in' or 'opt out' by setting this variable's value to 'legacy'.

cobadger’s picture

Updated patch, moving this initialization into one of the existing db updates per @svenryen in https://www.drupal.org/project/eu_cookie_compliance/issues/2985662#comme...

svenryen’s picture

We're not setting a variable to hold which template to use for the 'opt in' and 'opt out' options, only for the 'consent by default' option - so there's no risk that the wrong template will be used for 'opt in' or 'opt out' by setting this variable's value to 'legacy'.

Missed that part (I hadn't read the entire patch in detail). Thanks for clarifying.

adamps’s picture

#19 is very clever - I was about to comment again to point out that problem.

cobadger’s picture

Updated patch to include a value for array item eu_cookie_compliance['info_template'] upon install, and to move a parenthesis in db update function.

cobadger’s picture

Updated patch with correction to eu-cookie-compliance-popup-info-consent-default.tpl.php.

svenryen’s picture

Thanks for the patch. It needs a port to Drupal 8.

summit’s picture

Nice patch!

cobadger’s picture

Patch ported to Drupal 8 (attached.)

Status: Needs review » Needs work
cobadger’s picture

Testing revised patch for D8.

cobadger’s picture

Status: Needs work » Needs review

Please review Drupal 8 patch:
eu_cookie_compliance-missing_button-2985662-drupal_8-31.patch

cobadger’s picture

Apologies for all of the uploads.

Status: Needs review » Needs work

The last submitted patch, 33: eu_cookie_compliance-missing_button-2985662-drupal_8-33.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

cobadger’s picture

Status: Needs work » Needs review

Testbot failed because there is no test coverage in the module.

svenryen’s picture

The patch needed a bit of polishing.
CSS was slightly off, there were a few coding standard issues and a notice in Drupal 7.

Here are updated patches for D7 and D8.

Thanks for the hard work. This is an important issue.

svenryen’s picture

Status: Needs review » Fixed

  • COBadger authored 0eb6ac7 on 8.x-1.x
    Issue #2985662 by COBadger, svenryen: Missing button
    

  • COBadger authored da64a07 on 7.x-1.x
    Issue #2985662 by COBadger, svenryen: Missing button
    

Status: Fixed » Closed (fixed)

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