Problem/Motivation

In order to get the entity of a field you need to use $this->getParent()->getValue().
This has a couple of disadvantages:

  • getValue doesn't hint towards entities

Proposed resolution

Add a getEntity method on entityAdapter

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff-4-9.txt638 bytesmsankhala
#9 EntityAdapter-getEntity-2978067-9.patch1.35 KBmsankhala
#4 EntityAdapter-getEntity-2978067-4.patch1.35 KBmsankhala
#2 EntityAdapter-getEntity-2978067-2.patch700 bytesmsankhala

Comments

dawehner created an issue. See original summary.

msankhala’s picture

msankhala commented
Status: Active » Needs review
StatusFileSize
new EntityAdapter-getEntity-2978067-2.patch700 bytes

Here is the first version of the patch.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

We should probably add a test for this as well.

msankhala’s picture

msankhala commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new EntityAdapter-getEntity-2978067-4.patch1.35 KB

Here is updated patch with the test.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/tests/Drupal/Tests/Core/Entity/TypedData/EntityAdapterUnitTest.php
@@ -284,6 +284,13 @@ public function testGetValue() {
+    $this->assertEquals($this->entity, $this->entityAdapter->getEntity());

Is there a reason we don't use assertSame? This seems to make semantically more sense :)

dawehner’s picture

dawehner
Primary language German
commented

Thank you for working on this one.

msankhala’s picture

msankhala commented

I just kept testGetEntity() in consistent with testGetValue() which does the same thing. Do you recommend changing this to assertSame() only for testGetEntity()?

dawehner’s picture

dawehner
Primary language German
commented

I just kept testGetEntity() in consistent with testGetValue() which does the same thing. Do you recommend changing this to assertSame() only for testGetEntity()?

No worries. I just prefer better code over consistency of bad code :(

msankhala’s picture

msankhala commented
StatusFileSize
new EntityAdapter-getEntity-2978067-9.patch1.35 KB
new interdiff-4-9.txt638 bytes

Here is the updated patch.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This looks solid now.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

Thanks for the patch! Nice work @msankhala.

It's a very small addition, but I think we should probably also still add a add a change record for the addition so developers know they can use it.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

I added a change record

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • larowlan committed dd7fdb3 on 8.7.x 
    Issue #2978067 by msankhala: EntityAdapter should have a getEntity...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Fixed

Committed dd7fdb3 and pushed to 8.7.x. Thanks!

Published changed record

Status: Fixed » Closed (fixed)

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