Problem/Motivation
Currently, \Drupal\Core\Entity\EntityBase::getTypedData()
does not consider the derivatives generated by \Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver
. Instead, an EntityAdapter
is returned for all entities (e.g., instead of a ConfigEntityAdapter
for configuration entities).
Proposed resolution
EntityBase::getTypedData()
should be updated to consider all entity data type derivatives - not only for entity types, but also entity type bundles (e.g., entity:node
and entity:node:article
).
Doing so will cause the correct typed data object to be returned by EntityBase::getTypedData()
.
Remaining tasks
RTBC & commit
Issue fork drupal-2870874
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:
Comments
Comment #9
clayfreemanSince a
ConfigEntityAdapter
has been added, I'm going to be investigating a path to making sure it's returned by::getTypedData()
.Comment #10
tim.plunkettComment #12
clayfreemanThe MR from #11 modifies
\Drupal\Core\Entity\EntityBase::getTypedData()
to use derivative data types for entities where possible. As a result, the correct entity adapter should be returned according to the logic in\Drupal\Core\Entity\Plugin\DataType\Deriver\EntityDeriver::getDerivativeDefinitions()
:I added a test case for this change to
\Drupal\Tests\Core\Entity\ContentEntityBaseUnitTest
sinceEntityBase
didn't seem to have its own unit test, and it would be much more involved to not leverage existing testing infrastructure in this unit test. Let me know if a unit test forEntityBase
should be created.Comment #13
clayfreemanComment #14
clayfreemanPer @alexpott's review via Slack:
I've taken the following actions:
::getTypedData()
Comment #16
tim.plunkett+1 to this improvement, nice work @clayfreeman.
Comment #17
clayfreemanThis looks great - thanks Tim!
Comment #18
clayfreemanActually, I noticed the new test case was failing due to the change in logic introduced in #16, so I adjusted the test to match the changes therein.
Since the scope of the change is fairly small, I don't believe it warrants demotion from RTBC.
Comment #19
Wim LeersReally nice work here, but it currently does not yet work for config entity types. I know config entity types do not yet have validation constraints in Drupal core, but this is possibly the last missing piece before those can "just work". See my comments on the MR. This would help #2164373: [META] Untie config validation from form validation — enables validatable Recipes, decoupled admin UIs … … and getting CKEditor 5 in core (which will use config entity validation).
Comment #20
Wim LeersUgh, never mind, the suggestion I left on the MR won't work, because apparently the config schema structure (
filter.format.*
) does not match the pre-existing data type definition for config entity types (which indeed isentity:filter_format
, which the current merge request is already generating).Either way, this issue is a step forward for making config entity validation possible, because
$filter_format->getTypedData()->validate()
now returnsConfigEntityAdapter
instead ofEntityAdapter
:)RTBC++
Comment #21
clayfreemanComment #22
larowlanLeft some comments on the MR.
Technically this was self-RTBC'd by @clayfreeman which is frowned upon.
But it was also RTBC+1 from Wim, which makes it OK I think.
Because my comments on the MR don't change the fundamentals, its ok to re-rtbc this after those changes.
Comment #23
clayfreemanI opted to rename the
::getClass()
method to::getTypedDataClass()
since the original name wasn't super descriptive of what the method was doing.Re-RTBC per #22. (Original RTBC was for Tim's changes prior to #16.)
Comment #26
larowlanCommitted 61bff4e and pushed to 9.3.x. Thanks!
I think there's a slight risk of disruption here so erring against backporting to 9.2.x
I'll ask for a second opinion.
Comment #27
clayfreemanPushed the patchset rebased on 9.2.x - let me know if adjustments need to be made.
Comment #28
larowlanIn discussing the potential impact of this w.r.t its eligibility for backport, I'm of the opinion that this is more of a part feature request, part bug, as there is nothing in core that implements
::createFromEntity
for anything other than the base EntityAdapter.So I'm going to split the difference and mark this as a task. I'll add a change record detailing the new functionality available to those who alter the typed-data plugin definitions.
Comment #29
larowlanChange notice at https://www.drupal.org/node/3232431