preprocess_HOOK__candidates in themes can be assigned to module invoke maps

I think there is something in:

 foreach ($grouped_functions[$first_prefix] as $candidate) {
         if (preg_match("/^{$prefix}_preprocess_(((?:[^_]++|_(?!_))+)__.*)/", $candidate, $matches)) {
           if (isset($cache[$matches[2]])) {
             $level = substr_count($matches[1], '__');
             $suggestion_level[$level][$candidate] = $matches[1];
             $module_preprocess_function = $prefix . '_preprocess_' . $matches[1];
             if (isset($this->getPreprocessForSuggestions()[$module_preprocess_function])) {
               $invokes[$candidate] = ['module' => $prefix, 'hook' => 'preprocess_' . $matches[1]];
             }
           }
         }
       }

That needs updating.

The hooks are correctly discovered, we may need to split the preprocess for suggestions for mapping.

Or we add a type to the suggestions and use that...

At least a major, but probably critical.

I think this is ready for review assuming the tests pass.

@berdir pointed out there are multiple workarounds for this (add the template, implement the base hook and check variable state)

Also this exists in 10.x

We should still address this and figure out a better test, but this can remain normal.

Needs work to implement the test we discussed.

I pushed more explicit test coverage, didn't remove yours yet. We can build on the existing preprocess and the existing test for this and don't need to add more stuff.

Note that the assertStringContainsString() does not yet fail in HEAD, because it still works as a function as it goes through ModuleHandler::legacyInvoke() as invoke() falls back to that in case the listener isn't known. I wasn't sure if and how much existing explicit test coverage we have and fails in our bulk conversion issue, but I thought it makes sense to make this explicit in this scope, should be easier to debug in the future as we refactor things.

The trick is making it the active theme, because right now we just install it and don't make it active, so we can't render with it.

The nice side effect that I didn't include here yet is that this makes the weird constructor argument stuff for Registry unnecessary. Setting the active theme means we can just do $registry_theme = \Drupal::service(Registry::class); and the constructor and manual setThemeManager call and just use as a service like any other and it will get the theme through the theme manager as regular requests. Then we no longer execute a different code path just for those tests. I think we want to do this cleanup as part of cleaning up the Registry constructor, we can also drop the root parameter in D12 and then we can reshuffle the whole thing (with BC)

Ah that makes sense, and that cleanup sounds great, I can create an issue to track that in a bit.

I think your test looks good, but I want to review it closer in a bit.
These are the tests that had failures related to this fix:

• Theme (Drupal\Tests\system\Functional\Theme\Theme)
• Claro Views Ui (Drupal\FunctionalJavascriptTests\Theme\ClaroViewsUi
• Navigation Block Ui (Drupal\Tests\navigation\FunctionalJavascript\NavigationBlockUi)
• Ajax Block (Drupal\Tests\layout_builder\FunctionalJavascript\AjaxBlock)
• Image (Drupal\Tests\ckeditor5\FunctionalJavascript\Image)

I just stumbled on this, from a custom subtheme of ui_suite_bootstrap and some hooks such as

#[Hook('preprocess_paragraph__{bundle}')] 
#[Hook('preprocess_views_view__{view_id}')]

The MR seems to solve it.

Current MR snapshot

Can confirm the merge request fixes this in the theme I'm working on, thanks! Will this fix will make it into the next patch release of 11.3.x? I realize it can worked around but it was also really difficult to track down what was going wrong on my end, so I can imagine there are likely a good number of people getting bit by this. Regardless, thanks for the fix!

I think this is ready for review of you want to take a pass.

I did the actual fix so I can't mark it myself.

Test only fails as expected, I have not reviewed @berdir's tests super closely yet.

@nicxvan I think with the new RTBC rules, it's perfectly valid if I review your fix and you review the test coverage. If you check my feedback and remove the extra test coverage and review my test then I'm OK with setting this to RTBC afterwards.

Yes, I reviewed your test now, it's much cleaner and more resilient than what I was doing.

I've removed my tests.

I think the fix is ready for review.

Reviewed.

I addressed your feedback!

Thanks, I think this makes much easier to understand what we do there. As mentioned before, I think I can RTBC this, since I haven't touched the implementation changes while you have reviewed the tests I wrote for it.

Not sure about priority here. I think wasn't clear on my 10.x comment back then in slack. This is definitely a new bug, what I experienced in 10.x was something else that is also related to preprocess for theme suggestions, but is different. This is definitely a regression and probably could be seen as major.

This is also blocking: #3560851: Convert test_theme to OOP

Forgot I didn't bump this back to major after looking at 20 and 6

One minor issue I spotted

I've addressed your feedback! Thanks!

Committed to main and backported to 11.x and 11.3.x

Thanks folks