See:

CommentFileSizeAuthor
#11 2095587-remove-identifiableinterface-11.patch3.87 KBdixon_
#12 interdiff.txt571 bytesdixon_
#3 2095587-remove-identifiableinterface-1.patch3.86 KBdixon_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Issue tags: +Entity Field API

Tagging

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Title: #HardProblems conclusion: Remove IdentifiableInterface » #HardProblems DX conclusion: Remove IdentifiableInterface
dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
FileSize
2095587-remove-identifiableinterface-1.patch3.86 KB

Here we go.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2095587-remove-identifiableinterface-1.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review

Can anyone please mention the reason here in the issue for the removal of IdentifiableInterface?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Needs work

:/ x-post

Berdir’s picture

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

#3: 2095587-remove-identifiableinterface-1.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

The problem is that the separate interface hides the very important id() method for entities in a separate interface, this was pointed out by @webchick in a DX review.

The interface was added for param converting, but it turns out that it's not actually necessary, so the only place that uses it is the data reference stuff, where we already need a subclass anyway and one of the two implementations already has a overridden method anyway.

+++ b/core/lib/Drupal/Core/Entity/EntityInterface.php
@@ -42,6 +41,14 @@
+   *   The object identifier, or NULL if the object does not yet have an identifier.

This should now say The entity identifier I think.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work
dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
FileSize
2095587-remove-identifiableinterface-11.patch3.87 KB

This should now say The entity identifier I think.

Fixed this.

I've also been debugging the failing test. It passes just fine through the browser UI for me, but fails when running through Drush. It seems to be related to session and login handling somehow.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
CreditAttribution: dixon_ commented
Status: Needs work » Needs review
FileSize
interdiff.txt571 bytes
Berdir’s picture

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

Ok, this looks good to me, thanks!

fubhy’s picture

fubhy CreditAttribution: fubhy commented

Yeah. +1 from me.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: #HardProblems DX conclusion: Remove IdentifiableInterface » Change notice: #HardProblems DX conclusion: Remove IdentifiableInterface
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Approved API change

Yayyy!! :D Thanks, folks!

Committed and pushed to 8.x.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Title: Change notice: #HardProblems DX conclusion: Remove IdentifiableInterface » #HardProblems DX conclusion: Remove IdentifiableInterface
Status: Active » Fixed
Issue tags: -Needs change record

There was no change notice for this being added, so we don't need to update any :)

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