Many calls to ContextRepository::getAvailableContexts() due to entity upcasting

Reroll for 11.x.

For the API change mentioned in #28 and CR in #27

Here's a patch for 10.1.x branch.

This resolves an issue I was experiencing when attempting to apply runtime contexts to condition plugins. The condition plugins, specifically entity_bundle:node, was not able to evaluate b/c of missing context value.

        $contexts = $this->contextRepository->getRuntimeContexts($condition_plugin->getContextMapping());
        $this->contextHandler->applyContextMapping($condition_plugin, $contexts);

Sorry, I'm not able to help with CR or the issue summary; I'm not sure what exactly is needed.

Created MR based on #30.
Attaching the patch for 10.2.x branch

Ran into this today when doing some profiling. Tracing the code before finding this issue I also discovered that we load all the contexts to only use the language ones; I don't fully understand this subsystem but it seems like we could optimise here. LazyContextRepository::getAvailableContexts() takes 15% of the request time in some cases according to xhprof so if we can remove this entirely it looks like it would be a good performance gain.

Fixed the performance test assertions (decreasing some values!) and rebased.

Updated IS. Does this need a change record? Not sure who this would affect or who to write the change record for.

@Berdir thanks for the explanation - this is quite the mess and I'm not sure either how to immediately start untangling it.

To fix the immediate performance issue (all contexts are loaded but only two are ever used) I attempted a stopgap approach in MR!7222. This only loads the contexts that EntityRepository needs - if the site isn't multilingual then it doesn't load them at all. This also removes one of the @todos but replaces it with another :)

I think longer term that just passing around an optional $langcode would be a better idea and perhaps we should try to untangle it all in one go by switching to that, but without diving in and trying it I'm not sure of the full implications.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

On my test site MR!7222 shaves at least 2-3ms off every page load that invokes EntityConverter::convert().

I updated the main MR with an attempt to implement what I suggested in my comment, I also added the test from 📌 Unpublished translations should fallback to the original language Needs work because this should allow that one to pass as well. Didn't really test yet beyond that.

So my proposal basically works, but after working through 📌 Deprecate the "entity_upcast" operation specified in EntityConverter Active and #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing → once again, the operation/entity_upcast thing is not as straight forward as the langcode thing is, so I think that needs to stay for now to avoid too much complexity and change in a single issue. I think I can revert it to the point where the operation is just passed in as a regular array key, in sync with existing language fallback context parameters, without those pseudo-plugin-context-ids.

Not sure which MR is to be reviewed. Could one be hidden please?

longwave → changed the visibility of the branch 3158130-performance-fix to hidden.

Hidden mine, while it looks like it works I think Berdir's covers more ground in the direction we want to go in.

Still NW for now as it needs rebasing.

Resolved merge conflicts in the MR, then realized that the patch from #35 applies to 10.2.4. :shrug:

Rebased again.

Left some comments on the MR.

Had a go at a change record, @Berdir can hopefully correct my mistakes and flesh this out a bit more! https://www.drupal.org/node/3437685 →

Changing the argument defaults from NULL to [] means changing the interface, not worth doing here with all the other changes, let's just leave this as-is for now.

Did a review, will try to do that myself when I find time, but feel free to pick it up if you have time earlier, not exactly sure when I will.

Addressed my review. Also deprecated the constant, FWIW, there's not a single usage of this in all of contrib.

Added a question to the MR as it appears we're also implementing 📌 Unpublished translations should fallback to the original language Needs work here. If this is unavoidable I think we need a change notice for that issue explaining the change and also how to revert it back to the previous behaviour.

Found the problem, I needed to drop the $legacy_contexts and instead just pass $contexts through. Thanks for catching that!

And yes, that single 200/403 line is literally the whole test coverage that exists in core for the current entity_upcast behaviour, and we only added that in a separate issue as part of the translate-editable-content permission. There's that unit test mock, but that does not test what's going on inside EntityRepository.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Thanks - the explanation and fix makes perfect sense, and it seems in that other issue people are relying on that test assertion so let's keep it as is and defer fixing that until later (or never).

Everything else here looks great so marking RTBC, we have some other good performance improvements in 10.3 so would be nice to land this one as well.

Nice work untangling the various scope issues here, hopefully we can keep going in the related issues and make all this a bit more understandable.

Also pleased to see the performance tests pick up that there is some improvement (even if the improvement is not really that we're removing the cache get, at least we can see that something is happening less than it previously was).

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

Here's a patch for Drupal 10.2.6.