FieldItemDataDefinition::getLabel() should show the label of the field type

@rlmumford Use $this->getTypedDataManager() instead of $this->typedDataManager. Also can you write test case for it.

Updated patch to add $this->getTypedDataManager() instead of $this->typedDataManager.

Please review.

No need to add TypedDataTrait trait, Parent class already using this triat. Updated the patch.

getLabel should be typehinted as a new function.

Also some test coverage will be needed

Thanks!

I've added a test for the getLabel method. I would appreciate your assistance in reviewing it.

Thank you.

#13 I should of also tagged for a change record. Can be super simple.

Recommend turning patch to MR as patches are going away.

I have created a merge request. Could you please take a look and let me know if everything is in order?

Will add to my list to take a look!

Removing tests tag as they have been added and can be seen in the test-only feature

There was 1 failure:
1) Drupal\KernelTests\Core\Field\FieldItemTest::testFieldItemDataDefinitionReturnLabel
Failed asserting that null matches expected 'Text (plain)'.
/builds/issue/drupal-3128190/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3128190/core/tests/Drupal/KernelTests/Core/Field/FieldItemTest.php:109
/builds/issue/drupal-3128190/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
FAILURES!
Tests: 2, Assertions: 31, Failures: 1.

Believe the last thing to do is draft a change record.

Updated issue summary too with the correct template

Please help to review change record https://www.drupal.org/node/3404250

Thanks! Think this one is good.

I'm triaging RTBC issues. I read the IS and the comments, the MR and the Change record.

Thanks for working on this feature!

Overall, the change is working but there is some more things to do to finish this.

• There is a comment in the MR that this has an out of scope change.
• The test should also test when there is a label.
• The change record should explain what the return value will be when there is a label and when there is not. Currently, it is a misleading.

Back to needs work for a few more things.

1. There is a comment in the MR that this has an out of scope change

DONE

2. The test should also test when there is a label.
   

   It is there https://git.drupalcode.org/project/drupal/-/merge_requests/5550/diffs#dc...

3. The change record should explain what the return value will be when there is a label and when there is not. Currently, it is a misleading.
   

   Still pending

I resolved all items in comment #23. Please help to review.

Feedback appears to be addressed.

I left two suggestions in the MR to simply method name and a comment.

Updated the method name and comment as per #29, Please review.

Appears suggestions were adopted

Committed and pushed 94b03bcdec to 11.x and 973d283bea to 10.3.x. Thanks!

This is almost a bug...

We cannot add a return type here. This would break contrib code that extends this class and overrides the method already.

diff --git a/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php b/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php
index 2cd2137b0e..2604eec502 100644
--- a/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php
+++ b/core/lib/Drupal/Core/Field/TypedData/FieldItemDataDefinition.php
@@ -98,7 +98,7 @@ public function setFieldDefinition($field_definition) {
    *
    * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
    */
-  public function getLabel(): string {
+  public function getLabel() {
     return parent::getLabel() ?: $this->getTypedDataManager()->getDefinition($this->getDataType())['label'];
   }

Fixed on commit.

After committing I wonder if we should push this fix to the parent class and make the following change...

diff --git a/core/lib/Drupal/Core/TypedData/DataDefinition.php b/core/lib/Drupal/Core/TypedData/DataDefinition.php
index a82f819b4c..c4acc004ec 100644
--- a/core/lib/Drupal/Core/TypedData/DataDefinition.php
+++ b/core/lib/Drupal/Core/TypedData/DataDefinition.php
@@ -72,7 +72,7 @@ public function setDataType($type) {
    * {@inheritdoc}
    */
   public function getLabel() {
-    return $this->definition['label'] ?? NULL;
+    return $this->definition['label'] ?? $this->getTypedDataManager()->getDefinition($this->getDataType())['label'] ?? NULL;
   }
 
   /**

And remove the override in FieldItemDataDefinition

Can someone file a follow-up to do this?

As @alexpott suggestion, I moved code to parent class but I dont know how to fix the test failed.