Problem/Motivation
Part of #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent
Convert all template_preprocess
Steps to reproduce
Proposed resolution
* Move all template_preprocess functions in update module to ViewsThemeHooks, including hook_theme itself
* Deprecate the old functions and move them to views.module, so that the file definitions can be removed
* Update all references, including direct calls in tests and @see/@covers in tests and templates.
* Figure out how to define initial preprocess callbacks based on the plugin definition and theme name.
Remaining tasks
User interface changes
Introduced terminology
API changes
Data model changes
Release notes snippet
Issue fork drupal-3535680
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:
- 3535680-convert-templatepreprocess-in
changes, plain diff MR !12709
Comments
Comment #3
berdirThis might not be as hard as I feared, but it does depend on this central piece of magic:
I think we generally want less magic like this (service providers), but it's not enforced, just a default, and it feels in line with the kind of magic that this hook does.
There is also the bit about theme_file that plugins can in theory provide, but the new attribute classes are actually missing that and I want to deprecate the underlying 'file' key as well, so I just went ahead and added that deprecation right now. There are a bunch of them in contrib, but again, they would need to resort to a plugin definition alter hook for this in D12 with attributes anyway. And all they need to do for D12 compatibility is move the functions into their .module file (or convert to initial preprocess, but that would require 11.3 unless we backport this)
Comment #4
lendudeLeft a small comment on de MR. Might just be me reading the flow wrong.
Yeah the code in #3 pretty magic, but it's not too bad I feel. It does need some figuring out what's going on, so if others agree this is the best way to go, it would benefit from a comment or being moved to a private method to document what is going on I think
Comment #5
nicxvan commented@lendude does @berdir's response cover it for you?
Comment #6
lendudeYeah that clears up my question! Thanks.
Comment #7
nicxvan commentedI'll do a full check when I get a moment
Comment #8
nicxvan commentedOk I did my review too:
No double _
All deprecations for the functions are __FUNCTION__
Versions are all correct
The functions call the correct method
I haven't done the zebra check yet but that probably won't work on this one.
Two comments on the MR
And the change records mentioned in slack are needed.
Comment #9
nicxvan commentedMR comments have been addressed!
Per 8 and a double check I think this is ready.
Change record also looks good!
Comment #10
nicxvan commentedI reviewed the new cr, looks good, the new commit was just to update the cr link
Comment #11
berdirI now also created the second CR about theme_file (which, as mentioned in the CR, is basically already deprecated in D12 as we forgot to add it to the attribute classes and rarely used, so I thought it's OK to explicitly deprecate for D12).
FWIW, I'm not entirely happy about the approach that I implemented, writing the CR and explaining it reminded me of that again. Too much magic. An alternative is putting it on on the specific plugin class, that would be a much better location, but I don't really see a way to make that work with DI, it would need to be a static method. The ThemeHooks class approach has the advantage that DI works. But the whole feature is pretty magical already and entirely optional. You can alter it, you can also not define a theme key and fully define it yourself.
Comment #12
catchA few merge conflicts in this one unfortunately.
Comment #13
berdirRebased, only had two conflicts on test classes, some use and docblocks in core/modules/views/tests/src/Kernel/ViewsPreprocessTest.php and core/modules/views/tests/src/Kernel/Handler/ArgumentSummaryTest.php. Should be fine to set back to RTBC assuming that test are green.
Comment #16
catchI wonder a bit if there's a way to deprecate the magic theme stuff altogether, but this isn't the issue for that and I'm not sure exactly how we'd do it at all (except purely via change record?).
Committed/pushed to 11.x, thanks!