🇨🇦Canada @nbeaucage

To add a bit of context to this, the original code below:

$this->configuration['use_default_markers'] = $values['use_default_markers'] ?? TRUE;

would assign the integer value 1 instead of the boolean value TRUE if the use_default_markers value was checked/selected, which would then cause the submit operation to fail since the configuration object excepts a boolean value, as per the defined schema in:

modules/ckeditor5_plugin_pack_highlight/config/schema/ckeditor5_plugin_pack_highlight.schema.yml

ckeditor5.plugin.ckeditor5_plugin_pack_highlight__highlight:
  type: mapping
  label: Custom markers
  mapping:
    options:
      type: sequence
      label: 'Markers'
    use_default_markers:
      type: boolean
      label: Use default markers

nbeaucage → made their first commit to this issue’s fork.

I'm not sure how crediting works, lemme try this.

Thanks! Releases should be updated soon.

I would like to propose we simply remove the superfluous getter and setter within PrimitiveBase. This would solve the adjustments requested by this issue. I took the liberty of reverting the previous commit, and having only the getter and setter removed. Please feel free to revert my revert if that should not have been done.

The rework related to having a base $value property within the TypedData base class can be addressed in the other related issue ( https://www.drupal.org/project/drupal/issues/3385629 📌 Useless method overrides in \Drupal\Core\TypedData\PrimitiveBase and missing property definition in TypedData Needs work ), since it might be more complex than the fix that was proposed here.

For example, the \Drupal\Core\TypedData\Plugin\DataType\Map class, which also extends TypedData, does not possess nor makes use of a $value property, but instead uses a $values property (plural, since it's a key/value store of data).

So we can't really add a $value property to the base TypedData class, since it is not relevant to all of its extending children. Also note the comment at the top of the TypedData class:

/**
* The abstract base class for typed data.
*
* Classes deriving from this base class have to declare $value
* or override getValue() or setValue().
*
* @ingroup typed_data
*/

In our current case, PrimitiveBase declares the $value property as instructed, while Map overrides the ::getValue() method from TypedData. Both solutions are following the defined instructions, so I would leave them as-is within this specific issue, while they could be addressed in the related issue instead.

What I usually do when implementing base classes for my teams is the following (which I am 100% open to discuss if it is a viable solution or not):

• Have a public getter method declared in an Interface (e.g. TypedDataInterface)
• Implement said public getter method in a base, abstract class (e.g. TypedData)
• No property defined in the base abstract class, but instead declare an abstract protected intermediary getter method (e.g. ::getValueProperty())
• The public getter method implemented in the base abstract class only calls the abstract protected intermediary getter method, not a property
• Thus, children of the base abstract class MUST implement the protected intermediary getter method, and customize it as needed
• In our case, PrimitiveBase would then declare its own $value property, since it is needed in its context, and then return it in the protected getter function
• The same logic can be used for the public and abstract protected setter methods, basically replacing $this->value by something such as $this->getValueProperty()

Anyway, those are my two cents!

Now, I am not sure what to do regarding the PHPStan baseline (where adjustments/updates should be made).

The test pipeline seems to be failing without much information, so I'll leave this as "Needs work" for now.

Also I'm not sure if this requires a Change Record, feel free to ask for one if needed.

I'll be working on this.

This happened to us just now, and we found out that our container configuration was not properly setting the user_role condition.

Previously we were matching the anonymous user role only (for the condition), but since upgrading to the 2.x version of the module, the negate sub-property was set to true instead of our initial/expected false value.

So instead of having GTM fire off for anonymous users, it was firing for all authenticated users.

Setting the conditions.user_role.negate YAML property to false, and then re-importing our container configuration file fixed the issue for us (with a subsequent cache rebuild).

We feel that the upgrade process from 1.x to 2.x wrongly translated the YAML property role_toggle: 'include listed' to conditions.user_role.negate: true instead of negate: false. Might be wrong about this though, just throwing out some ideas.

Got it, thanks!

@smustgrave Thank you for the feedback!

I have one question regarding the Change record being in a draft state: when should it be published (by me or anyone else)? Once this issue's MR is merged? I'm new to the Change record process, so I'm wondering what to do about that.

Thanks for your time!

• Created draft Change record: \Drupal\Core\CacheDecorator\CacheDecoratorInterface is deprecated →
• Adjusted deprecation statement to @deprecated in drupal:10.2.0 and is removed from drupal:11.0.0.

I had to be specific with the versions for the @deprecated statement, so I looked for the last/only usage removal of CacheDecoratorInterface in 2020, which seemed to be pointing to the 9.0.0 version of Drupal core.

I found this commit: Issue #3092090 by Berdir, Wim Leers, plach, amateescu: Remove legacy Path Alias subsystem, and then this issue: Remove legacy Path Alias subsystem → , which was Closed (fixed) for the 9.0.x-dev version of Drupal core.

Please feel free to have me update the @deprecated statement as needed if it seems incorrect.

I'll be working on this.