Updated: Comment #N

Problem/Motivation

Splitt-off of #2002138: Use an adapter for supporting typed data on ContentEntities.

We want to rename that method on entities to toArray() to have common method to get an array representation of an entity. Config entities already had their getExportProperties() renamed.

Proposed resolution

The issue above renames the method on content entities. To make that issue smaller, because it's going to grow a lot before it's done, let's just rename getPropertyValues() on ComplexDataInterface as well and remove setPropertyValues() there as well. It's never used outside of tests and if we want to, we can just let setValue() take it's place if we want to?

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#27 entity-toArray-2223361-26-interdiff.txt1.54 KBBerdir
#27 entity-toArray-2223361-26.patch14.53 KBBerdir
#24 entity-toArray-2223361-24-interdiff.txt1.41 KBBerdir
#24 entity-toArray-2223361-24.patch13.77 KBBerdir
#21 interdiff.txt1.3 KBtim.plunkett
#21 entity-toArray-2223361-21.patch13.35 KBtim.plunkett
#18 d8_to_array.patch13.22 KBfago
#18 d8_to_array.interdiff.txt1.66 KBfago
#17 d8_to_array.interdiff.txt1.66 KBfago
#17 d8_to_array.patch13.22 KBfago
#9 rename-get-property-values-2223361-9-interdiff.txt1.41 KBBerdir
#9 rename-get-property-values-2223361-9.patch12.82 KBBerdir
#6 rename-get-property-values-2223361-6-interdiff.txt934 bytesBerdir
#6 rename-get-property-values-2223361-6.patch12.62 KBBerdir
#4 rename-get-property-values-2223361-4-interdiff.txt1.08 KBBerdir
#4 rename-get-property-values-2223361-4.patch12.77 KBBerdir
#2 rename-get-property-values-2223361-2.patch12.28 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Issue tags: +KV Entity Storage

Awesome

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
rename-get-property-values-2223361-2.patch12.28 KB

Let's see how this goes.

I also want to promote usage of $entity->get('some_field')->toArray() over getValue() now that we have this but that can be a follow-up issue and wouldn't be an API change but it would also help to keep the size of #2002138: Use an adapter for supporting typed data on ContentEntities down. I'll open another issue.

Status: Needs review » Needs work

The last submitted patch, 2: rename-get-property-values-2223361-2.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
rename-get-property-values-2223361-4.patch12.77 KB
rename-get-property-values-2223361-4-interdiff.txt1.08 KB

Uhm, not sure how that happened.

Status: Needs review » Needs work

The last submitted patch, 4: rename-get-property-values-2223361-4.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
rename-get-property-values-2223361-6.patch12.62 KB
rename-get-property-values-2223361-6-interdiff.txt934 bytes

Ok, doing the lazy thing for now, there are various test entity classes that extend directly from Entity and now need that method, including mocks and so on.

Status: Needs review » Needs work

The last submitted patch, 6: rename-get-property-values-2223361-6.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

See #2223423: Stop extending \Drupal\Core\Entity\Entity in unit tests for the non-lazy solution.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
rename-get-property-values-2223361-9.patch12.82 KB
rename-get-property-values-2223361-9-interdiff.txt1.41 KB

Fixing those test fails, missed that the removed call changed the value.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs review » Reviewed & tested by the community

Works for me. @Berdir++

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: Unassigned » fago
Issue tags: +API change

Would like to have the OK from @fago on this as well, let's give him some time to comment on this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: rename-get-property-values-2223361-9.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

9: rename-get-property-values-2223361-9.patch queued for re-testing.

The last submitted patch, 9: rename-get-property-values-2223361-9.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

9: rename-get-property-values-2223361-9.patch queued for re-testing.

catch’s picture

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

Looks like retests don't change status. Back to RTBC.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
Assigned: fago » Unassigned
Status: Reviewed & tested by the community » Needs review
FileSize
d8_to_array.patch13.22 KB
d8_to_array.interdiff.txt1.66 KB

Yeah, this works for me. As said in some other issue, I'm +1 on toArray(). I'm not so sure about removing setPropertyValues(), but given it's close to no where used it seems pointless to keep it.

However, as toArray() is now defined in ComplexDataInterface and EntityInterface which the content entity both implements, I think we should make sure the docs align well. Thus, I've worked a bit on the docs, please review the attached changes and updated patch.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented
FileSize
d8_to_array.interdiff.txt1.66 KB
d8_to_array.patch13.22 KB

Fixed a comma which should be a point.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Hm, computed is a concept that doesn't really exist on EntityInterface.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I know it doesn't have computed stuff *formally*, but still I think it makes sense to have it there as many entities have *informally* computed stuff - i.e. an entity could have $entity->data which is somehow computed and customly set. This shouldn't be in there though.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
FileSize
entity-toArray-2223361-21.patch13.35 KB
interdiff.txt1.3 KB

That comment seems specific to content entities, why not just add the docs on ContentEntityBase? Since we're not allowed to append docs to {@inheritdoc}, just replicating them.

Also, in case someone disagrees and moves it back, note that I corrected "not-computed" to "non-computed".

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yeah, that makes more sense to me for now as well.

@fago is right that config entities have computed things but those aren't values for me (like the plugin (bag) for those that have that.). Maybe we can describe it in a different way somehow, I think the idea is that you can call toArray() of an entity and pass those values to a new entity object and you then basically have the same entity.

Also, the overlap will be less problematic with the adapter approach as ContenEntityBase will no longer implement both interfaces.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

ok, works for me but shouldn't the clarification be on ContentEntityInterface then? It's not only a matter for the base class implementations.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
entity-toArray-2223361-24.patch13.77 KB
entity-toArray-2223361-24-interdiff.txt1.41 KB

Ok, like this.... ? please? :)

Note that I also changed property to field as it's now on ContentEntityInterface. I explicitly avoided using property or field in the original patch :)

Status: Needs review » Needs work

The last submitted patch, 24: entity-toArray-2223361-24.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented 
+++ b/core/lib/Drupal/Core/Entity/ContentEntityInterface.php
@@ -124,4 +124,16 @@ public function getFieldDefinition($name);
+   * Gets an array of plain property or field values, including only

Still property and fields ;) Except for the fail, this looks good now.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
entity-toArray-2223361-26.patch14.53 KB
entity-toArray-2223361-26-interdiff.txt1.54 KB

Fixed the new test and comment.

fago’s picture

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

Great!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2815203 and pushed to 8.x. Thanks!

  • Commit 2815203 on 8.x by alexpott: 
    Issue #2223361 by Berdir, fago, tim.plunkett: Rename...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

We probably need to check existing change notices too

Status: Fixed » Closed (fixed)

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