Closed (fixed)
Project:
EU Cookie Compliance (GDPR Compliance)
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
18 Jul 2019 at 10:57 UTC
Updated:
12 Oct 2020 at 17:49 UTC
Jump to comment: Most recent, Most recent file


Comments
Comment #2
akalam commentedComment #3
svenryen commentedIf 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.
Comment #4
reszliI started working on a better UI for categories (for now without the individual cookie details):
Comment #5
reszlidid the same for D7 as well in https://www.drupal.org/project/eu_cookie_compliance/issues/3137762#comme...
Comment #6
reszliaccidentally left a usage of select_all_categories_by_default setting in the JS code, while the setting was already removed
attaching corrected patch
Comment #7
reszlisome small improvements:
Comment #8
reszliforgot the interdiff above
Comment #9
reszliComment #10
grayle commentedTested 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.
Comment #11
grayle commentedOh, just noticed, description field is required now. It wasn't before, and I don't believe it should be now.
Comment #12
akalam commentedTo 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.
Comment #13
reszliI updated the issue summary and rerolled the patch with description field being optional
Comment #14
reszliComment #15
anybodyReally really great work, thank you all very much!
This also helps to add information about all scripts and cookies used in that group! Wonderful! :)
Comment #16
svenryen commentedCan 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.
Comment #17
svenryen commentedD8: 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?
Comment #18
svenryen commentedIs 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.
Comment #19
bramvandenbulcke commentedI 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:
Comment #20
dmsmidtThanks 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.
Comment #21
dmsmidtI'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
Cookiebot categorie settings
Cookiepro modes
Cookiepro categorie settings
Comment #22
dmsmidtHmm, it even seems pre-checked checkboxes are not allowed anymore.
http://curia.europa.eu/juris/document/document.jsf?text=&docid=218462&pa...
Comment #23
svenryen commented@dmsmidt - Doesn't the implementation of GDPR vary from country to country (when it comes to pre-checked checkboxes)?
Comment #24
anybodyre#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.
Comment #25
dmsmidtI 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.
Comment #26
anybodyThanks @dsmsmidt!
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!
Comment #27
l_vandamme commentedI 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)
Comment #28
svenryen commentedThanks, L_VanDamme. I'll have a look later.
Comment #29
bramvandenbulcke commentedIn 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.
Comment #30
JordiBEI updated the D8 patch with the remarks below:
Included reworked patch & interdiff on #13 (D8 only)!
Comment #31
JordiBERerolled patch because of small problem with '...'
Comment #32
svenryen commentedI have the following problems with the patch in #31:
@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?
Comment #33
neslee canil pintoComment #34
svenryen commentedI 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?
Comment #35
svenryen commentedI 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/translateContent, Interface and Configuration translations are all enabled.
Comment #36
grayle commentedRerolled patch against head of dev, still have to go over everything and change/fix what needs fixing but first things first.
Comment #37
shaktikHi Grayle,
Kindly check below coding standard issue.
It should be FALSE
It should be TRUE
No newline at end of file
it should be one line space here.
Comment #38
grayle commentedYou 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.
Comment #39
grayle commentedComment #40
grayle commentedUpdate to D7 patch:
- change disabled to changeable_state to match D8
Comment #41
grayle commentedUpdated D8 patch. Added newline, reverted 2 changed lines that didn't need to change for this patch.
Comment #43
svenryen commentedLooks 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 :) ).
The default state of the category checkbox. When this is enabled, the checkbox will be shown in a checked state on page load.
Disable tracking by default for this category
Enable tracking by default for this category
Locked 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.
Let visitors change the checkbox state.
Disable the checkbox so that the state can't be changed
Please remove code rather than comment it out. We have the old code in git :)
Cookie categories are managed in the Categories tab (found at the top of this page).
Comment #44
shaktik@svenryen,
I am able to access this page (for translations):
/admin/config/system/eu-cookie-compliance/categories/test/edit/translatemention #35appylyed patch #36.
attached screenshots.

Comment #45
grayle commentedAlright, 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"?
Comment #46
svenryen commentedLooks interesting. Which of these 3 options would have a disabled state?
Comment #47
anybodyShould 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?
Comment #48
svenryen commentedWe 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.
Comment #49
grayle commentedNotice only would have it checked and disabled. Opt in is standard, opt out is just pre checked.
Comment #50
svenryen commentedWe 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.
Comment #51
svenryen commentedHow about simply naming them:
?
Comment #52
grayle commentedSure, 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.
Comment #53
svenryen commentedI think three options is an improvement over the two x two options that are in the patch.
Comment #54
grayle commentedRerolled 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.
Comment #55
svenryen commentedThanks for spotting that. We really should have had some tests in this module.. :)
Comment #56
jamesoakleyLet 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
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
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.
Comment #57
grayle commentedI 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.
Comment #58
svenryen commentedYeah, let's go for implementing the way the checkboxes function (checked, unchecked and so forth). No need to change any variable names.
Comment #59
grayle commentedD8 reroll based on new HEAD of dev + updated labels
D7 in progress
Comment #60
grayle commentedD7 reroll + update to match D8 patch
Comment #61
svenryen commentedThanks! Will take a look tonight or this weekend.
Comment #62
svenryen commentedI think we're nearly there now.
D7 feedback:
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.
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.
Comment #63
svenryen commentedComment #64
svenryen commentedI also can't save an empty GTM data field. I get the error "The GTM data is not a valid JSON.".
Comment #65
grayle commentedWait, what GTM field? There's nothing in this module that directly does anything with GTM is there?
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.
Should be fine. Not 100% sure why it was already gone, but as long as it's gone it's fine.
Comment #66
svenryen commentedMight 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.
Comment #67
grayle commentedIf something breaks with that module, that's fine. I'm maintainer there too, so I'll fix whatever needs fixing on that end.
Comment #68
grayle commentedAnd 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?
Comment #69
svenryen commentedDo you have steps for this?
Comment #70
svenryen commentedCorrect 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:
Comment #71
svenryen commentedAlso, 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.Comment #72
grayle commentedA 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.
Comment #73
svenryen commentedOn 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.Comment #74
svenryen commentedYou have a conflict with the
variablemodule. If I rundrush dis variable, the table populates with the categories I've created. So can you look at the original code that works well withvariableinstalled and improve the patch so that we can still supporti18nwebsites?Comment #75
grayle commentedHolding 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.
Comment #76
grayle commentedAnd 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.ymland set status to 0Create a new yml file called
domain.config.<domain_machine_name>.eu_cookie_compliance.cookie_category.catb.ymlEmpty it out except for
status, which you set to 1Import 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.
Comment #77
reszlifor 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
Comment #78
grayle commentedComment #79
neslee canil pinto@Grayle as per i can understand it is "whether" and not "weather". Correct me if i am wrong.
Comment #80
grayle commentedComment #81
svenryen commentedComment #85
svenryen commented