Summary

The new feature for cookie categories is great, and let sites to be GDPR compliant. However, there is still work to do to ensure it works for users.

  • From the administrator point of view, we need a way to manage individual categories with a good UX.
  • From a developers point of view, the information should be stored in the eu_cookie_compilance.settings configuration entity (as now it does), but in a structured way (now is just text that need to be split by line or by pipe)

Proposal for the user experience

organize categories
add edit categories

Solution

  • Evolve the admin form to allow to add/edit/delete separately the different Cookie categories
  • Evolve the eu_cookie_compliance.settings configuration entity to store the cookie categories information in a structured way.
  • Refactor the places where cookie compliance categories are read to adapt they to the new format.
CommentFileSizeAuthor
#80 D7-cookie_categories_ui-3068670-80.patch36.48 KBgrayle
#80 D7-cookie_categories_ui-3068670-interdiff.78-80.txt519 bytesgrayle
#78 D7-cookie_categories_ui-3068670-75-78.interdiff.txt6.07 KBgrayle
#78 D7-cookie_categories_ui-3068670-78.patch36.47 KBgrayle
#76 D8-cookie_category_ui-3068670-75-76.interdiff.txt1.39 KBgrayle
#76 D8-cookie_category_ui-3068670-76.patch37.44 KBgrayle
#75 D7-cookie_categories_ui-3068670-75.patch35.38 KBgrayle
#75 D8-cookie_category_ui-3068670-75.patch36.88 KBgrayle
#62 Screen Shot 2020-09-04 at 19.38.33.png38.71 KBsvenryen
#60 D7-cookie_categories_ui-3068670-60.patch34.38 KBgrayle
#59 D8-cookie_category_ui-3068670-59.patch37.67 KBgrayle
#54 D8-cookie_category_ui-3068670-54.patch39.54 KBgrayle
#54 D8-cookie_category_ui-3068670-41-54.interdiff.txt28.67 KBgrayle
#45 ux-45.png32.5 KBgrayle
#44 Screen Shot 2020-09-01 at 15.41.16.png71.2 KBshaktik
#44 Screen Shot 2020-09-01 at 15.40.15.png38.29 KBshaktik
#41 D8-cookie_category_ui-3068670-36-41.interdiff.txt526 bytesgrayle
#41 D8-cookie_category_ui-3068670-41.patch34.59 KBgrayle
#40 D7-cookie_categories_ui-3068670-38-40.interdiff.txt4.49 KBgrayle
#40 D7-cookie_categories_ui-3068670-40.patch33.43 KBgrayle
#38 D7-cookie_categories_ui-3068670-38.patch33.3 KBgrayle
#36 D8-cookie_category_ui-3068670-36.patch35.03 KBgrayle
#31 D8-cookie_category_ui-3068670-31-diff.patch36.92 KBJordiBE
#30 D8-cookie_category_ui-3068670-30-interdiff.patch7.06 KBJordiBE
#30 D8-cookie_category_ui-3068670-30-diff.patch36.92 KBJordiBE
#27 D7-cookie_categories_ui-3068670-13-27-interdiff.patch3.73 KBl_vandamme
#27 D7-cookie_categories_ui-3068670-27.patch29.83 KBl_vandamme
#21 Cookiepro-categorie-settings.jpg171.52 KBdmsmidt
#21 Cookiepro-modes.jpg319.32 KBdmsmidt
#21 Cookiebot-categorie-settings.jpg61.91 KBdmsmidt
#21 Coockiebot-modes.jpg288.06 KBdmsmidt
#13 interdiff-9-13.txt572 bytesreszli
#13 cookie_categories_ui-3068670-13.patch36.01 KBreszli
#13 D7_interdiff-9-13.txt586 bytesreszli
#13 D7-cookie_categories_ui-3068670-13.patch27.67 KBreszli
#13 add-edit-categories.png72.18 KBreszli
#13 organize-categories.png132.31 KBreszli
#9 cookie_categories_ui-3068670-9.patch36.01 KBreszli
#9 D7-cookie_categories_ui-3068670-9.patch27.67 KBreszli
#8 interdiff_6-7.txt1.55 KBreszli
#7 cookie_categories_ui-3068670-7.patch35.92 KBreszli
#6 cookie_categories_ui-3068670-5.patch35.85 KBreszli
#4 cookie_categories_ui-3068670-4.patch35.22 KBreszli
bda91371e0e6428a9625c75593d8fdc5.jpeg25.68 KBakalam

