Add return types to EntityDefinitionTestTrait

Based on https://www.drupal.org/project/drupal/issues/3529510#comment-16141888 📌 Determine if there are other TestTraits we can add return types to. Active

However, I'm pretty sure if you override a method from a trait then the trait method effectively does not exist, so you can do whatever you want with the signature and PHP won't care. See https://3v4l.org/kW8DR

All but 2 of these functions are adding void so those are all safe.

The other 2 already had @return docs so they aren't technically required here to cleanup the baseline, should we exclude them to make this easier to get in?

Good point, thanks for pointing that out. I've reverted those two.

Seems pretty straight forward.

I'd expect an RTBC for this to describe how precisely the many many methods included in this MR were reviewed to ensure that it was actually intentional that they had no return value. :)

28 methods none had @return

Trait itself getUpdatedEntityTypeDefinition and getUpdatedFieldStorageDefinitions are the only methods that have a return and those were untouched.

@smustgrave, I was implying that you should actually read the methods (as I stated explicitly on the new sub-meta). :) In general, for any issues that add typehinting, you should actually read the code (at least briefly) rather than relying solely on the docs or static analysis, unless it's impossible in context for the typehint to be anything other than what was added (as it was for actual test runner methods in test classes). As per #3529510-11: [meta] Add return types to test traits because there is no BC break! → :

We simply need to open the methods and verify that they don't return anything under any circumstance (and moreover weren't supposed to, which static analysis can't tell us).

Since we need to review the whole methods to verify that they are, indeed, intended to be void, the line length of the methods is the factor we want to look at

I will be reading (or at least scanning) every method in the trait to review this for commit.

I read over the full trait. The majority of methods on the trait simply set something-or-other in state; the others perform various operations like entity updates or changes to the entity storage. None had any conceivable code path that could ever have contained a return value.

Saving credits. I'm granting credit for #8 as a best effort, although I would prefer to see a little more detail (and actual review of the code, not just the docs) in future issues for this meta in order to receive credit. Thanks!

Committed to 11.x and 11.2.x. Thanks! Another big 1.8K slice out of the baseline!

In the previous issue, the baseline didn't actually need cleanup for this. Since we're so close to the release and since 10.5 is a maintenance minor, I don't think it's worth backporting all these issues. So marking fixed against 11.2.x.

Aside for future issues: Because of #3, I actually think these issues can even potentially go into 11.2 patch releases, although I'll want a +1 from another RM before I actually backport something post-11.2.0.

xjm → closed merge request !12363

This might be more disruptive than I previously thought unfortunately; see #3529510-25: [meta] Add return types to test traits because there is no BC break! → through #28. We're going to try another child issue there and then circle back to this. One key thing to check is which core test base classes use this trait and might break contrib. (Though the fact that no one came here over the weekend being frustrated with us is a good sign.)

The trait is used in 11 classes. One of them (EntityTestResourceTestBase) is an abstract base class.

$ grep -r "use EntityDefinitionTestTrait" core
core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php:  use EntityDefinitionTestTrait;
core/modules/content_moderation/tests/src/Kernel/ContentModerationStateTest.php:  use EntityDefinitionTestTrait;
core/modules/views/tests/src/Kernel/EventSubscriber/ViewsEntitySchemaSubscriberIntegrationTest.php:  use EntityDefinitionTestTrait;
core/modules/field/tests/src/Kernel/Entity/Update/SqlContentEntityStorageSchemaColumnTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateMultipleTypesTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/ContentEntityNonRevisionableFieldTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/EntitySchemaTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateProviderTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/FieldableEntityDefinitionUpdateTest.php:  use EntityDefinitionTestTrait;
core/tests/Drupal/KernelTests/Core/Entity/DefaultTableMappingIntegrationTest.php:  use EntityDefinitionTestTrait;

With the help of AI (sorry) I ran this script to extract all the function names in the trait and check if they are implemented anywhere else:

for fn in $(grep -E 'function\s+[a-zA-Z0-9_]+\s*\(' core/modules/system/tests/src/Functional/Entity/Traits/EntityDefinitionTestTrait.php | sed -E 's/.*function\s+([a-zA-Z0-9_]+).*/\1/'); do
  echo "Searching for: $fn"
  grep -r "function $fn" core | grep -v 'EntityDefinitionTestTrait.php'
done

The output is below:

Searching for: applyEntityUpdates
Searching for: doEntityUpdate
Searching for: doFieldUpdate
Searching for: enableNewEntityType
Searching for: resetEntityType
Searching for: updateEntityTypeToRevisionable
Searching for: updateEntityTypeToNotRevisionable
Searching for: updateEntityTypeToTranslatable
Searching for: updateEntityTypeToNotTranslatable
Searching for: updateEntityTypeToRevisionableAndTranslatable
Searching for: addBaseField
Searching for: addLongNameBaseField
Searching for: addRevisionableBaseField
Searching for: modifyBaseField
Searching for: makeBaseFieldEntityKey
Searching for: removeBaseField
Searching for: addBaseFieldIndex
Searching for: removeBaseFieldIndex
Searching for: addBundleField
core/modules/migrate_drupal/src/FieldDiscoveryInterface.php:  public function addBundleFieldProcesses(MigrationInterface $migration, $entity_type_id, $bundle);
core/modules/migrate_drupal/src/FieldDiscovery.php:  public function addBundleFieldProcesses(MigrationInterface $migration, $entity_type_id, $bundle) {
Searching for: modifyBundleField
Searching for: removeBundleField
Searching for: addEntityIndex
Searching for: removeEntityIndex
Searching for: renameBaseTable
Searching for: renameDataTable
Searching for: renameRevisionBaseTable
Searching for: renameRevisionDataTable
Searching for: deleteEntityType
Searching for: getUpdatedEntityTypeDefinition
Searching for: getUpdatedFieldStorageDefinitions

The migrate_drupal classes are false alarms because they are not in fact using the trait. So it seems we're good, at least for core.

@xjm not moving to fixed but #16 seems to answer your question I believe.

I think even if this was a bit disruptive for some contrib modules, it's covered under this in the bc policy:

https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... →

Test traits and abstract base classes should generally use deprecation where possible rather than breaking backwards compatibility, but they are still considered an internal API and may change if necessary.

Not having to add a baseline entry every time you use one of these traits is a very good improvement for both core and contrib which for me outweighs the possibility of someone potentially having overridden a trait method that's used from a base class they inherit from. The fix would be to add the equivalent type hint in the test, which would be fine with previous core releases too.

Thanks everyone; that's compelling enough for me!

Automatically closed - issue fixed for 2 weeks with no activity.