Attached patch improves/fixes the entity backwards compatibility (BC) decoratur of the new entity field API to support working with converted entity properties also.

Right now, it only supports dealing with fields or arbitrary stuff that is on $entity, but not with entity properties that have been converted to entity fields (e.g. $comment->subject, $node->title).

Patch introduces a new if clause in the BC decorator and adds a small test to make sure it works correctly.

CommentFileSizeAuthor
#12 d8_entitybc-1890242-12.patch6.36 KBdas-peter
#5 d8_entitybc-1890242-5.patch5.54 KBBerdir
#3 d8_entitybc-1890242-3.interdiff.do_not_test.patch5.54 KBplach
#3 d8_entitybc-1890242-3.patch716 bytesplach
d8_entitybc.patch5.54 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Related: http://drupal.org/node/1818556#comment-6946062

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Issue tags: +Entity Field API
plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community
FileSize
d8_entitybc-1890242-3.patch716 bytes
d8_entitybc-1890242-3.interdiff.do_not_test.patch5.54 KB

Looks good to me, thanks.

+++ b/core/modules/system/lib/Drupal/system/Tests/Entity/EntityFieldTest.php
@@ -537,4 +537,28 @@ protected function assertComputedProperties($entity_type) {
+   * @todo: Remove once the entit BC decorator is removed.

Fixed typo.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, d8_entitybc-1890242-3.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
d8_entitybc-1890242-5.patch5.54 KB

@plach: I think you mixed up the interdiff and actual patch file names :)

Just re-uploading the existing files, no commit credit needed :)

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Yes, that :)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

Not sure what this helps with. Is it blocking a specific issue?

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented

#1818556: Convert nodes to the new Entity Field API is already including the code in this patch. It allows to access $node->title instead of $node->title[LANGUAGE_NONE][0]['value'] if $node is a decorated entity.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Gotcha. I figured it was factoring out code from elsewhere but couldn't place it, makes sense it's for nodes, back to RTBC then.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Issue tags: -Entity Field API

#5: d8_entitybc-1890242-5.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Entity Field API

The last submitted patch, d8_entitybc-1890242-5.patch, failed testing.

das-peter’s picture

das-peter CreditAttribution: das-peter commented
Status: Needs work » Needs review
FileSize
d8_entitybc-1890242-12.patch6.36 KB

Re-rolled. As far as I can see the conflicts based on changed doc blocks only.
I couldn't run the tests as locally the installation fails because of some ViewStorageController related exception - debugging that for now.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Given we decided to convert entities better in a single step anyway, I wonder whether we should leave it as is and save all the field_info_fields() call? Thus, the BC-decorator would only allow for field API like access to entity fields.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Needs review » Reviewed & tested by the community

Since in #1810370: Entity Translation API improvements we agreed to try the ELD solution and thus we need to fix type hints, I'd like to try again the BC decorator way in the node ng issue.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

Is there an issue for fixing type-hints? I think we should not commit this unless we have an actual need for it somewhere, as it will slow down BC-mode due to the field-info checks.

plach’s picture

plach
he/him
Primary language Italian
Location Venezia
CreditAttribution: plach commented
Status: Reviewed & tested by the community » Closed (won't fix)

Is there an issue for fixing type-hints?

I'm doing it in the node ng issue (patch around 100K), let's close this. We'll see whether this works over there.