Sort components by module at discovery time
Closes #3493813
Merge request reports
Activity
305 305 if (!empty($validation_errors)) { 306 306 throw new IncompatibleComponentSchema(implode("\n", $validation_errors)); 307 307 } 308 309 // Sort the definitions by module weight, order for theme components @catch I am not sure what this comment means. It seems you have moved this block of code from ComponentPluginManager.php and also refactored it. Were you intending to delete it when you made this comment but changed you mind and moved it instead? If so, this comment could be updated.
Edited by andrew farquharsonWhere you state 'is irrelevant here' is confusing, is it not? Since 'here' refers to the file from which you have moved the code, does it not? That is, the source file. Assuming these changes are merged into core, once the merge happens, for someone reading the code and if the code is not a git clone therefore they cannot search the git log, how would they know that this block of code has been moved from another file? And so understand what you mean by '... is irrelevant here'?
Edited by andrew farquharson"here" refers to the proces of sorting the components that happens below this comment, not the old or new file. It's meant to answer the question "components are from themes or modules, why does this only care about modules when sorting". I'm open to suggestions on how to make clearer, could explicitly refer to ComponentNegotiator::maybeNegotiateByTheme()" for example. maybeNegotiateByModule() could in turn explain that components from modules are already sorted, I only kept that protected method for BC fwiw, really not much going on there anymore.
'Sort the definitions by module weight, order for theme components is irrelevant here and determined at runtime.' could be changed to 'We determine the order of the definitions by sorting the module weight. Whereas the order of the theme definitions is irrelevant at this point in the code. It is instead determined at runtime.'
That might be a bit verbose, but I think gets over the message.
Or a simpler change would be to just change '..order for theme components..' to '..the ordering of theme components..'.
Edited by andrew farquharsonchanged this line in version 6 of the diff
126 130 * The negotiated plugin ID, or NULL if none found. 127 131 */ 128 132 private function maybeNegotiateByModule(array $candidates): ?string { 129 $module_list = $this->moduleExtensionList->getList(); 130 if (!$module_list) { 131 return NULL; 133 foreach ($candidates as $candidate) { 134 if ($candidate['extension_type'] === ExtensionType::Module) { it's very clear that this class was written by someone who prefers functional programming/array callbacks over loops, but since we specifically want to return the first, this seems more efficient.
Not that it matters really, I'm not quite sure why we even support multiple modules providing the same component, that seems like such a weird edge case, it's one thing that themes can override components of their base themes, but two separate modules? Feels like this was just written to have something comparable to themes, there also don't really seem to be tests for this, certainly not tests that specifically test this implementation as nothing failed.
306 306 throw new IncompatibleComponentSchema(implode("\n", $validation_errors)); 307 307 } 308 309 // Sort the definitions by module weight during discovery so that it can be 310 // cached. Components provided by themes are sorted at runtime in 311 // \Drupal\Core\Theme\ComponentNegotiator::maybeNegotiateByTheme() as their 312 // order can vary based on the active theme. 313 $module_list = $this->getModuleExtensionList()->getList(); 314 $sort_by_module_weight_and_name = static function (array $definition_a, array $definition_b) use ($module_list) { 315 $a_weight = $module_list[$definition_a['provider']]?->weight ?? -999; 316 $b_weight = $module_list[$definition_b['provider']]?->weight ?? -999; 317 return $a_weight !== $b_weight 318 ? $a_weight <=> $b_weight 319 : $definition_a['provider'] <=> $definition_b['provider']; 320 }; 321 uasort($definitions, $sort_by_module_weight_and_name); added 40 commits
-
9d9af2fa...101e0606 - 39 commits from branch
project:11.x
- 992d91f2 - Merge branch drupal:11.x into 3493813-drupalcorethemecomponentnegotiatornegotiate-uses-a
-
9d9af2fa...101e0606 - 39 commits from branch