Move template_preprocess, _template_preprocess_default_variables into services

Converted the 6 year old patch to a merge requests. Rebase was messy (surprise!) and I will definitely still need to do another pass to clean up remaining references to template_preprocess(), update deprecation and drupal_static() stuff, but tests *are* passing, that part was easier than expected, so setting to needs review for now.

Starting to bringt this into shape.

Unsure on the BC layer on template_preprocess(). I checked https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22templ... and not a single one of those is for D8+, lots of copied core files and weird stuff.

I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.

For now, I didn't add a new public method to reset the variables, I just added it to resetActiveTheme(). Default variables acually depend on the active theme, and for the use case in user logout/login, it doesn't seem like bad idea to reset the active theme, since the active theme can depend on permission.

And per #41, that is literally the only use case. There are like 5 implementations of template_preprocess_default_variables_alter() hook in contrib, most notably the mgv project. And not one of them bothers with the reset. I'm not sure that hook should exist at all. There are so few use cases, you can do your own preprocess if you really want, provide twig functions and what not. But separate issue.

I added the @trigger_error() to template_preprocess(), but I'd suggest we just don't bother with drupal_static_reset()? I also didn't do a change record yet, same story. Do we really need to bother with a change record for something that has zero known usages? IMHO that just adds noise.

Change record for something like this is probably still useful, I haven't rebutted the code yet, but this should is a great clean up.

I reviewed this, one minor comment.

At first I questioned it being on ThemeManager or a helper, but I think it makes sense internal.

I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.

Makes sense to me.

For now, I didn't add a new public method to reset the variables, I just added it to resetActiveTheme(). Default variables acually depend on the active theme, and for the use case in user logout/login, it doesn't seem like bad idea to reset the active theme, since the active theme can depend on permission.

I think this also makes sense, but I'm not 100% sure. I do think it's a good thing to get rid of drupal_static_reset, worst case if others disagree we can create a new method to reset only these variables.

I also confirmed the search for template_preprocess were all previous to drupal 8 or distributions that had core copied.

This will definitely cause a conflict for the preprocess issue, but this look great.

I'll wait on the CR and give others a chance to review.

Created a CR: https://www.drupal.org/node/3500806 → , applied the suggestion.

This looks like a really good cleanup to me and I think makes sense on it's own - the CR mentions calling drupal_static_reset with a given key

Can we emit a deprecation error for that key inside drupal_static_reset?

This was discussed in slack, and unless I'm mistaken @larowlan confirmed the deprecation is not necessary because there are very few implementations of that hook and it's highly unlikely that it will need to be called mid request like it does in user, none of the implementations in contrib called drupal_reset_static.

Also this is going into a minor and there will be a CR.

Looks good!

Here is a link to the discussion referenced in 66: https://drupal.slack.com/archives/C079NQPQUEN/p1737757941674099
Also had a quick discussion about how this deprecation is a bit weird cause the values are automatically added. Calling the service is to preserve the current behavior even thought it shouldn't be strictly necessary.

catch → closed merge request !10910

This looks fine and I think we can live without the drupal_static_reset() bc layer given there are no known usages at all.

Committed/pushed to 11.x, thanks!