Hello,

When exporting configurations from this module, something struck me when having a look at labels.
Here is the section I'm talking about in the user interface:
EU Cookie Compliance banner labels

These 3 fields match the configuration entries that follow:

popup_agree_button_message: 'OK, I agree'
popup_disagree_button_message: 'More info'
disagree_button_label: 'No, thanks'

Looks like there's an issue with technical names, as I would have expected popup_disagree_button_message to be popup_find_more_button_message, but it's already in use for the "Thanks banner"...

I'm confused.

CommentFileSizeAuthor
#29 eu_cookie_compliance-names_mixed_up-2969161-25.patch10.27 KBdennis_meuwissen
#28 eu_cookie_compliance-names_mixed_up-2969161-25.patch.txt10.27 KBdennis_meuwissen
#24 eu_cookie_compliance-names_mixed_up-2969161-24.patch10.28 KBijsbrandy
#21 eu_cookie_compliance-names_mixed_up-2969161-21-8.x.patch10.25 KBeduardo morales alberti
#21 eu_cookie_compliance-names_mixed_up-2969161-21-8.x.patch10.25 KBeduardo morales alberti
#16 interdiff_4_16_v8.x.txt2.21 KBeduardo morales alberti
#16 eu_cookie_compliance-names_mixed_up-2969161-16-8.x-1.2.patch10.37 KBeduardo morales alberti
#15 interdiff_9_15.txt2.38 KBeduardo morales alberti
#15 eu_cookie_compliance-names_mixed_up-2969161-15-7.x.patch10.98 KBeduardo morales alberti
#10 eu_cookie_compliance-names_mixed_up-2969161-9-7.x.patch10.21 KBeduardo morales alberti
#6 eu_cookie_compliance-technical_names_mixed-2969161-6-8.x.patch9.71 KBeduardo morales alberti
#4 eu_cookie_compliance-technical_names_mixed-2969161-4-8.x.patch9.43 KBeduardo morales alberti
eucc-CookieBannerLabels.png11.11 KBpacproduct

Comments

pacproduct created an issue. See original summary.

svenryen’s picture

svenryen commented
Category: Bug report » Feature request

It's an old module, so we have carried along some legacy names for variables, as new ones were introduced.

I don't think this will be a prioritized task, but you're welcome to provide a patch. Please don't break backwards compatibility :)

If it's okay, I'm classifying this as a feature request, since it's not really a bug per se.

svenryen’s picture

svenryen commented
Status: Active » Closed (works as designed)
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Closed (works as designed) » Needs review
StatusFileSize
new eu_cookie_compliance-technical_names_mixed-2969161-4-8.x.patch9.43 KB

Hi, here is my patch to help you to rename that variables.

Status: Needs review » Needs work

The last submitted patch, 4: eu_cookie_compliance-technical_names_mixed-2969161-4-8.x.patch, failed testing. View results

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new eu_cookie_compliance-technical_names_mixed-2969161-6-8.x.patch9.71 KB

Update patch.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Ready to review.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
svenryen’s picture

svenryen commented

Thanks. The patch would need a port to drupal 7 before we can add this to the module.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-9-7.x.patch10.21 KB

The Drupal 7 patch is ready to review.

svenryen’s picture

svenryen commented 
+++ b/templates/eu_cookie_compliance_popup_info.html.twig
@@ -30,8 +30,8 @@
-      {% if disagree_button %}

The problem here is that you're changing the template twig file. This means anybody who made a custom template will have to update their twig when these changes are introduced. I thank you for your enthusiasm in making this patch, but we can't change this variable in the theme and twig due to legacy reasons. It may break some sites when they upgrade the module.

We can make these changes, but we can't change the _theme() and the twig.

svenryen’s picture

svenryen commented
Status: Needs review » Needs work
svenryen’s picture

svenryen commented

So just to clarify my previous comment.

The old template has a variable called {{ disagree_button }} . Some sites may have created a template in their theme folder to override the default template, using this variable.

When you introduce a new variable such as {{ more_info_button }} , you need to keep providing the old variable, even though that does indeed mean having duplicate variables for the same purpose.

We can't accept a patch that may break an existing site. Can you please update the patch so that it doesn't remove any existing variables? Let me know if this was unclear or if you have any questions.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented

Thank you svenryen, I'll work on it today.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-15-7.x.patch10.98 KB
new interdiff_9_15.txt2.38 KB

Added deprecated variable disagree_button to avoid problems in customs templates.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-16-8.x-1.2.patch10.37 KB
new interdiff_4_16_v8.x.txt2.21 KB

Drupal 8 Added deprecated variable disagree_button to avoid problems in customs templates.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: eu_cookie_compliance-names_mixed_up-2969161-16-8.x-1.2.patch, failed testing. View results

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Patch needs update to apply in actual version.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs review » Needs work
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-21-8.x.patch10.25 KB
new eu_cookie_compliance-names_mixed_up-2969161-21-8.x.patch10.25 KB

Update patch Drupal 8 to apply in the latest version.

eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
eduardo morales alberti’s picture

eduardo morales alberti
Primary language Spanish
Location Spain, 🇪🇺
commented
Status: Needs work » Needs review

Patch applied successfully.

ijsbrandy’s picture

ijsbrandy commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-24.patch10.28 KB

I was not able to use the patch from #21 with stable version 8.x-1.5. Attached patch to apply to stable version 8.x-1.5

Status: Needs review » Needs work

The last submitted patch, 24: eu_cookie_compliance-names_mixed_up-2969161-24.patch, failed testing. View results

svenryen’s picture

svenryen commented

The patch seems ok (although haven't tested it yet), but we need a port to Drupal 7 before this can be committed.

svenryen’s picture

svenryen commented
Status: Needs work » Needs review

Didn't notice there was a d7 patch in #15. Marking this for needs review. Thanks for the patches.

dennis_meuwissen’s picture

dennis_meuwissen commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-25.patch.txt10.27 KB

Attached a patch that applies to 1.8.

dennis_meuwissen’s picture

dennis_meuwissen commented
StatusFileSize
new eu_cookie_compliance-names_mixed_up-2969161-25.patch10.27 KB
svenryen’s picture

svenryen commented
Status: Needs review » Fixed

The 7.x patch no longer applies, so I'll go ahead and patch 8.x to resolve this issue.

If anybody feels strongly about the 7.x patch, feel free to reopen the issue with an updated patch.

It's time to give some more love to the 8.x version, and accept that we can't keep adding every feature to the 7.x branch.

  • a3ad136 committed on 8.x-1.x 
    Merge remote-tracking branch 'origin/8.x-1.x' into 8.x-1.x

* origin/8.x...

Status: Fixed » Closed (fixed)

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