Problem/Motivation

Drupal\Core\Entity\Sql\DefaultTableMapping says it requires ContentEntityTypeInterface in the constructor. However all the calls that are done by this class can be satisfied if the $entity_type implements Drupal\Core\Entity\EntityTypeInterface

This is relevant in case we want to support EntityListBuilders for non Content Entities, discovered as part of: #2401463: Make dblog entities

Proposed resolution

Replace ContentEntityTypeInterface with EntityTypeInterface in the DefaultTableMapping class.

Remaining tasks

Not sure if we want tests for this. In theory it should be covered by some of the existing EntityListBuilder tests since they call this class when sorting the table.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#2 3233143-2.patch2.71 KBdagmar

Comments

dagmar created an issue. See original summary.

dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Status: Active » Needs review
StatusFileSize
new 3233143-2.patch2.71 KB
dagmar’s picture

dagmar
Primary language Spanish
Location Argentina
commented
Issue summary: View changes

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented

Running against 9.5 now.

Is this a task more then a bug? If it's a bug will it require test cases?

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

9.5 passed and the change is small and easy to review. Looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Category: Bug report » Task
Related issues: +#2232465: Deprecate table names from entity definitions

I'm sympathetic to what the issue is trying to do but I'm not sure it is practical for the following reasons:

  • The only other implementation in core is \Drupal\Core\Config\Entity\ConfigEntityType and this class definitely won't work with that.
  • Some people want to deprecate these methods from the EntityTypeInterface as these are storage implementation details - see #2232465: Deprecate table names from entity definitions

I think limiting this to ContentEntityTypeInterface is pragmatic. Going to ping entity system maintainers before closing as works as designed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Closed (works as designed)

Discussed with @larowlan who pointed out that

Also the original justification 'to allow using entity list builders with non content entities' sounds no longer relevant - you can definitely have list builders for config entities, even as far back as 8.2

Given this and #8 I'm closing this issue.