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

Working on it, Assigning to me.

Added updated patch and addressed #3 as well.

1. +++ b/core/lib/Drupal/Core/Entity/EntityRepository.php
   @@ -133,13 +133,9 @@ public function getActive($entity_type_id, $entity_id, array $contexts = NULL) {
   +  public function getActiveMultiple($entity_type_id, array $entity_ids, array $contexts = []) {
   
   @@ -177,7 +173,7 @@ public function getCanonical($entity_type_id, $entity_id, array $contexts = NULL
   +  public function getCanonicalMultiple($entity_type_id, array $entity_ids, array $contexts = []) {
   

   I don't think we should be changing this here.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -105,24 +105,11 @@ public function convert($value, $definition, $name, array $defaults) {
        // @todo Consider deprecating the legacy context operation altogether in
   ...
   +    //   https://www.drupal.org/node/3031124
   

   as per #3 this should be removed entirely.

Working on the #8 comments.

@acbramley Thanks for the review,

As we are overriding the $contexts right after the function start then initialising the argument in defensive case the right way.

regarding point 2 do we need to delete the todo along with legacy context?

Updated the patch as per #11. I changed function definition in my last patch which was wrong.

Ops I missed the patch, Uploading it.

Re-rolled #13 for 9.4.

Added Patch against #18 in Drupal 10.1.x

Patch #13 not work in drupal 10.1.x-dev please reroll the patch.
Thank you.

#21 applied cleanly drupal 10.1.x-dev

The patch doesn't need a re-roll, it needs to be fixed as it is failing CI.

This patch is applying for 10.1.x clean. Also PHPstan CCF failed is fixed in it.
Thanks

It looks ready to go, all places are cleaned and no tests failed!

It needs change record to point what effect module developers can get as only one language context is available

Also issue summary needs update about API changes or language change could go to follow-up

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().

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

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.

Added a question to the MR as it appears we're also implementing #2978048: Unpublished translations should fallback to the original language 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.

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!