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

Command icon 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

catch created an issue. See original summary.

godotislate’s picture

The 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->entity might be pretty disruptive. I think there's a lot of similar use in Twig templates as well: entity.field_reference.entity.

godotislate’s picture

Quick 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.

catch’s picture

The magic getter use seems like something that could be replaced with property hooks, once on D12/PHP 8.4.

I 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.

EntityReferenceFieldItemist could 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 ___get bug.

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

godotislate’s picture

Two separate thoughts:

First, separate from the specific entity property 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 specific get*() and set*() 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 using get() 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:

  • Add an optional second boolean argument to EntityStorageInterface::loadMultiple() and EntityStorageInterface::load() to look like public function loadMultiple(?array $ids = NULL, bool $use_fibers = TRUE) and public function load($id, bool $use_fibers = TRUE), where if the second argument is passed as FALSE, fibers are not used
  • In Drupal\Core\Entity\Plugin\DataType\EntityReference::getTarget(), pass FALSE to the entity storage load() call
  • In Drupal\Core\Field\EntityReferenceFieldItemList::referencedEntities(), pass FALSE to the entity storage loadMultiple() call
berdir’s picture

I 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.

catch’s picture

Opened #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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

catch’s picture

ghost of drupal past’s picture

generate bundle classes into phpstorage on bundle create / update / delete events , add methods or property hooks or w/e for each field, gg

andypost’s picture