Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
@see #939462: Specific preprocess functions for theme hook suggestions are not invoked
-
Preprocess functions MUST be registered manually for hooks and hook suggestions.
#939462: Specific preprocess functions for theme hook suggestions are not invoked -
Registered hook suggestions MAY need to declare additional variables.
#1222254: Remove theme_task_list() and call theme('item_list__tasks') instead.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#133 | theme_registry-1886448.patch | 71.13 KB | dawehner |
#133 | interdiff.txt | 8.48 KB | dawehner |
#127 | interdiff.txt | 11.37 KB | dawehner |
#127 | theme_registry-1886448.patch | 67.21 KB | dawehner |
#125 | theme_registry-1886448-125.patch | 69.82 KB | dawehner |
Comments
Comment #1
sunClarifying scope/title.
Comment #2
sunFinally: Sanity.
Attached patch rewrites the whole thing. Fully works in manual testing, including installer. Let's see what the testbot says.
Started with a literal copy of the procedural functions (the base for the interdiff), followed by:
Maintained in the theme-registry-1886448-sun branch of the Platform sandbox.
Comment #4
sunSemi-final magic:
Register and inject theme registry as a proper kernel service.
Comment #6
sunD'oh, sorry. Forgot to update the installer accordingly.
Comment #8
dawehnerMarked #1881116: Create a theme registry service. as duplicate
Comment #9
fubhy CreditAttribution: fubhy commentedShouldn't we do that in a follow-up? (Not that I don't agree... The magic we currently use for that is extremely fragile.)
Should be $this->registry = $this->load($complete); in this case... However I think it might even be better to remove $complete alltogether. Either you invoke getRuntime or you invoke get. No need for this cross-mapping.
I would go even further and remove support for multiple theme engines altogether... But that's something we should do in a separate issue. We can do that AFTER the conversion to Twig is finished.
That looks rather redundant and might be abused/used incorrectly with horrible performance impact. (e.g. if a module simply want's to invalidate the theme registry but uses the API incorrectly and triggers a rebuild directly after that for no reason).
Actually, this method just clears the theme registry and doesn't rebuild it as advertised by the docblock.
Comment #10
andypostSuppose this needs benchmarks, probably overhead should not be visible
Comment #11
dawehnerAnother possible follow-up could be to move the globals like $theme, $theme_key into a service as well.
Comment #12
sunTestbot results for this patch should show more green.
Comment #13
fubhy CreditAttribution: fubhy commentedThese should all be protected. Accessing ->get() and ->reset() should be all you really need to do from the outside.
Architecturally I find this hard to review because when looking at the (old, copied) code for the theme registry it makes me feel really uncomfortable. I guess we have to start somewhere...
Also, what are we going to do with ThemeRegistry? Let's turn the runtime theme registry into a service too so we can inject 'theme.registry' and also get rid of ->getRuntime() entirely.
Comment #15
sunSubstantial progress, incorporating most of the above feedback:
The new DUTB RegistryUnitTest takes 1 second to complete, and during the entire course of this rewrite, I already thought of creating it, but got "distracted" by trying to make the patch pass all other core tests... stupid me! There's a ton of processing logic going on in the theme registry, which is completely untested right now. Now, the new RegistryUnitTest leverages the theme_test.module and test themes to verify that the contents of the registry equal our expectations (without providing actual implementations). I've added a couple of @todos there, which I'm still resolving currently.
Comment #17
sunFixed horizontal + vertical merging of (pre)process functions, added massive test coverage, as well as extensive docs.
Comment #19
sunEssentially, I've rewritten the entire build process for (pre)process functions and introduced a compilation step that sorts them correctly.
The good news is that I'm adding massive test coverage here, so we can perfectly look into replacing the current theme registry building and compiling with a simpler implementation in follow-up issues. For now, my goals are limited and focused:
1) Turn the entire thing into a proper service.
2) Resolve the epic bug of (pre)process functions for hook suggestions.
3) Add extensive test coverage as dedicated DUTB unit test for the registry functionality only - i.e., the
theme()
counter-part is explicitly not involved.Let's see what the testbot has to say about this one. :)
Comment #21
cweagansThere's already an issue open for that. I'm not sure why we'd ever want to keep that functionality, but some people seem to want it. I don't particularly care about it that much, so I haven't put much energy into arguing that point. #1537050: [meta] Should we keep / improve multiple theme engine functionality?
Comment #22
dawehner@sun
How can we help to move forward on this issue? Have you made some progress, which is not yet part of that issue?
Comment #23
dawehnerLet's first start with just a rerole, which doesn't fail completely and then try to fix the bugs.
Comment #24
dawehnerFixed some of the preprocss functions in the views_ui.module file
Injected the module handler and use it on the registry.
Comment #26
dawehnerFixes the scanning of twig files and a preprocess function for exposed forms in views.
Comment #28
dawehnerUpdates:
Comment #30
dawehnerFixed a single issue in the locale.pages.inc
Comment #32
dawehnerNow that the cache system is fully injected using a separate cache bin name but the same db table, doesn't work anymore.
Comment #34
dawehner#32: drupal-1886448-32.patch queued for re-testing.
Comment #36
dawehnerLet it be installed again.
Comment #38
dawehnerConverted the theme registry to implement the desctructableInterface and fixes some of the tests.
Comment #40
dawehner#38: drupal-1886448-38.patch queued for re-testing.
Comment #42
dawehnerThe ThemeRegistry Object still loads the theme cache entry for itself, so we can't change that now.
Comment #44
dawehnerThere seemed to be an old cache entry in there from D7, though this only is the problem for actual real life upgrade.
Sadly this doesn't fix the upgrade tests.
Comment #46
dawehner#44: drupal-1886448-44.patch queued for re-testing.
Comment #48
dawehnerThis is insteresting: I guess we could maybe solve this with #1786490: Add caching to the state system or at least we have to figure out why a cacheArray object is created and destroyed in CoreBundle->build().
Comment #49
xjmDiscussed in IRC: This is the
preservedpreferred way to resolve #939462: Specific preprocess functions for theme hook suggestions are not invoked, so let's make this a critical.Comment #50
BerdirYou could override __destruct(), do nothing there and instead add destruct(). I did that in AliasWhitelist, I think.
Comment #51
dawehnerSadly this doesn't fix the upgrade tests, maybe it helps for the ones, which doesn't fail locally.
Comment #53
BerdirI don't think this works in the installer as it requires compiler passes and we don't compile here I think.
Comment #54
dawehnerWow, after hours of debugging at least the twig debug test got fixed.
Comment #56
dawehnerFixed the notice and rerolled for the services patch.
Comment #57
jibranGreat Work @dawehner. Yay! patch is green now.
Comment #58
dawehner#56: drupal-1886448-56.patch queued for re-testing.
Comment #59
dawehner#56: drupal-1886448-56.patch queued for re-testing.
Comment #61
dawehnerRerolled against moving template.php to $theme.theme
Comment #62
dawehnerForgot some files.
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedlets use \Drupal
Lets get rid of this
Drupal::service('theme.registry')->reset();
is a good enough one liner:)
Comment #65
dawehnerThanks for your review rootatwc!
Fixed all your points.
I wasn't able to fix the test yet. The problem is basically that the theme registry service is initialized too early in the
process, so it can't be switched to the new theme.
Comment #66
dawehnerRerolled the patch against some recent core changes.
Additional the test got fixed by resetting the global variables, so that the theme can call drupal_theme_initialize() again, and get the right default theme.
Comment #68
bcn CreditAttribution: bcn commented#66: drupal-1886448-66.patch queued for re-testing.
Comment #70
dawehnerForgot the Theme Registry file :(
Comment #72
ParisLiakos CreditAttribution: ParisLiakos commentedshould be baseThemes
Comment #73
dawehnerThis is green again!
Comment #75
dawehnerPosting the interdiff of the previous comment.
Comment #76
dawehnerComment #77
dawehnerFixed some comments + the review from #72
Comment #79
dawehner#77: drupal-1886448-77.patch queued for re-testing.
Comment #81
ParisLiakos CreditAttribution: ParisLiakos commentedthis edit fail..is weird..the array order changes.
lets see this green again..and then decide what to do with all these todo's
Comment #83
dawehner#81: drupal-1886448-81.patch queued for re-testing.
Comment #85
dawehnerI know this still fails, but this is just a rerole.
Comment #86
dawehnerWow, I needed more then a week for this line ...
Comment #88
dawehnerThere might be also an issue on module_enable, see http://drupal.org/node/1975354#comment-7398450
Comment #89
dawehner#86: drupal-1886448-86-mega-debugging.patch queued for re-testing.
Comment #91
dawehnerJust a rerole + potential fix, at least all the theme and image tests run fine.
Comment #92
tim.plunkettI think the @todo's for using $this->registry throughout should be resolved before commit, but the rest are mostly just baggage from theme.inc.
This worries me, since there is work being done to make services lazy-load. Then this would stop doing anything.
I've not seen @global before, but dawehner says it is standard phpdoc
This is going to be a huge PITA, but it does help resolve ambiguity...
Contains
Needs a docblock
All of these need visibility public/protected
This seems like something sun shoved in... Oh well.
Not okay :)
Beautiful!
Even themes have to specify these? :(
Comment #93
barraponto CreditAttribution: barraponto commentedYeah, as a themer, I think declaring preprocess functions hurts my DX. Unless my base theme does autodetection magic (but it would be a regression, anyway).
Comment #94
dawehnerIt feels okay to accept this for the maintenance theme, as it's not on the actual site.
See http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria...
As far as I understood #939462-170: Specific preprocess functions for theme hook suggestions are not invoked right it feels okay, but yeah we need a discussion whether we want to go with this route.
Comment #96
Fabianx CreditAttribution: Fabianx commentedTwig Initiative here, we don't like to specify preprocess.
As you are rewriting this anyway: This is how we would like our theme system to work:
- Still needs a registry, but really only for registering hooks and finding templates - DONE.
No more magic, very very similar to FORM API.
Example for terminology:
- hook = hook
- themeID = node
- suggestionID = article
1.
hook_theme - as is
2.
home_theme_suggestions_alter
home_theme_suggestions_themeID_alter
The only way to add or remove suggestions. Replaces $variables['#theme_suggestions']. Also allows altering the default found suggestion, similar to the display_id in views_preprocess_view.
3.
hook_theme_prepare
hook_theme_prepare_themeID
Prepares variables for output in a theme (like a link moving from path, text to url, check_plained text), replaces template_preprocess*
4.
hook_theme_prepare_alter
hook_theme_prepare_themeID_alter
hook_theme_prepare_themeID_suggestionID_alter
Allows altering the variables, like form_formID alter for the suggestionID.
5. Output
- theme function -> HTML
- template -> HTML
- special render = TRUE / FALSE flag as third parameter to return variables as is -> array().
And thats it.
Clean, extensible, no magic, and works exactly like form API.
and _alter is fast by definition and the right thing to do here.
Comment #97
fubhy CreditAttribution: fubhy commented#96 this... +1000 totally agreed
Comment #98
barraponto CreditAttribution: barraponto commentedhook_theme_suggestions_alter rocks.
Comment #99
c4rl CreditAttribution: c4rl commentedWith regard to #96 and the general progress here, I'm adding #1899454: [meta] Refactor Render API as a related issue to the summary. There's some potential overlap with both of these.
Since I initially filed that issue I've been preoccupied with the Twig conversion, but now that Twig has gained some solid momentum, I will be diving in to Render and theme-system related issues full speed.
Comment #99.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #99.1
jenlamptonrewrite with some context :)
Comment #99.2
jenlamptonadded back C4rl's render api link
Comment #99.3
quicksketchCopy of the revision from May 26, 2013 - 15:46.
Comment #100
jenlamptonAfter further consideration, I think maybe making the theme system work how we want it to work, and turning it into a proper service may be two separate issues. I've created #2004872: [meta] Theme system architecture changes.
If this is a mistake and you'd prefer to handle all the new hooks here as well, than feel free to close that issue as a dupe of this one, and update this summary to include those changes as well. (I'd originally done that, but thought better of it)
Comment #101
BerdirRe-roll and changed ThemeRegistry to use a CacheCollector, see interdiff.
Crazy amount of conflicts, let's see how this goes.
Comment #103
BerdirUps. This might fix the installer.
Comment #105
BerdirFixed a merge conflict in user_theme(), a typo in ThemeRegistry and added a missing preprocess definition, there are probably more of those.
Comment #107
Berdir#105: theme-registry-1886448-105.patch queued for re-testing.
Comment #109
BerdirLost the interdiff from #103.
Comment #111
fgm#109: theme-registry-1886448-109.patch queued for re-testing.
Comment #113
jenlamptonIs this really critical? I don't think it's blocking anything we're working on since we have new ways to solve #939462: Specific preprocess functions for theme hook suggestions are not invoked
Bumping to normal.
Comment #114
dawehnerThis patch potentially allows to enable lazyloading, so things like rest requests can serve the data faster.
Comment #115
catchThe theme registry is already loaded on demand (i.e. via the first theme() call, not when the theme system is initialized). I don't think this is a release blocker, but it's also unlikely to cause any public API changes (or no more than CacheArray did in Drupal 7), so assuming that's the case it could get in after this week once it's ready.
Comment #116
Crell CreditAttribution: Crell commentedTagging. This would be good for performance to not always initialize the theme system, but would be an API change at this point. Will need a decision from on-high to bump to major, after there's a good issue summary.
Comment #117
moshe weitzman CreditAttribution: moshe weitzman commented@Crell - read the comment before yours. We already lazy load the registry. Are you suggesting that we want to avoid theme init too?
Comment #118
andypostTheme should be always initialized while #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system is not in
Comment #118.0
andypostReverted to the previous revision, and added back a link to c4rl's render API issue
Comment #119
dawehnerDecided to give it another try and started with a new patch which tries to change no existing behavior.
Instead of using a DrupalUnitTest it also mocks everything in a proper unit test.
Comment #120
dawehnerComment #121
jibran80 chars
doc missing.
White space.
expand to 80 chars.
oops!!!!
Comment #123
dawehnerThis could be green now.
Comment #125
dawehnerFixed the last failure.
Comment #126
amateescu CreditAttribution: amateescu commentedSince we're touching those lines already, can we please rename the variable to $theme_registry? $hooks->has($hook) doesn't make any sense :)
You're removing much more than just theme_registry cache, I think the comment needs to be changed..
Unrelated change?
This looks a bit weird now because the build() method already has access to those arguments.
Another unrelated change?
You mean \Drupal\Core\Theme\Registry::init()?
::processExtension() :)
Double period.
Comment #127
dawehnerFixed the points.
Comment #128
amateescu CreditAttribution: amateescu commentedThe interdiff looks nice, so assuming the bot comes back green, I think it's ready.
Comment #129
Xano127: theme_registry-1886448.patch queued for re-testing.
Comment #130
Xano127: theme_registry-1886448.patch queued for re-testing.
Comment #131
alexpottThis function is still called in DisplayPluginBase::displayOptionsForm()
So either there are no tests for this or it is dead code!
The
use Drupal\Core\Cache\CacheBackendInterface;
can be removed from the top of the file. And also we can removeuse Drupal\Core\Utility\ThemeRegistry;
too.build() no longer takes any params
_theme_process_registry() has been removed by this patch - in fact here it should be changed to be the method that this docblock is documenting :)
Missing @param
drupal_theme_initialize() does not return anything.
$data
is no longer used so this line can be removed.Not necessary.
I think this should just be
@var TestRegistry
since this variable is not an instance of \Drupal\Core\Theme\RegistryComment #132
alexpott#131 means that this needs work :)
Comment #133
dawehnerGreat review, thank you!
Damn right, this was part of the last version before the retry.
The only issue I can think of at the moment is https://drupal.org/node/2049209#comment-7993037
Good catch.
Removed, thanks.
We can just point to ourselves.
You seem to use storm for review as well.
Fixed as well.
Comment #134
tim.plunkettOoh, I like the changes to Views in that last interdiff.
I think that adequately addresses @alexpott's questions.
Comment #136
tim.plunkett133: theme_registry-1886448.patch queued for re-testing.
Comment #137
star-szrYeah, the changes to Views look great! Thanks for all your work on this @dawehner and thanks to @alexpott and @amateescu for the detailed reviews.
Back to RTBC per #134 because the patch is green.
Comment #138
sunSad to see the originally contained improvements gone, but I understand that I was probably shooting for too much at once.
Thanks a ton to @dawehner and @Berdir for taking this on and getting the main change done! :)
Comment #139
alexpottCommitted 3cda830 and pushed to 8.x. Thanks!
Comment #140
dawehnerhttps://drupal.org/node/2137545
Comment #141
markhalliwellLooks good! Made a few changes to verbiage though.
Comment #142
dawehnerTthank you jibran and Marc Carver for these updates.
Comment #143
yched CreditAttribution: yched commentedLooks like the patch that got in still had a few @todos ?
Comment #144
star-szrWe did not go the route of manually declaring preprocess functions.
Comment #146
larowlanretro adding the theme service tag
Comment #147
sunComment #148
sun