Comments

akalam created an issue. See original summary.

akalam’s picture

Issue summary: View changes
svenryen’s picture

If this gets implemented, we need to either convert the current settings, or keep using the current setting format for cookie categories so that we don't break existing sites that have adopted this feature.

It would also be nice if we can commit the code to both D7 and D8 versions of the module when a patch is ready and tested.

reszli’s picture

StatusFileSize
new35.22 KB

I started working on a better UI for categories (for now without the individual cookie details):

  • created cookie_category config entity, introduced default_state and disabled flags for each category
  • implemented hook_update_N to migrate existing string config to new config entity
  • created admin UI for CRUD and for ordering categories
  • split permissions and menus to allow management of categories only, without having access to popup behaviour
  • rewired front-end to use new config
reszli’s picture

reszli’s picture

StatusFileSize
new35.85 KB

accidentally left a usage of select_all_categories_by_default setting in the JS code, while the setting was already removed
attaching corrected patch

reszli’s picture

StatusFileSize
new35.92 KB

some small improvements:

  • removed category checkbox default value from template - to allow setting it dynamically via JS
  • also exposed the entire list of cookie categories (with all attributes) to be available at drupalSettings.eu_cookie_compliance.cookie_categories_details
reszli’s picture

StatusFileSize
new1.55 KB

forgot the interdiff above

reszli’s picture

StatusFileSize
new27.67 KB
new36.01 KB
  • combined the D7 patch into the same issue to ease the process
  • also reverted the last change re. checkbox default value on D8
grayle’s picture

Status: Needs review » Reviewed & tested by the community

Tested it on a fresh install as well as an upgrade, both work.

I set it to RTBC but technically this patch doesn't include the full scope yet. We should decide if we want to reduce the initial scope and create a followup or add everything in one go.

grayle’s picture

Status: Reviewed & tested by the community » Needs work

Oh, just noticed, description field is required now. It wasn't before, and I don't believe it should be now.

akalam’s picture

To be honest, I created the issue with the aim to implement it by myself, but sadly I had no time for this issue, so I'm very happy reszli could work on it and create a patch. thank you!

I agree with the idea of reducing the initial scope. As long as we are improving the current interface it will be great to stabilize the patch and move it forward with the maintainers agreement, and then create new issues to iterate over it and continue improving.

reszli’s picture

Title: Improve the UX for cookie categories and allow entering information for individual cookie descriptions » Improve the UX for cookie categories
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new132.31 KB
new72.18 KB
new27.67 KB
new586 bytes
new36.01 KB
new572 bytes

I updated the issue summary and rerolled the patch with description field being optional

reszli’s picture

Issue summary: View changes
anybody’s picture

Really really great work, thank you all very much!
This also helps to add information about all scripts and cookies used in that group! Wonderful! :)

svenryen’s picture

Can we make one small UI improvement, so that the Categories page is easier to find?

It would be great if the Categories page can have a tab making it possible to easily tab between Settings and Categories.

Also, can we add text saying "See separate Categories tab for managing cookie categories themselves." in D8?

Are you okay to add this, @reszli, otherwise I can fix it next week.

svenryen’s picture

Status: Needs review » Needs work

D8: For a multilingual site, I have the option to translate my categories in the dropdown, but the page throws an error:
/admin/config/system/eu-cookie-compliance/categories/cat1/edit/translate
I have no permission to view the page with uid 1.

D7: How do we translate the categories? And did we lose the option to localize the settings page through i18n_variable or am I looking in the wrong place?

svenryen’s picture

Is there also a conflict with #3109907: Consider zero categories as non-agreed? I tried to select no category, and the status is still stored as "accepted" in the cookie.

Try these steps to reproduce:
1. Delete all cookies for the domain
2. Alternatively, set up banner to show Thank you notice
3. Select no categories in the banner
4. Click "Save preferences"

Result:
The thank you banner now pops up again and the cookie status is 2.

Expected:
Cookie status should be null when no categories are selected and thank you banner should not be seen.

bramvandenbulcke’s picture

