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.