Circular dependency between bundle and data type info

Created a merge request with a failing test to show this issue. I guess the MR doesn't show up here, because I targeted the branch in the MR of 🐛 Statically cached derivative definitions cannot be cleared in any way RTBC . That's only because that already introduces a basic test for the entity bundle data types which I just amended here to show the bug.

Not 100% sure yet how to fix this, will see if I can find some time to play around with this and work on a fix.

OK, I guess the Gitlab Pipeline doesn't work if not targeting the proper repo. Leaving as is, though, for now, as it makes the review of the test change introduced here easier. Can of course easily change this and retarget the MR if this gets traction sooner than 🐛 Statically cached derivative definitions cannot be cleared in any way RTBC lands, but let's see.

Oh, totally forgot to link the merge request here, sorry: https://git.drupalcode.org/issue/drupal-3399559/-/merge_requests/1

Didn't expect to be able to fix this with a literal one-liner but, yeah, pushed a commit now that makes the test green.

The fix is really just duct tape, but having thought about this for the bit I'm somewhat convinced it's the best we can do. I don't see a way to resolve the circular dependency in principal: both sides of the dependency as described in the issue summary are valid and needed APIs. And we also have no mechanism to partially or incrementally build up these sets of data: either you have cached data type definitions or you don't in which case we must build all of them right now. Same goes for the (cached) bundle info. In theory one could devise a system where the entity system is able to dynamically amend an already existing set of data types. But that system does not and cannot exist in Drupal in its current form. So I think, unfortunately, building behavior around the expectations of when things are cached (or not) is the next best thing.

Marking this as needs review now (see above for the link to the merge request). If someone wants to actually review this and set this to RTBC I can re-target the MR against 11.x properly and then we'll see if this or 🐛 Statically cached derivative definitions cannot be cleared in any way RTBC lands first. The current MR just makes it a tad easier to review (at least I hope so).

Took a look at the linked issue and seems to have some failures.

Do you mean the MR? See #5

The MR from #6 has a composer failure and the tests were skipped.

Yes, I tried to explain that in #5. It's that way because the MR doesn't target the core branch but the branch from 🐛 Statically cached derivative definitions cannot be cleared in any way RTBC . I verified that the test passes locally and was looking for a review on the approach here (and targeting that other branch makes it a bit easier to review). But I guess that's causing more confusion than it's helping so I will retarget the MR.

tstoeckler → closed merge request !5576

Went ahead and opened another merge request against 11.x now. I rant the test-only pipeline and was surprised to see the test pass even though the non-test changes were correctly reverted. It took me a while to realize why, but the test actually suffers from 🐛 Statically cached derivative definitions cannot be cleared in any way RTBC . It totally makes sense because I had only found that one "by accident" when debugging this, but then when I later created this issue I didn't realize that the test actually needs that to properly fail without the fix. So marking postponed now (even though this could definitely still really benefit from a review).

🐛 Statically cached derivative definitions cannot be cleared in any way RTBC landed, so this is now genuinely ready for review.

Added the proposed resolution (vageuly copied from #7) to the issue summary. There is no functional change here, but for the actual bugfix, so I don't think this needs a change notice.

The MR feels very much like a sticking plaster. I'm not sure you'd expect different results from Entity::getTypedData() depending on when it is called.

Do you think that it is worth deprecating the recursive call into \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() and in a future version of Drupal throwing a exception when this happens.

Really tricky issue and great sleuthing on working about what is good on. The issue summary is impressive.

Thanks for taking a look @alexpott!

Basically the tl:dr; of the below is: would love to hear more of the thoughts behind your comment ;-)

Really tricky issue and great sleuthing on working about what is good on. The issue summary is impressive.

Thanks! That really means a lot.

The MR feels very much like a sticking plaster. I'm not sure you'd expect different results from Entity::getTypedData() depending on when it is called.

Not sure what you mean by this. One symptom of this bug is exactly that Entity::getTypedData() may return different results depending on what happened before. In particular, in the example in the summary $term->getTypedData() would incorrectly return a typed data instance of entity:taxonomy_term and then if for some reason only the typed data cache was cleared it would then correctly return an instance of entity:taxonomy_term:tags. Adding this to the issue summary in fact, because I think it's helpful context, thanks for bringing that up! So, yeah, the MR doesn't feel great, 100% agreed, but maybe you can elaborate what you mean by this specifically.

Do you think that it is worth deprecating the recursive call into \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() and in a future version of Drupal throwing a exception when this happens.

Again, not really sure what you mean by this, unfortunately. Will take some stabs at a guess, but maybe you can elaborate here, as well:

1. Are you suggesting to not have bundle-specific data types at all? Not sure how we would properly validate bundle-specific validation constraints if we were to do that. But that would very cleanly solve this issue by actually properly getting rid of the circular dependency. (To be clear, not necessarily implying you actually suggested this, just trying to explore all possible options here.)
2. Are suggesting to not allow fetching the typed data information when loading a (bundle) entity? In theory I guess this would be the less invasive half of the circle to break, as the use-cases described in the issue summary (while valid) seem less important than having bundle-specific data types at all (i.e. 1.) Not really sure how to achieve this, though, as this would mean calling $typedDataManager->getDefinitions() without a primed bundle cache would yield a (deprecation notice, but eventually an) exception?
3. Having written the above and looked at the issue again, I had an idea of a maybe less terrible way to solve this issue at hand, so just listing it here to make it easier to refer to down below: Avoid loading entities at all for the building the bundle information. We really only need the IDs and labels of the entities, so we could fetch those directly from the storage (either via an entity query directly or via a new dedicated method on the storage). The only problem would be entity types that override label() and do something fancy there, but maybe it's fair to deprecate doing that for bundle entities (because presumably we don't want to just wholesale deprecate overriding that method)? While throwing the deprecation notice / exception would then require something like the somewhat ugly method_exists($entityType->getClass(), 'label') it would at least be a way forward that does not depend on the internal state of the typed data manager.

I think we should explore #19.2 and #19.3 more.

Thanks @catch for taking a look.

Since I just encountered this beauty of a bug once again (yes, don't ask...) adding another suggestion to fix this here that was proposed by a certain ghost of drupal past on Mastodon during this year's Drupal Dev Days so that I don't have to dig that up again. Slightly altering/rephrasing the details, but I cannot take credit for the idea itself. Will refer to this as "Suggestion 4" for now:

• Rename getAllBundleInfo() to doGetAllBundleInfo() (or similar).
• Remove the fetching of the labels from doGetAllBundleInfo() and, thus, from the cache entirely. Instead just load the label dynamically in getBundleInfo(). (We could also consider adding a new getBundleNames or something if you just need the keys and not the labels, but that's a minor detail (and possibly a follow-up).)
• Re-instate getAllBundleInfo() to just call getBundleInfo() for all entity types. It will be slower than before but it's not a very common use-case to need bundle information for all entity types. It could also easily be deprecated and have the few cases where it's used in core just move to calling getBundleInfo() for the entity types that it needs.

I personally think this is the best suggestion so far. Would love to hear thoughts on this!