I tested the latest patch on a fresh Drupal 8 install and also on an existing Drupal 8 multilingual website.

The functionality is working well. The update from a prior version also worked without a problem. Great work!

I have some remarks from a user perspective:

  • The link to the page with categories is hard to find. I think lots of users won't find the page with the categories. It has the same depth as the general EU Cookie Compliance settings page. You can either opt for a tab, as proposed by svenryen; a link at the end of the line "Opt-in with categories" (under Consent for processing of personal information) could also work; or finally an integration on the page itself could also work (integrate the new way to administer cookie categories on the settings pages).
  • The translation options on a multilingual setup will be harder to find for an end user, because it's handled differently. Before this patch, all fields could be translated in the tab 'Translate eu cookie compliance'. The new category fields don't show up there. These fields can be translated on the field level instead (Operations > Translate (via the dropdown)). I believe this will lead to confusion for end users; I didn't find this option at first. I didn't have problems with translating the fields though.
dmsmidt’s picture

Thanks for working on this improvement and the screenshots.

Aside from a technical review I'll give some some other possible improvements.

Be consistent with the word 'cookie', sometimes it starts with a capital sometimes is doesn't.

From a legal standpoint users give consent for certain types or are informed without being able to deny consent (e.g. sometimes anonymous statistics).

So the wording 'disable/enable tracking' is misleading. Only certain categories allow for tracking. Functional and anonymous statistics cookies don't track. An improvement would be: 'Default consent action: confirm/decline'.

If you don't allow people to change the state, you are not asking for consent, you are just informing. So the second enable/disable could work a bit different.

'Cookie category type: informative / ask for consent'

If the choice is 'informative' the user doesn't even need to give a default state and we can hide the option.

I've no legal background or native tongue (just done a lot of research), so I'll try to see what other services use as an example.

dmsmidt’s picture

StatusFileSize
new288.06 KB
new61.91 KB
new319.32 KB
new171.52 KB

I'm not sure how this module currently handles different modes of operation, it has been a while since I checked. But here are some examples by professional services.

Coockiebot modes
Coockiebot modes

Cookiebot categorie settings
Cookiebot categorie settings

Cookiepro modes
Cookiepro modes

Cookiepro categorie settings
Cookiepro categorie settings

dmsmidt’s picture

Hmm, it even seems pre-checked checkboxes are not allowed anymore.

http://curia.europa.eu/juris/document/document.jsf?text=&docid=218462&pa...

svenryen’s picture

@dmsmidt - Doesn't the implementation of GDPR vary from country to country (when it comes to pre-checked checkboxes)?

anybody’s picture

re#22+#23: @smsmidt and @svenryen:
While it may depend on country laws and court decisions, customers may want to use prechecked checkboxes, even if it's not legal or legality is unclear. At least for "technical cookies" it's definitely allowed and best practise and I think we should see the technical options but pre-check them according to GDPR best practices.

So what I'd suggest here is to let the page owner decide and implement the options technically. If we're more than technical, we may add a sentence like "Pre-checking a non-technical group may conflict with GDPR" and add a link.

Furthermore I experience that GDPR is handled more or less strict from country to country and customers sometimes decide against full GDPR compliance to collect more important data.

dmsmidt’s picture

I agree that we leave the option in, all vendors of consent managers seem to do (see #21) and we probably don't have the profession to make a legal point here ;-)

@Anybody, anonymous statistics is also allowed, even without the possibility to deny it, as long as you inform the user (according to a Dutch lawyer I bothered with questions).

Still, if we are improving the UX, we should work on our wording of things.

anybody’s picture

Thanks @dsmsmidt!

@Anybody, anonymous statistics is also allowed, even without the possibility to deny it, as long as you inform the user (according to a Dutch lawyer I bothered with questions).

Yes! If setting a cookie, this probably isn't allowed in Germany due to curt decisions. So it seems it depends on countries / court decisions. If not using a cookie (which cookie compliance has in name) or similar technology it seems to be allowed to still track the user under certain restrictions server-side for example. But that's out of focus here.

What's even more in focus is to prevent the loading of third-party / external content completely if no consent is given for that category because it may expose user information to that third party. Any if it's allowed or not, we should be user-friendly here :)

I totally agree on the importance of wording!

l_vandamme’s picture

