Workaround PHP bug with fibers and __get()

PHPStan is complaining, in which case, maybe we don't add the second argument explicitly and use func_get_args(). Since we're not sure it's going to be API anyway, we can maybe just leave it that way.

Failing test seems unrelated and probably just needs a re-run.

Code changes look good, but we probably need a test.

Even if we don't make this API, do we need to document this (via CR?) for anyone who needs a workaround for something unforeseen until we make API changes in D12/PHP8.4?

Thanks for creating this issue.

I'll try to explain how my original issue came about in my setup and perhaps it could help with writing a test.

My site has the groups module enabled and some pretty complex permission configuration.

The issue cropped up in integration tests, so it's easy to reproduce in my setup, but difficult to distill down to a Kernel test.

But this is the scenario:

1. A route + controller with a custom access check which iterates over a users group memberships and makes calls like this
$access->orIf(GroupAccessResult::allowedIfHasGroupPermission($group, $account, 'permission));
This route is placed in the main menu of the site.

User 1 and User 2 have different group permissions

2. As user 1, visit a route related to a group. e.g. /group/1/add_member and check results
3. As user 2, this same group route - Get Error.

We get as WSOD because of the NULL returned from the an ->entity call in the group module (specifically GroupRelationship::getRelationshipType() which returns $this->get('type')->entity

I can see a warning like Warning: Undefined property: Drupal\Core\Field\EntityReferenceFieldItemList::$entity in logs before the critical error because the entity is now NULL.

The order of users logging in (who have different access rights) doesn't matter, it's just that they have different cache contexts I assume.

The MR here fixes it. Nice!

> I'm not sure how the global flag would help with locating the workaround inside __get(), because that has no knowledge of entity reference fields or access to the storage handler

I was wondering if that flag would be so global that it wouldn't be about a specific storage or even the entity system. \Drupal\path\Plugin\Field\FieldType\PathFieldItemList::computeValue currently uses the storage directly and therefore bypasses the fiber suspend logic (and also a possible static cache), but it's imaginable that something hits an alias lookup within a magic method? So it wou'dn't be a skip-fiber-in-entity-load flag but a skip-all-fiber-stuff. A little bit like the FiberResumeType we added, but in reverse.

So it wou'dn't be a skip-fiber-in-entity-load flag but a skip-all-fiber-stuff.

This sounds like a really good idea for new API, but is that something that could be included in a patch release?

I think this is good enough for now. Maybe this prevents bulk loading in a few cases when it would be safe to do so as we don't know if that method is called within a magic __get() or not, but that's better than the pretty common fails on contrib tests and real projects that we're seeing.

I've seen failing tests in the core views as fibers issue (which would likely make this far more common even if we fix the specific scenario by not using magic there), in paragraphs, webform, a groups module and more. It's very confusing when you run into it and while the workarounds are usually easy, understand what you have to change is not.

I think that's more than enough reason to make this a major issue and committing a somewhat ugly workaround like this.

+1 for RTBC bc of urgency (maybe this should be critical?), but I think it'd be good to create a follow up to write a test. Since, per #7, it will help if/when we have a more robust solution/API for this later.

One concern I have is a that a considerable amount of ->entity property calls are going to use an already loaded entity. Just like getTarget() can't figure out if the call came through __get(), this currently can't know if the entity is already loaded or not. I'm not sure what the overhead of this is and it might not be measurable, but it's there. It's only when there is a fiber, but a surprising amount of logic runs within a fiber because rendering always uses one.

In ERR, I recently added the ability to check whether the target is already loaded, see \Drupal\entity_reference_revisions\Plugin\DataType\EntityReferenceRevisions::isTargetLoaded, \Drupal\entity_reference_revisions\Plugin\Field\FieldType\EntityReferenceRevisionsItem::isEntityLoaded, this is used to check if an entity is already set, which fixes some edge cases of replacing changed entities, see #3281020: referencedEntities: Use loadMultipleRevisions instead of loadRevision, core has an issue for this as well: #2826717: EntityReferenceFieldItemList::referencedEntities might not return up-to-date entity objects.

Long story short, if we were to add the same capability to regular ER fields to fix that issue, we could optimize this to only use a fiber if we're going to load an entity. That might not happen before D12 anyway.

One more comment on the MR, since I think we only need the fiber trap for the entity property.

Ran the test-only job and it fails as expected. Nice!

I think there's a way to consolidate the two test methods into one by passing in a boolean flag for whether to test the list or the item and using a data provider, but I'm not sure that's completely necessary.

RTBC +1 otherwise.

I spent much of today discussing this issue with @catch - I think we're in the best possible place but I do think if we add anymore fiber logic like the entity loading stuff we will need to consider if it is called from __get() so that we can avoid this in the future.

Follow-up for __isset() in #3590537: Workaround PHP bug with fibers and __isset()