Issue #3158130: Many calls to ContextRepository::getAvailableContexts() due to entity upcasting
Closes #3158130
Merge request reports
Activity
added 116 commits
-
9d414f58...35a8fc6d - 114 commits from branch
project:11.x - 9ef019ae - Apply [#3158130-30] patch
- a35913ae - Update performance tests.
-
9d414f58...35a8fc6d - 114 commits from branch
137 137 $active = []; 138 138 139 139 if (!isset($contexts)) { 140 $contexts = $this->contextRepository->getAvailableContexts(); 140 $contexts = []; 141 141 } 224 224 return $contexts[$context_id]->getContextValue()->getId(); 
Right now, it's still in theory possible to pass in a context that's essentially forcing a specific language.
The issue that added this code somehow merged the concept the plugin context system with the g$context parameter for \Drupal\Core\Entity\EntityRepositoryInterface::getTranslationFromContext/hook_language_fallback_candidates_alter(), which is documented as:
* @param array $context * (optional) An associative array of arbitrary data that can be useful to * determine the proper fallback sequence.and is then passed to \Drupal\Core\Language\LanguageManagerInterface::getFallbackCandidates(), which documents the langcode and operation ('entity_view', 'views_query' (AFAIK unused, pretty sure that's a Drupal 7 entity API leftover thing) and locale_lookup) and data.
EntityConverter below is then however specifying the operation as 'entity_upcast' (and NOT entity_view) with that pseudo plugin context ID thing, which is then internally mapped back to just 'operation'.
The entity_upcast operation (which is not documented on \Drupal\Core\Language\LanguageManagerInterface::getFallbackCandidates()) is older than the plugin context stuff, but it's causing issues as well. See #2978048, currently getting a translation from an entity is different when you view that entity directly vs. viewing it through an entity reference or view, because the EntityConverter uses a non-default/non-documented operation. #3031124 instead proposes to remove that operation entirely, which makes sense, just not sure about the BC implications.
I think when that was added originally, it was just a misunderstanding of that, IMHO any time you call getFallbackCandidates() with an entity, you should use entity_view.
So, what I think we should do, not necessarily everything in this issue, but splitting it up is tricky as it's all partial changes on the same lines of code.
- Deprecate and remove the entity_upcast operation
- Deprecate the ability to provide a different operation through EntityRepository, it's an entity, it should force entity_view.
- Deprecate the attempt to merge this context thing with plugin contexts
- Allow to pass in a specific language as just 'langcode' (which right now you would actually need to provide as
['@language.current_language_context:language_content' => new Context(new ContextDefinition('language'), $language_object)]. That would mean that getContentLanguageFromContexts() would become just a BC layer, just in case there is code out there that did this and we can then remove it in D12 or whatever. - We could even consider to change $contexts to just $langcode, but would require more BC work, and I can't entirely rule out that there is another use case out there.
- Clean up the documentation around language fallback $contexts parameter and expected operations key parameter.
IMHO we have two options on how to get there:
Postpone this on [#3031124], remove the operation there, possibly with the test coverage from the other issue that tried to fix the entity_upcast problem with a workaround (copy paste of the hook), maybe also deprecate both the operation and the CONTEXT_ID_LEGACY_CONTEXT_OPERATION thing there. Then here deprecate the rest of the plugin context and language stuff, keeping just langcode. Or we just do both things at once in here.
Edited by Sascha Grossenbacher
added 1 commit
- 407a5223 - work around not yet existing change record, this is just a poc
added 1 commit
- dc4ad115 - unused uses and leftover context repository in unit test
added 42 commits
-
dc4ad115...f2165da9 - 41 commits from branch
project:11.x - dcc583df - Merge remote-tracking branch 'origin/11.x' into 3158130-many-calls-to
-
dc4ad115...f2165da9 - 41 commits from branch
added 1 commit
- ddb517eb - Merge remote-tracking branch 'origin/11.x' into 3158130-many-calls-to
added 1 commit
- da48ab58 - Merge remote-tracking branch 'origin/11.x' into 3158130-many-calls-to
added 42 commits
-
da48ab58...f2165da9 - 41 commits from branch
project:11.x - 9816a874 - Merge remote-tracking branch 'origin/11.x' into 3158130-many-calls-to
-
da48ab58...f2165da9 - 41 commits from branch
added 13 commits
-
9816a874...fc0aa24e - 5 commits from branch
project:11.x - 53ab3fea - Apply [#3158130-30] patch
- bfbacb38 - Update performance tests.
- ce04c4bf - Deprecate operation context, remove entity_upcast operation
- 76ae8a8f - Add test from 2978048
- 0b275785 - work around not yet existing change record, this is just a poc
- 383aeca9 - remove uses
- 1996c9dc - test and deprecation updates
- d50a2d5f - unused uses and leftover context repository in unit test
Toggle commit list-
9816a874...fc0aa24e - 5 commits from branch
504 504 } 505 505 } 506 506 507 /** 508 * Tests that unpublished translations fallback to the original language. 509 */ 510 public function testUnpublishedTranslations() { changed this line in version 14 of the diff
186 187 } 187 188 188 189 if (!isset($contexts)) { 189 $contexts = $this->contextRepository->getAvailableContexts(); 190 $contexts = []; 190 191 } 191 192 192 // @todo Consider deprecating the legacy context operation altogether in 193 // https://www.drupal.org/node/3031124. 194 193 $legacy_context = []; 195 194 $key = static::CONTEXT_ID_LEGACY_CONTEXT_OPERATION; 196 195 if (isset($contexts[$key])) { 196 @trigger_error('Providing an operation context to EntityRepository::getCanonicalMultiple() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. There is no replacement. See https://www.drupal.org/node/3158130', E_USER_DEPRECATED); changed this line in version 14 of the diff
added 2 commits
132 130 return $entity; 133 131 } 134 132 135 // Do not inject the context repository as it is not an actual dependency: 136 // it will be removed once both the todo items below are fixed. 137 /** @var \Drupal\Core\Plugin\Context\ContextRepositoryInterface $contexts_repository */ 138 $contexts_repository = \Drupal::service('context.repository'); 139 // @todo Consider deprecating the legacy context operation altogether in 140 // https://www.drupal.org/node/3031124. 141 $contexts = $contexts_repository->getAvailableContexts(); 142 $contexts[EntityRepositoryInterface::CONTEXT_ID_LEGACY_CONTEXT_OPERATION] = -
Per #45, I think this is one step too far for this issue. We can simplify this to just $contexts = ['operation' => 'entity_upcast'], but lets keep that bit and leave it to the two referenced issues. There are no fails, but see discussion in those two issues, there are people relying on the current behavior, so lets not break what we don't have to here.
This also means we have to remove/revert the test changes in content_translation/node, as that remains unchanged now for this issue.
