Problem/Motivation
Opening this as a task, but it is a possible approach for the problem described in https://www.drupal.org/project/drupal/issues/3518668#comment-16356696 and https://github.com/php/php-src/issues/14983
We have the following supporting things like $entity->fieldwhatever->entity;
FieldItemList
/**
* {@inheritdoc}
*/
public function __get($property_name) {
// For empty fields, $entity->field->property is NULL.
if ($item = $this->first()) {
return $item->__get($property_name);
}
}
Which calls this:
FieldItemBase
/**
* {@inheritdoc}
*/
public function __get($name) {
// There is either a property object or a plain value - possibly for a
// not-defined property. If we have a plain value, directly return it.
if (isset($this->properties[$name])) {
return $this->properties[$name]->getValue();
}
elseif (isset($this->values[$name])) {
return $this->values[$name];
}
}
I think we could provide non-magic versions of these, e.g.:
FieldItemList
public function getFirstItem($name);
FieldItemBase
public function getValue($name);
Then you could do:
$entity->field_whatever->getFirstItem('entity');
Steps to reproduce
Proposed resolution
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3565858
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
godotislateThe magic getter use seems like something that could be replaced with property hooks, once on D12/PHP 8.4. It might be worth exploring whether property hooks play well with the use of Fibers.
In the meantime, needing to replace or avoid the use of
$entity->field_reference->entitymight be pretty disruptive. I think there's a lot of similar use in Twig templates as well:entity.field_reference.entity.Comment #3
godotislateQuick chat with @catch on slack and he brought up that property hooks on the entity object won't work. Since the field names are not predefined, we can't specify them as properties in code. Could possibly use property hooks on the field item list objects, but that would be only be for list object types that correlate to specific field item types and the properties would be known. Not enough to solve the problem here either way.
Comment #4
catchI initially was thinking along those lines, but I didn't see how it can work with this specific API, then I pinged @godotislate in slack and we'd both missed something (different somethings) but came up with something that might work.
EntityReferenceFieldItemistcould define $entity with property hooks. Then $entity->field_reference_field->entity would be to the public property instead of__get()and the property hooks would kick in. No actual API change.Since the main problem we have with
__get()is the Fiber issue, that would be enough. We could still move some more thing to formal methods later on if we want to.Property hooks appear to be unaffected by the fiber reference counting bug: https://3v4l.org/tpXCf#v8.5.1 (unless that example is wrong which it might be). I started from https://3v4l.org/ju5mp#vnull which shows the
___getbug.The big drawback to this is that property hooks are only available from PHP 8.4, and they're a syntax error on lower versions. That might make this issue 12.x only, or we could potentially do something with class_alias().
If all of the above works, then we might want to just disable Fiber::suspend() within entity loading on PHP versions < 8.4
Comment #5
godotislateTwo separate thoughts:
First, separate from the specific
entityproperty concerns, I think there is value in adding API methods to access a specified property value from the first field item. I think it would be a good thing to move away from magic getters and setters overall, and while I know there are projects that use custom bundle classes to add their own specificget*()andset*()methods (that can also be typed), those have to be done project to project, so having generic methods still has use. In fact, I wonder if there's also value in a method that gets the values for all field items in a list for a certain property. I've done something like$values = array_column($entity->get('field_name')->getValue(), '{PROPERTY NAME}');on several projects, so I think an API method that just does basically that would be useful. Then for Twig, perhaps there can be an extension that handles dot properties by using the new API, or usingget()directly, instead of relying on the magic getter. See: https://stackoverflow.com/a/77559746. Though if so, we probably should make some progress on #3490787: Make it possible not to throw an exception when accessing a non-existing field with FieldableEntityInterface::get(), so that we can replace the magic getter use without throwing exceptions.Second, about the entity property specifically, and maybe this should be a separate issue, but before we can use property hooks, I wonder if we can do something like:
public function loadMultiple(?array $ids = NULL, bool $use_fibers = TRUE)andpublic function load($id, bool $use_fibers = TRUE), where if the second argument is passed as FALSE, fibers are not usedDrupal\Core\Entity\Plugin\DataType\EntityReference::getTarget(), passFALSEto the entity storageload()callDrupal\Core\Field\EntityReferenceFieldItemList::referencedEntities(), passFALSEto the entity storageloadMultiple()callComment #6
berdirI have lots of thoughts on this, but I'm going to need some time to put that in comments/code.
One is that I had planned to extract my idea in #2345611: Load user entity in Cookie AuthenticationProvider instead of using manual queries into a separate issue, although likely with less magic/assumptions than what I did there, I think that issue was trying to do too much. Essentially, the idea there is to have a method directly on ContentEntityInterface, something like getFieldPropertyValue('field_name', 'entity')
Another is opening an issue for twig to use proper methods and not it's built in assumptions, see comment #10 in #3490787: Make it possible not to throw an exception when accessing a non-existing field with FieldableEntityInterface::get(). As twig is translated to PHP code, we can still do node.field_name.entity there but do whatever we want with that, but i need to investigate how to do that.
One thing to consider is that magic bypasses creating of property objects for non-computed properties, so we also need an API that does what FieldItemBase:__get() does, otherwise we might end up making things slower and use more memory.
And yes, impact of this and also the get()/__get() issue is massive if we want to fully deprecate magic methods completely, but first we need alternatives.
Comment #7
catchOpened #3566626: [12.x] Use property hooks for EntityReferenceFieldItemList->entity which doesn't get rid of __get() but will get rid of very common path where it's called.
Comment #9
catchComment #10
ghost of drupal pastgenerate bundle classes into phpstorage on bundle create / update / delete events , add methods or property hooks or w/e for each field, gg
Comment #11
andypostlooks it's better approach instead of magic (g|s)etters #3281720: [meta] Deprecate __get/__set() on ContentEntityBase
Looks everything is blocked on memory leaks and #1977266: Fix ContentEntityBase::__get() to not return by reference
OTOH there's
- #2896474: Provide an API to temporarily associate data with an entity
- PHP 8.5 property getters also a bit can help