- Issue created by @jigarius
- @jigarius opened merge request.
- Status changed to Needs review
about 1 year ago 4:26am 31 October 2023 - ๐จ๐ฆCanada jigarius Montrรฉal
Created an MR. Here's how ::referencedIds() works.
- Returns Target IDs, respecting deltas
- Invalid IDs are returned; there is no way to validate them, but that's normal.
- NULL items are not returned.
Please let me know if you want me to change anything.
- Status changed to Needs work
about 1 year ago 3:13pm 2 November 2023 - Status changed to Needs review
about 1 year ago 4:39pm 2 November 2023 - ๐จ๐ฆCanada jigarius Montrรฉal
Oops. Added a change record (unpublished).
- Issue was unassigned.
- Status changed to RTBC
about 1 year ago 6:22pm 2 November 2023 - ๐บ๐ธUnited States smustgrave
Thanks, and good job adding examples to the CR.
- Status changed to Needs review
about 1 year ago 11:27pm 15 November 2023 - ๐บ๐ธUnited States xjm
This should have a subsystem maintainer review since it's altering the public API of entity references. Thanks!
- Status changed to Needs work
about 1 year ago 2:53pm 30 November 2023 - ๐จ๐ญSwitzerland berdir Switzerland
Adding a new method to EntityReferenceFieldItemListInterface would break all modules implementing that, like entity_reference_revisions and I guess dynamic_entity_reference as well.
Also, this method doesn't make sense for those other implementations, ERR has two ids (id and revision id), DER has entity type and id as the entity type can vary. So it would need to be a new interface only implemented by the core entity reference field. I'm not sure that's worth it, the array_column() approach isn't that complex.
- ๐บ๐ธUnited States bradjones1 Digital Nomad Life
Came here to share similar sentiments as #8 and also, yeah, some implementations also would require an entity_type per reference to be meaningful. As proposed, if you used this on a DER field you would get an array of arrays (I think?) or at the very least IDs that were commingled among different entity types. I think the general idea here is good but we are trying to make entity references more robust, and this hard-codes in some assumptions around the core implementation.
- ๐จ๐ฆCanada jigarius Montrรฉal
Thanks for all the feedback. A different interface for a simple entity reference that just contains a target_id makes sense. However, I'll think aloud to get some more opinions.
- All field types have can have multiple columns to store the "value", which is returned by "::getValue()".
- So, if we want to check if a multi-value text field contains the value "foo", we'll have to do the same "array_column($entity->field->getValue())" exercise; which is perfectly fine, but looks kind of bulky (which is what motivated me to create this issue).
- What do you opine about having a generic way of getting a particular column or columns from a field's "value"?
- Say, something like
::getValue(array $columns = []);
? If $columns is empty, then "::getValue()" behaves like it does right now; if it contains exactly one value, it returns just that column; if it contains multiple values, it returns an array of arrays containing just the specified columns? - Maybe it can be a separate method instead of "::getValue()", say "::getColumn()".
Again, I'm just thinking aloud to see how you feel about providing a cleaner, easier-to-read way of getting an array of values in a field. A majority of simple fields have just one "value" column that contains the main value, so usually, devs would do something like,
$entity->field->getColumn("value")
to get something like,[0 => "foo", 1 => "bar"]
, etc.If this doesn't sound appealing and if the general opinion is that the dev should repeat "array_column()" for such cases, I will mark the issue as closed.
- ๐จ๐ญSwitzerland berdir Switzerland
getColumn() is an interesting thought, could be nice, but again, we need to think about BC. However, if we add it on FieldItemList then it will likely work for basically all fields. Not sure what to do with the interface, this is always awkward.
Related but different is I'm thinking about not using magic methods, ๐ [meta] Deprecate __get/__set() on ContentEntityBase Needs work is for the entity/field level, unsure about the properties, also in relation to helping people understand the difference between getValue() vs ->value and so on (because there is so much unnecessarily convoluted code out there that makes me sad as an entity maintainer). So maybe we should have a plan that covers that too, so that we can come up with an API and method names that make sense combined as well and then implement it in smaller issues.
Also somewhat related, there are some experiments with such methods on ๐ Load user entity in Cookie AuthenticationProvider instead of using manual queries Needs work although with the goal to bypass the field objects entirely, but still not sure if it's a good idea to do that.