Problem/Motivation

I came to this while trying to remove _comment_entity_uses_integer_id() as part of #3566536: [meta] eliminate core .module files.

Only in core we have many places where we check if the entity type ID is integer (or string). Some candidates (but there should be many more in contrib) :

  • _comment_entity_uses_integer_id()
  • DefaultHtmlRouteProvider - 4 occurrences
  • \Drupal\Core\DefaultContent\Exporter - 2 occurrences
  • \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem
  • \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema EDIT: Changes to this class are not applied because, in certain entity definition update scenarios, the entity type might not be available in code. See the EntityDefinitionUpdateMultipleTypesTest::testInstallFieldableEntityTypeWithoutInCodeDefinition() test. Also, the field storage definitions are already statically cached in SqlContentEntityStorageSchema, so it makes no sense to load them again via the field manager.
  • \Drupal\Core\Entity\ContentEntityStorageBase
  • \Drupal\content_moderation\Entity\Routing\EntityModerationRouteProvider
  • \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage
  • \Drupal\workspaces\WorkspaceTracker
  • \Drupal\Tests\jsonapi\Functional\ResourceTestBase

Proposed resolution

  • Add a ::hasIntegerId() method to the entity type class and return:
    • TRUE for integer,
    • FALSE for string,
    • NULL if the entity type lacks an ID key or the entity class doesn't implement FieldableEntityInterface
  • Use everywhere in core

Remaining tasks

  • Decide on which class to add. EDIT: It was added in EntityTypeInterface
  • Decide if we should statically cache the value inside the entity type

User interface changes

None.

Introduced terminology

N/A

API changes

New entity type method.

Data model changes

None.

Release notes snippet

Issue fork drupal-3566801

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: Unassigned » claudiu.cristea

Let's see where this goes

claudiu.cristea opened merge request !14293

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Parent issue: » #3566536: [meta] eliminate core .module files
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes

IS fix

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Issue summary: View changes
claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
Status: Active » Needs review

This ready for review

smustgrave’s picture

smustgrave commented

This seems like it could be a disruptive change based on all the tests that had to be updated to get the MR green.

Also seems like it should be broken up some

nicxvan’s picture

nicxvan commented

I'm not sure this needs to be broken up too much, most test changes were simplifications.

I think some of the protected removals maybe can be removed in a follow up.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

@smustgrave

This seems like it could be a disruptive change based on all the tests that had to be updated to get the MR green.

core/modules/layout_builder/tests/src/Unit/OverridesSectionStorageTest.php
Only removing unnecessary docs
core/tests/Drupal/KernelTests/Core/Entity/EntityTypedDataDefinitionTest.php and core/tests/Drupal/Tests/Core/Config/Entity/ConfigEntityTypeTest.php
Are new tests, testing the new feature
core/tests/Drupal/Tests/Core/Entity/Routing/DefaultHtmlRouteProviderTest.php and core/tests/Drupal/Tests/Core/Entity/Sql/SqlContentEntityStorageTest.php
Are only adding missing mock method return because of the new method.

@nicxvan

I think some of the protected removals maybe can be removed in a follow up.

Not sure which...

  • \Drupal\comment\CommentTypeForm::entityTypeSupportsComments() is just a wrapper used in one place. We can keep it but why?
  • \Drupal\content_moderation\Entity\Routing\EntityModerationRouteProvider::getEntityTypeIdKeyType() is only used in content_moderation. Keep in mind that I've kept (and deprecated) the one from DefaultHtmlRouteProvider because that's the only one with some chances to be reused somewhere. However, it's still protected
  • \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::hasIntegerId() is dead code, not used anywhere

Again, even it's not covered by BC policy, I kept DefaultHtmlRouteProvider::getEntityTypeIdKeyType(), as deprecated, because there's a chance that some contrib/custom code might use it

I hope I've answered both remarks.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

nicxvan’s picture

nicxvan commented
Status: Needs review » Needs work

I had another fresh look at this, I think the scope is fine, I had a brief chat with @smustgrave about it as well.

I have a couple of new comments and one question in light of my new review.

This simplifies the check quite a bit, I think it deserves it's own CR like it has.

I think the CR needs some work, an example or two from this CR, also I'm not sure what Some derved changes: means in the CR.

Pretty close, but needs work.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review
  • Addressed the remarks
  • Updated CR
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Some minor remarks on the MR.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

Thank you @nicxvan ands @berdir. I've addressed the remarks.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me, I wasn't aware that we have many integer id checks. I left one thread active on the MR to highlight for committers.

I was wondering if we should put this only on ContentEntityTypeInterface, but I think there is value in having it as an API on config entity types too, entity reference fields for example can be against config entities and they should then always return FALSE, so we don't need another condition there.

  • catch committed 182f4825 on 11.x 
    task: #3566801 Helper to check if the entity type's ID is integer

By:...

  • catch committed 747ad627 on main 
    task: #3566801 Helper to check if the entity type's ID is integer

By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Also wondered a bit about the lack of dependency injection here. I can't think of anywhere better to put this, and if we do, we could move it then, so let's go ahead. Committed/pushed to main and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !14293

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Published the CR

mondrake’s picture

mondrake
Location 🇮🇹
commented

This commit added two instances of #[Group('legacy')] attribute usage, that are not correct: https://www.drupal.org/node/3447698

The @group legacy annotation, that in old tests was used to indicate to the Symfony PHPUnit-bridge tests that were expected to check deprecations, must be replaced by the #[IgnoreDeprecations] attribute.

Will be fixed in #3497124: Deprecate expectDeprecation(), use PHPUnit's expectUserDeprecationMessage*() instead.

Status: Fixed » Closed (fixed)

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