I updated the D7 patch because the categories were not translatable anymore since changing the variable name.
This update uses the variable info hook to register the new eu_cookie_compliance_categories variable for translation and updates the category related forms to use realm variables.

(also included an interdiff between #13 and my reworked patch)

svenryen’s picture

Thanks, L_VanDamme. I'll have a look later.

bramvandenbulcke’s picture

In addition to the remarks of @dmsmidt: I did another test today with the Drupal 8 version and I was confused with the naming on the table under cookie categories (see the first screenshot of this post right under "Proposal for the user experience").

At the end of the table we have "default" (on/off) and "disabled" (yes/no). I would propose to change this to "default state" (on/off) and "changeable state" (yes/no). I guess you have to reverse the yes/no output. Especially "Disabled" was confusing to me.

Maybe a native English speaker has a better proposal.

JordiBE’s picture

I updated the D8 patch with the remarks below:

  • Tabs implemented for easier switching by svenryen #16
  • Message / info about cookie category tab by svenryen #16
  • Change disabled to changeable suggested by bramvandenbulcke #29

Included reworked patch & interdiff on #13 (D8 only)!

JordiBE’s picture

StatusFileSize
new36.92 KB

Rerolled patch because of small problem with '...'

svenryen’s picture

I have the following problems with the patch in #31:

  • BadMethodCallException: The "\Drupal\eu_cookie_compliance\Controller\AdminController::access" method is not callable as a _custom_access callback in route "/admin/config/system/eu-cookie-compliance" in Drupal\Core\Access\CustomAccessCheck->access() (line 69 of core/lib/Drupal/Core/Access/CustomAccessCheck.php). This can be temporarily fixed by modifying requirements in the .routing.yml file.
  • Symfony\Component\Routing\Exception\RouteNotFoundException: Route "entity.cookie_category.collection" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 208 of core/lib/Drupal/Core/Routing/RouteProvider.php).

@JordiBE or @L_VanDamme, could you take a look and see whether everything needed is in the patch file, or if something is broken in the latest -dev that we need to fix?

neslee canil pinto’s picture

Issue tags: +Needs reroll
svenryen’s picture

I now applied the patch in #31 again, and noticed it had expanded some files in the wrong folder when I pasted it into PhpStorm.

I think the following options should be removed from the module settings page:
- Tick the first checkbox and mark it read only.
- Tick all category checkboxes by default.

Are we still doing the D7 port?

svenryen’s picture

I still have the problem where UID 1 isn't able to access this page (for translations):
/admin/config/system/eu-cookie-compliance/categories/test/edit/translate

Content, Interface and Configuration translations are all enabled.

grayle’s picture

StatusFileSize
new35.03 KB

Rerolled patch against head of dev, still have to go over everything and change/fix what needs fixing but first things first.

shaktik’s picture

Hi Grayle,

