Skip to content
Snippets Groups Projects

Sort components by module at discovery time

Closes #3493813

Merge request reports

Code Quality is loading
Test summary results are being parsed

Closed by catchcatch 5 months ago (Feb 6, 2025 6:18pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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 farquharson
  • I don't get the question. Moving this here is the whole point and the comment I added is to explain what it does and why it's ok that sorting for theme-provided components is not defined.

  • Where 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 farquharson
  • Sascha Grossenbacher changed this line in version 6 of the diff

    changed this line in version 6 of the diff

  • Please register or sign in to reply
  • added 38 commits

    Compare with previous version

  • 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.

    • I can tolerate loops, but as you guessed I prefer a different style.

      This looks good.

    • Please register or sign in to reply
  • 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

    Compare with previous version

  • added 1 commit

    • 1f3f7e3d - Update file ComponentKernelTestBase.php

    Compare with previous version

  • closed

  • Please register or sign in to reply
    Loading