- Issue created by @jan kellermann
- First commit to issue fork.
- Merge request !79Add onInit, onAccept and onDecline config keys to the Klaro App Service → (Open) created by kensae
- 🇧🇪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
Updated patch. Passed onInit, onAccept, onDecline to services' config settings.
- 🇩🇪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:
- 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. - 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. - 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.
- Shouldn't we provide a proper update for
- 🇩🇪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.)