Kindly check below coding standard issue.

  1. +++ b/config/install/eu_cookie_compliance.settings.yml
    @@ -58,12 +58,9 @@ withdraw_message:
    -select_all_categories_by_default: false
    

    It should be FALSE

  2. +++ b/config/install/eu_cookie_compliance.settings.yml
    @@ -58,12 +58,9 @@ withdraw_message:
    -fix_first_cookie_category: true
    

    It should be TRUE

  3. +++ b/eu_cookie_compliance.install
    @@ -450,3 +450,63 @@ function eu_cookie_compliance_update_8120() {
    \ No newline at end of file
    

    No newline at end of file
    it should be one line space here.

grayle’s picture

StatusFileSize
new33.3 KB

You want me to use uppercase YAML booleans, which afaik require expanded, non-standard decoding interpretation, on lines that got removed in the patch? That'll be tricky ;)

I will add the newline though.

--------------

In any case, here's the rerolled and improved D7 patch for categories. Tabs added, multilingual support added.

grayle’s picture

grayle’s picture

StatusFileSize
new33.43 KB
new4.49 KB

Update to D7 patch:
- change disabled to changeable_state to match D8

grayle’s picture

Status: Needs work » Needs review
StatusFileSize
new34.59 KB
new526 bytes

Updated D8 patch. Added newline, reverted 2 changed lines that didn't need to change for this patch.

Status: Needs review » Needs work

The last submitted patch, 41: D8-cookie_category_ui-3068670-41.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

svenryen’s picture

Looks good on visual inspection. I tried to improve the interface labels since there was some feedback on that further up in the issue thread. (But I feel we should have a native English speaker help us here :) ).

  1. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +      '#description' => $this->t("The default state, either enable or disable the tracking by default."),
    

    The default state of the category checkbox. When this is enabled, the checkbox will be shown in a checked state on page load.

  2. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +        0 => $this->t('Disable tracking'),
    

    Disable tracking by default for this category

  3. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +        1 => $this->t('Enable tracking')
    

    Enable tracking by default for this category

  4. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +      '#title' => $this->t('Changeable state'),
    

    Locked state

  5. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +      '#description' => $this->t("This will determine if the visitor can change the category's state or it must accept the default state."),
    

    Enable this option to lock the state of the category checkbox. When enabled, the visitor needs to accept the cookies and scripts configured for the category.

  6. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +        1 => $this->t('Visitor will be able to change the state.'),
    

    Let visitors change the checkbox state.

  7. +++ b/src/Form/CookieCategoryForm.php
    @@ -0,0 +1,136 @@
    +        0 => $this->t('Visitor will not be able to change the state.'),
    

    Disable the checkbox so that the state can't be changed

  8. +++ b/src/Form/EuCookieComplianceConfigForm.php
    @@ -211,12 +211,18 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
    +//    $form['consent_per_category']['cookie_categories'] = [
    

    Please remove code rather than comment it out. We have the old code in git :)

  9. +++ b/src/Form/EuCookieComplianceConfigForm.php
    @@ -211,12 +211,18 @@ class EuCookieComplianceConfigForm extends ConfigFormBase {
    +      '#markup' => $this->t("See separate Categories tab for managing cookie categories themselves"),
    

    Cookie categories are managed in the Categories tab (found at the top of this page).

shaktik’s picture

@svenryen,

I am able to access this page (for translations): /admin/config/system/eu-cookie-compliance/categories/test/edit/translate mention #35
appylyed patch #36.

attached screenshots.

grayle’s picture

StatusFileSize
new32.5 KB

Alright, we may as well steal some stuff from others who have come before us. A proposal, based on some discussions higher up the thread.

I left out "Implied Consent" because afaik that doesn't fit. Unless we want to call "Notice Only" "Implied Consent"? Visitors still have to click accept even with "notice only". Perhaps "Consent Required"?

UX proposal

svenryen’s picture

Looks interesting. Which of these 3 options would have a disabled state?

anybody’s picture

Should we perhaps split this issue into a clean plan for 2.0.x admin interface / functionality for cookie categories and a plan for 1.x (if wanted) based on the patches above?

svenryen’s picture

We definitely want this in 1.x since there's a lot of effort invested in the 7.x version. Also since this is sponsored by a vendor it makes a lot of sense to commit it and tag a new release once the bug reports are sorted.

grayle’s picture

Notice only would have it checked and disabled. Opt in is standard, opt out is just pre checked.

svenryen’s picture

We already have methods for cookie compliance on the settings page.

Would it not confuse people if you use the same terms for cookie categories?

I think we should rephrase "Notice only" since it gives no clue that this option actually represents an enforced option.

svenryen’s picture

How about simply naming them:

  • Checked
  • Unchecked
  • Checked and disabled

?

grayle’s picture

Sure, but if we're sticking to checkbox descriptions purely then I'd keep what we have now in the patch and I'll update the labels in the current patch.

svenryen’s picture

I think three options is an improvement over the two x two options that are in the patch.

grayle’s picture

Rerolled D8 patch against new HEAD of dev, added default checkbox state field to replace the 2x2 option from before, actually added an update hook to install the config entity which was apparently missing, made the custom service storage manager the actual storage manager because that just seemed neater and hopefully didn't miss any of the feedback.

Also found out why zero categories seems to work as accepted: it works if you use the "accept" button but if you switch it out to "accept all" and "save preferences", saving preferences without any categories selected still saves it as normal. I'll make a new issue to address that, it's also "broken" in the current dev/release.

svenryen’s picture

Thanks for spotting that. We really should have had some tests in this module.. :)

jamesoakley’s picture

Let me add my thoughts.

