Remove PrimitiveBase::setValue() and ::getValue() — identical to their parents

I'll be taking this as part of a Symetris contribution workshop.

I'll be working on this.

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.

it's only failing in the javascript tests, looks like a random failure to me. I think this is actually good to go.

If we are going to do this we should at least remove getValue() as well, as that's also the same as the parent implementation. If this is the only change this doesn't need a change record, there is no functional change here, just removing useless code.

But having said that, having the getter and setter in the abstract base class refer to a property that doesn't exist is a bit of a strange design, and PHPStan is already complaining about this:

message: "#^Access to an undefined property Drupal\\\\Core\\\\TypedData\\\\TypedData\\:\\:\\$value\\.$#"
count: 2
path: lib/Drupal/Core/TypedData/TypedData.php

Would it be better to remove getValue/setValue from the abstract class and implement them only in PrimitiveBase? We would need to implement backward compatibility here though.

Also this is a duplicate of 📌 Useless method overrides in \Drupal\Core\TypedData\PrimitiveBase and missing property definition in TypedData Needs work

Would it be better to remove getValue/setValue from the abstract class and implement them only in PrimitiveBase? We would need to implement backward compatibility here though.

That'd indeed be a BC break. That's why I proposed this.

The simpler and lower-risk change is to remove both ::setValue() and ::getValue(), and define the protected $value property in abstract class TypedData.

P.S.: Sorry for not finding 📌 Useless method overrides in \Drupal\Core\TypedData\PrimitiveBase and missing property definition in TypedData Needs work 😬.

Unwanted code has been removed based on the last comment.

Fixed Pipeline failure issues and added #10 changes in this Mr against 11.x .
Please review.

Close, but not quite 😅 Too many changes, some of which constitute a BC break.

Also: we need to update the PHPStan baseline, that's what the MR pipeline is currently failing on. Some errors no longer need to be ignored! 🎉

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).

Can confirm that the setValue and getValue are the exact same as TypedData

So cleanup looks fine to me.

I think we should deprecate \Drupal\Core\TypedData\TypedData::getValue() and \Drupal\Core\TypedData\TypedData::setValue() and tell people to either extend PrimitiveBase or implement it themselves.

Addded the change as per #17 but i think we may need to fix php standards for that as well.

This still needs the deprecation #17 asked for.

I will work on this.

I added deprecations and tests and reverted the commit that removed the functions to deprecation.
However, PHPStan warns of making a deprecated call by adding the deprecations.
The following classes directly inherit TypedData and have no overridden function implementations.

- core/lib/Drupal/Core/TypedData/Plugin/DataType/Any.php
- core/lib/Drupal/Core/Config/Schema/Element.php
- core/modules/layout_builder/src/Plugin/DataType/SectionData.php
- core/modules/system/tests/modules/entity_test/src/TypedData/ComputedString.php
- core/modules/text/src/TextProcessed.php
- core/tests/Drupal/Tests/Core/Plugin/Fixtures/Plugin/DataType/TestDataType.php

If change it to inherit from PrimitiveBase, I should implement PrimitiveInterface::getCastedValue().

How should I fix it?

Need to make sure no code in core is calling the deprecated functions. So when we remove it'll be a simple delete.

I removed the methods from TypedData and reimplemented them in TypedData inherited classes that did not implement the method.
And, I removed ignoreErrors item from .phpstan-baseline.php since $value is no longer used in TypedData.

Believe feedback has been addressed.

This is removing 29 lines of code to add 88 lines which are mostly duplicated, I think it needs more discussion. Why not a trait?

We could use a Trait, but there are concerns.
PrimitiveBase::setValue uses properties ($name and $parent) defined in the parent TypedData class.
These properties should also be implemented in a new Trait, as the Trait defines the setValue function.
Since it is assumed that the Trait will be used by classes that inherit TypedData, classes that use the Trait will redefine the properties of TypedData.
Currently, there is no problem as the final values are the same and no errors have occurred.
However, if changes $name or $parent in TypedData in the future, the changes will no longer directly follow the properties in the Trait by redefinition, and the values ​​of properties in classes that use the Trait may not be set appropriately.
Is this an unnecessary concern?