Refactor callback mechanism

Created on 31 October 2024, 11 months ago

Problem/Motivation

Since v0.7.10 klaro has own callback functions onAccept, onDecline and onInit. We need this callbacks for Google Consent Mode v2.

Proposed resolution

Add these options to Klaro Apps.

Remaining tasks

  • Create options and change Klaro App Form
  • Mark the existing hook as deprecated (the current callback was called on every change, so we cannot move the existing callback to the new onAccept, onDecline

User interface changes

Settingsform for Klaro Apps.

Data model changes

Config for Klaro Apps.

Feature request
Status

Active

Version

3.0

Component

Code

Created by

🇩🇪Germany jan kellermann

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @jan kellermann
  • First commit to issue fork.
  • Pipeline finished with Success
    7 months ago
    Total: 177s
    #427506
  • 🇧🇪Belgium kensae

    I've added onInit, onAccept and onDecline options to the Klaro App entity and I've updated the form in MR!79

  • 🇲🇾Malaysia ckng

    Tested MR!79 working. Generated the patch file.

  • 🇲🇾Malaysia ckng

    Updated status, as JS logics are missing.

  • 🇲🇾Malaysia ckng

    Updated patch. Passed onInit, onAccept, onDecline to services' config settings.

  • Pipeline finished with Failed
    6 months ago
    Total: 256s
    #444818
  • 🇲🇾Malaysia ckng

    Rebased and rerolled the patch.

  • Pipeline finished with Success
    6 months ago
    Total: 171s
    #444829
  • 🇩🇪Germany jan kellermann

    Big thanks to kensae and ckng. I have to check in the next days and will add some tests for the new options.

  • 🇷🇺Russia niklan Russia, Perm

    I think the update for existing Klaro app entities should also be provided, since the entities now have new properties and the existing configurations are invalid for the new schema. I'm curious about whether this update should be more cautious, considering how long it has been since this issue was created. In its current state, it will overwrite all existing callback data after the update, which could be a bit intrusive. Perhaps only set a default value if no value exists at the time of the update? This way, projects that relied on this merge request until the official release will retain their configuration.

  • 🇩🇪Germany jan kellermann

    I think we cannot write an update hook for old javascript snippets. We have to use both variants in parallel and deprecate the old way in next major release.

  • 🇷🇺Russia niklan Russia, Perm

    Yes, we can't update the existing JS snippet. But, there are other problems related to the new properties:

    1. Shouldn't we provide a proper update for klaro_app entities in order to set default values for newly created properties? I have tested this locally and it looks like Drupal automatically detects this and, on the next configuration export, adds missing properties.
    2. Shouldn't we update the default klaro_app entities (./config/install) to match the schema? Drupal seems to do this at some point, by merging missing properties. But currently, they are becoming out of sync with actual entities, and this might be a problem in certain scenarios.
    3. The description of on_decline should be checked and updated to make it more specific when the callback is called. At the moment, I'm unsure which is correct, but it seems that the behavior is correct, so the description needs to be updated.
  • 🇩🇪Germany jan kellermann

    Thank you, niklan! You are of course absolutely right that we need an update hook for the addition of config entities and that the schema also needs to be updated. (I had understood that we would split the existing scripts into the new areas - which I think is too risky.)

  • I played around with the events and observed the following sequences, starting without the klaro cookie:

    • A: Load: onInit, onDecline, deprecated User declines: onDecline, deprecated
    • B: Load: onInit, onDecline, deprecated User accepts: onAccept, deprecated

    Subsequent page loads:
    Load: onInit, onDecline|onAccept, deprecated

    For me it looks like the behavior is sound, as niklan mentioned in #15.3
    Applying this sequence for googles consent mode, one could use the onInit event to send the default consent state (denied) and then use the onAccept event to communicate a possible consent.

    Regarding the description one could write, that the consent was explicit or also implicit, e.g.:
    This javascript will be executed after the user has explicitly or implicitly (page load) declined the service.

    When it comes to the update script. The cautious approach would be to set the properties only, when not present. Something like:
    $klaro_app->set('on_init', $klaro_app->get('on_init') ?: '');
    right?

    Regarding #15.2: For me this looks like a lot of work as all services would need to be migrated and probably tested. As far as I understand, they would just work like before. Why not adapt them one-by-one afterwards when needed, with people involved, who know these services well and can test them?

  • 🇷🇺Russia niklan Russia, Perm

    Regarding #15.2: For me this looks like a lot of work as all services would need to be migrated and probably tested.

    In 15.2, I've talked only about entities in `./config/install`, which are used for new installations (when the module is installed, like `hook_install()`). Changes here won't affect existing installations. However, it looks like Drupal provides missing data from default values, so it is mitigated in a way.

    Basically, they are pre-exported entities, which are simply outdated from what they actually will be, but since the user will export configuration changes after a new module install regardless, they will be valid ones in the configuration sync directory.

    This javascript will be executed after the user has explicitly or implicitly (page load) declined the service.

    This one is much better, because it doesn't imply that user have to do any action for this event to occur.

  • Status changed to Needs work 3 months ago
  • 🇨🇦Canada seanmacgillivray

    The patch in #13 Refactor callback mechanism Active has an update hook ID collision with another from https://www.drupal.org/project/klaro/issues/3484844 Show title on notice dialog optional Postponed , suggest incrementing it to 10015 or higher.

  • 🇬🇧United Kingdom scott_euser

    Trying to understand how this works, in the deprecated callback its clear

    On load of Klaro, we can take one action or another based on consent.

    But in the new UI proposed by this MR, how would that work. We have these scenarios right?

    1. onInit when the user has already granted consent
    2. onInit when the user has not yet granted consent
    3. onAccept when the user consents during the current page view
    4. onDecline when the user declines consent during the current page view

    Is that right? If so, then how in onInit would we make the correct JS call based on already having accepted that service or not? The old callback form makes that distinction clear with the placeholder + description. Or maybe I misunderstand?

    Thanks!

  • 🇩🇪Germany sascha_meissner Planet earth

    @scott_euser AFAIS,

    - onInit will run on every load regardless of the consent, it is not possible to check to consent state in this callback as klaro is not fully initialized.

    onAccept and onDecline will also run on every load, but based on the (implicit) consent, as well as when consent changes on runtime (explicit).

    So you would have to move your code accordingly, probably you wont need the onInit callback at all.

    Still you are right, the placeholders for the callbacks should be more explanative and should also show the available arguments, because , even though not explicitly mentioned in the upstream klaro-js docs, there are some:

    - onInit (opts) opts.config, opts.service, opts.vars

    - onAccept/onDecline(opts) opts.config, opts.consents, opts.confirmed, opts.service, opts.vars

    // true || false
    klaro.getManager().getConsent(opts.service.name)
    
  • 🇬🇧United Kingdom scott_euser

    Great thanks for the details!

  • Pipeline finished with Success
    3 months ago
    Total: 149s
    #530978
  • Pipeline finished with Canceled
    3 months ago
    Total: 160s
    #530984
  • 🇬🇧United Kingdom scott_euser

    Okay placeholders + field descriptions updated.

    Still in 'Needs work' as per the discussion about update hooks.

  • Pipeline finished with Success
    3 months ago
    Total: 269s
    #530986
  • 🇩🇪Germany sascha_meissner Planet earth

    Thx @scott_euser for your work.
    I did also improve the placeholders, adding available arguments.

    Maybe the description texts still need a little bit work, personally i more like the proposal from #17

  • 🇬🇧United Kingdom dunx

    I just added 'Google Consent Mode v2' to the title. I'd been told there was an issue but was struggling to find it. Hopefully that's useful.

Production build 0.71.5 2024