I think we need to decide if the settings form labels will describe the way the checkboxes will function, or the way the website will function. Clearly these two are interlinked, but some of the suggestions above switch between describing checkbox properties and website functionality, which is then confusing. (For example, as I'll illustrate below, what exactly is "disabled"?)

So I think the following labels are clear.

... the way the checkboxes function

  • checked by default
  • unchecked by default
  • checked and disabled (user cannot clear the checkbox)

To explain, I think "by default" needs adding to "checked" / "unchecked", to make clear that we're simply describing how the checkbox is presented initially, not how the checkbox must remain.

I also think "disabled" needs some elaboration (hence "user cannot clear the checkbox" in parentheses), because otherwise first reading could give the impression cookies rather than [HTML INPUT type=checkbox] controls being disabled. It's back to the need to make clear that we're describing checkboxes here, not website / cookie behaviour.)

Or...

... the way the website behaves

  • Opt In (cookies are disabled until the user chooses to enable them)
  • Opt Out (cookies are enabled unless the user chooses to disable them)
  • Opted In - with no facility to opt out

The first two options are self-explanatory, although note again that the parenthesised explanations are careful to talk about the user enabling / disabling "them" (the cookies), not "it" (the checkbox), because we're consistently talking website behaviour not checkbox properties.

The last option is, I personally think, clearer than "notice only", only because I have to think two or three times to figure out what "notice only" means, and if we're describing website behaviour the website doesn't "notice" anything. The website behaviour, with this choice, is that cookies are permanently enabled.

Just my 2p.

grayle’s picture

I just refactored the patch to be based on explaining the way the checkboxes work, so I'd like to keep it that way. Also avoids double-dipping in the same well of terms that are already being used on the main settings page, as sven brought up.

I'll update the labels and descriptions to be clearer once a final decision has been made. Will change nothing about the functionality, assuming I don't have to change config keys around and we're just changing labels and descriptions.

svenryen’s picture

Yeah, let's go for implementing the way the checkboxes function (checked, unchecked and so forth). No need to change any variable names.

grayle’s picture

StatusFileSize
new37.67 KB

D8 reroll based on new HEAD of dev + updated labels

D7 in progress

grayle’s picture

Status: Needs work » Needs review
StatusFileSize
new34.38 KB

D7 reroll + update to match D8 patch

svenryen’s picture

Thanks! Will take a look tonight or this weekend.

svenryen’s picture

StatusFileSize
new38.71 KB

I think we're nearly there now.

D7 feedback:

