Problem/Motivation
This ticket is to target the deprecations template_preprocess discovery, including file/include callbacks.
Likely also _some_ but not all deprecate template_preprocess callbacks, such as those in core/includes/theme.inc.
Note: There is one overlap with theme engines. I've decided to remove theme engine preprocess callbacks here as well.
Note: This also removes $this->root as the first constructor argument from ThemeManager and Registry. Doing BC there would be pretty tedious as it's the first argument and we'd need to reshuffle all of them, remove types, do BC and so on. There are only very few contrib subclasses. bootstrap _will_ break, but it will need adjustments anyway as it hardcodes the constructor and does all kind of weird things: https://search.tresbien.tech/search?q=%22extends%20(ThemeManager%7CRegistry)%22&num=50&ctx=5.
Steps to reproduce
Proposed resolution
Remaining tasks
Ensure template_preprocess_views_mini_pager comments are cleaned up.
Create follow up issues:
- https://www.drupal.org/project/bootstrap
- https://www.drupal.org/project/bootstrap3
- https://www.drupal.org/project/component_connector
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3575542
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3575542-remove-deprecations-related
changes, plain diff MR !14890
Comments
Comment #2
berdirComment #3
berdirComment #4
berdirComment #6
berdirSo, I might have gone a bit overboard, but since we're already making changes to the Registry constructor anyway, I realized there's another issue that we can clean up properly. We already have the optional $theme_name constructor parameter, so I couldn't make keyValueStore required.
The thing is, that theme_name parameter has been bugging me for *many* years. It's only used in a few kernel tests that want to test the registry for a specific theme. But we already have a way to do thaat, by setting the current active theme either through config or an explicit API. This significantly complicates Registry, we need the themeInitialization service just for that. With this ability removed, init() is now a single line to get the current theme. There was also an initialized flag, but this flag hasn't been set to TRUE since 12 years.
The tests are updated to get the regular registry service and set the current theme. In the one test that needs multiple, all I need to do is switch out the currently active theme before calling get() on the same service.
Comment #7
berdirComment #8
nicxvan commentedThis is so satisfying, it looks so good, I just have a few questions.
Since we're already doing so much with the constructor already should we just go all the way with property promotion, we only want to document $this->registry but we can do that on the constructor I think.
ThemeTest references template_preprocess_html which we should probably clean up.
ViewsProcessTest references template_preprocess_views_mini_pager it's probably out of scope to clean that up here.
Finally there is a comment
* @todo Replace local $registry variables in methods with $this->registry.which I think we can delete.I confirmed all of the deletions in theme.inc are only the deprecations.
I pulled it down and reviewed Registry and reviewed it.
Then I searched for template preprocess.
Those test clean ups are almost as satisfying as the theme.inc.
I added links to the three modules extending Registry so we can create follow ups once this gets in.
I think this is actually pretty close.
Comment #9
dcam commentedI worked on reviewing this, but I didn't make it through checking all of the template preprocess functions before having to call it a night. I left a few comments on the MR.
If you run
grep -rn "template_preprocess_links" corethere are about 10 lingering references to it throughout Core. At least some of these seem to be referring to the docblock of the former function. These references should be updated.Comment #10
berdirAddressed the review, did property promotion and autowire, restored the removed functions that are scheduled for D13 removal, cleaned up template_preprocess_links() references.
Comment #11
berdirautowire has some unexpected side effects around the kernel, I have undone that, although I think it would be nice if we could find a better solution that doesn't require injecting the kernel here, but we already do a lot/too much for one issue.
Comment #12
nicxvan commentedGood catch in 9. I only checked we did not remove any non deprecated code I forgot to check the version.
Also the reason I didn't see those template preprocess references is because I accidentally restricted my search to *Test.php files.
I'll review the remaining functions later if dcam doesn't best me to it.
Comment #13
dcam commentedFYI, I just sat down to resume my review.
Comment #14
dcam commentedThank you for checking those removed theme function references before I got back to them. I double-checked them and didn't find anything.
Thank you for considering my feedback. I don't have anything else to add to my review. It sounds like @nicxvan was only planning on reviewing the other theme functions. Since that's pretty straightforward and I did it I will go ahead and mark this as RTBC.
Comment #15
nicxvan commented+1 rtbc
Comment #16
nicxvan commentedJust found something in the theme engine issue
Comment #17
nicxvan commentedNevermind, I was looking at the wrong tab, this issue is still RTBC
Comment #19
catchCommitted/pushed to main, thanks!