+++ b/js/eu_cookie_compliance.js
@@ -509,10 +509,6 @@
-    if (Drupal.eu_cookie_compliance.fix_first_cookie_category && !$.inArray(allCategories[0], _euccSelectedCategories)) {

I can't find the two hunks that you're removing in the patch, so I'm simply ignoring this. Should they have been in a different patch/issue?

I added one category in Drupal 7, and saved. The category does not show up in the list of categories. See screenshot.

Showing a missing category

D8 feedback:

It seems that since we're dealing with entities, sites that use Domain Access can no longer have per domain categories, right?
After upgrading, my GTM data field says 'null' for both categories. Because of this I have to flag the issue as "Needs work".

Patches didn't apply cleanly, but that's a minor issue. It was easy enough to sort out.

svenryen’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll
svenryen’s picture

I also can't save an empty GTM data field. I get the error "The GTM data is not a valid JSON.".

grayle’s picture

After upgrading, my GTM data field says 'null' for both categories. Because of this I have to flag the issue as "Needs work".

I also can't save an empty GTM data field. I get the error "The GTM data is not a valid JSON.".

Wait, what GTM field? There's nothing in this module that directly does anything with GTM is there?

I added one category in Drupal 7, and saved. The category does not show up in the list of categories. See screenshot

I can't reproduce this at the moment. New categories show up for single language sites, translation enabled sites, etc. If translations are enabled, it will create a category only for the language you're working in, seeing as the variable values are different per language. Afaik this is the same functionality as before, where you could define categories per language by translating it.

I can't find the two hunks that you're removing in the patch, so I'm simply ignoring this. Should they have been in a different patch/issue?

Should be fine. Not 100% sure why it was already gone, but as long as it's gone it's fine.

svenryen’s picture

Wait, what GTM field? There's nothing in this module that directly does anything with GTM is there?

Might be because I had the EU Cookie Compliance GTM module enabled. Just ignore that.

I'll have to try again with Drupal 7. I didn't really debug it.

grayle’s picture

If something breaks with that module, that's fine. I'm maintainer there too, so I'll fix whatever needs fixing on that end.

grayle’s picture

And for domain access in D8, it's still configuration so I think you can still define categories per domain.

If you're debugging D7 and it happens again, the only thing I can think of is the variable realm alter in .module (one of the last functions there) where I re-order the submits so ours happens first (where the data gets massaged into a structure that system_settings_form can handle, and thus by extension i18n variable can handle).

I use a hook_implements_alter to make sure we go last, or at least after variable realm. But maybe another module is breaking that, inserting yet another submit?

svenryen’s picture

And for domain access in D8, it's still configuration so I think you can still define categories per domain.

Do you have steps for this?

svenryen’s picture

Correct me if I'm really wrong here, but in eu_cookie_compliance_admin_category_form_submit(), where do you actually save the category?

Here's a list of my enabled modules, in case you need that in order to reproduce:

 Administration                       Navbar (navbar)                              Module  7.x-1.7     
 Chaos tool suite                     Chaos tools (ctools)                         Module  7.x-1.14    
 Core                                 Aggregator (aggregator)                      Module  7.67        
 Core                                 Block (block)                                Module  7.67        
 Core                                 Color (color)                                Module  7.67        
 Core                                 Comment (comment)                            Module  7.67        
 Core                                 Contextual links (contextual)                Module  7.67        
 Core                                 Database logging (dblog)                     Module  7.67        
 Core                                 Field (field)                                Module  7.67        
 Core                                 Field SQL storage (field_sql_storage)        Module  7.67        
 Core                                 Field UI (field_ui)                          Module  7.67        
 Core                                 File (file)                                  Module  7.67        
 Core                                 Filter (filter)                              Module  7.67        
 Core                                 Help (help)                                  Module  7.67        
 Core                                 Image (image)                                Module  7.67        
 Core                                 List (list)                                  Module  7.67        
 Core                                 Locale (locale)                              Module  7.67        
 Core                                 Menu (menu)                                  Module  7.67        
 Core                                 Node (node)                                  Module  7.67        
 Core                                 Number (number)                              Module  7.67        
 Core                                 Options (options)                            Module  7.67        
 Core                                 Path (path)                                  Module  7.67        
 Core                                 RDF (rdf)                                    Module  7.67        
 Core                                 Search (search)                              Module  7.67        
 Core                                 Shortcut (shortcut)                          Module  7.67        
 Core                                 Syslog (syslog)                              Module  7.67        
 Core                                 System (system)                              Module  7.67        
 Core                                 Taxonomy (taxonomy)                          Module  7.67        
 Core                                 Text (text)                                  Module  7.67        
 Core                                 User (user)                                  Module  7.67        
 Domain Access                        Domain Access (domain)                       Module  7.x-3.16    
 Location                             Smart IP (smart_ip)                          Module  7.x-2.53    
 Multilingual - Internationalization  Internationalization (i18n)                  Module  7.x-1.26    
 Multilingual - Internationalization  Variable translation (i18n_variable)         Module  7.x-1.26    
 Other                                Entity API (entity)                          Module  7.x-1.9     
 Other                                EU Cookie Compliance (eu_cookie_compliance)  Module              
 Other                                Focal Point (focal_point)                    Module  7.x-1.1     
 Other                                Jquery Colorpicker (jquery_colorpicker)      Module  7.x-1.3     
 Other                                Libraries (libraries)                        Module  7.x-2.5     
 Other                                Pixel Alter (pixel_alter)                    Module              
 Other                                Shadowbox (shadowbox)                        Module  7.x-3.0-rc2 
 Performance and scalability          Authcache (authcache)                        Module  7.x-2.3     
 Statistics                           Facebook Pixel (facebook_pixel)              Module  7.x-1.1     
 User interface                       jQuery Update (jquery_update)                Module  7.x-2.7     
 Variable                             Variable (variable)                          Module  7.x-2.5     
 Variable                             Variable realm (variable_realm)              Module  7.x-2.5     
 Variable                             Variable store (variable_store)              Module  7.x-2.5     
 Views                                Views (views)                                Module  7.x-3.23    
 Views                                Views UI (views_ui)                          Module  7.x-3.23    
svenryen’s picture

Also, after I run your updb, the settings on the EUCC Configuration page seem to be wiped, and I have problems saving even that page. I understand you can't reproduce this, but I'm seeing it every time, so even though this may be interference with some other module, chances are high other users may run into the same situation. Let me know if you'd like a DB dump in order to help you reproduce this.

grayle’s picture

A db dump can't hurt.

And I don't save it, system settings form saves it. Our submit happens before that one, and all that happens in ours is massaging of form values.

svenryen’s picture

On line 89, foreach (element_children($form['categories']) as $machine_name) {, the variable $form['categories'] is empty. The variable is being stored, but it's not rendered in the table.

svenryen’s picture

You have a conflict with the variable module. If I run drush dis variable, the table populates with the categories I've created. So can you look at the original code that works well with variable installed and improve the patch so that we can still support i18n websites?

grayle’s picture

StatusFileSize
new36.88 KB
new35.38 KB

Holding off on debugging after a quick talk on Slack, it may be the patch didn't apply correctly.

In the mean time, another round of rerolls to match new HEAD of dev.

grayle’s picture

And an update to the D8 patch so people can use categories per domain.

There's no UI for it, because Domain Config UI doesn't support non-basic settings forms, but Domain Access is not a simple module to begin with.

1. Create all categories you'll be needing, for all domains
2. Export configuration
3. Set the exported cookie category statuses to 0 for all those you don't need on your main domain
4. Follow the steps in the README in domain_config to create overrides for the ones you need for your other domains and override the status to be active.
5. Import configuration

Example:

Create CatA and CatB.
Export
Edit eu_cookie_compliance.cookie_category.catb.yml and set status to 0
Create a new yml file called domain.config.<domain_machine_name>.eu_cookie_compliance.cookie_category.catb.yml
Empty it out except for status, which you set to 1
Import everything

On your normal, main domain, you'll only see CatA.
On your other domain, the one you created the override for, you'll see CatA and CatB.

You could also create an override for CatA, to disable it on the other domain, in which case only CatB will show up on the other domain.

Seeing as it's, in my opinion, an edge case for an already somewhat edge case and advanced module (3.9k D8 installs) I think this level of support is currently fine.

If we want to enhance the UX in the future, we can create a submodule that will add a reference field to the cookie category config form and save it in third party settings (the way eucc gtm adds GTM info currently) and use that to alter the status of the config entities on the fly in hook_entity_type_load() and update the cacheability to include the site.url cache context there as well.

That way you could edit a category, and pick for which domains it should be used.

D7 still works as before afaik, all the categories are still in one big variable, just a different big variable from the rest of the settings.

reszli’s picture

for D7, the "Add new category" button (at the top of the page and in the empty table) does not support the i18n realm query param

grayle’s picture

Status: Needs work » Needs review
StatusFileSize
new36.47 KB
new6.07 KB
neslee canil pinto’s picture

Status: Needs review » Needs work

@Grayle as per i can understand it is "whether" and not "weather". Correct me if i am wrong.

+ * @return bool
+ *   Weather it is already in user or not.
+ */
grayle’s picture

Status: Needs work » Needs review
StatusFileSize
new519 bytes
new36.48 KB
svenryen’s picture

Status: Needs review » Reviewed & tested by the community

  • svenryen committed 4747587 on 7.x-1.x authored by Grayle
    Issue #3068670 by Grayle, reszli, JordiBE, L_VanDamme, dmsmidt, shaktik...

  • svenryen committed 8dd4557 on 8.x-1.x authored by Grayle
    Issue #3068670 by Grayle, reszli, JordiBE, L_VanDamme, dmsmidt, shaktik...

  • svenryen committed 4747587 on 7.x-2.x authored by Grayle
    Issue #3068670 by Grayle, reszli, JordiBE, L_VanDamme, dmsmidt, shaktik...
  • svenryen committed 87ed5ea on 7.x-2.x
    Merge branch '7.x-1.x' into 7.x-2.x
    
    * 7.x-1.x: (26 commits)
      Issue #...
svenryen’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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