Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Follow-up from #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter(), which we wanted to mark as fixed since the code is in to preserve the intent of that issue. We need @code sample in the documentation, and tests for when we send an array of hooks to theme() (via drupal_render()).
Proposed resolution
Finish it up! :)
Remaining tasks
- Docs in theme.api.php fleshed out with @code samples.
- Test coverage for use cases when an array of hooks is passed to theme().
User interface changes
n/a
API changes
n/a
Related Issues
#1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter()
Comment | File | Size | Author |
---|---|---|---|
#42 | add_code_sample_and-2111079-42.patch | 17.13 KB | mohit_aghera |
#38 | interdiff-35-38.txt | 2.04 KB | Mac_Weber |
#38 | add_code_sample_and-2111079-38.patch | 16.16 KB | Mac_Weber |
#37 | interdiff.txt | 2.37 KB | star-szr |
#37 | add_code_sample_and-2111079-35.patch | 16.22 KB | star-szr |
Comments
Comment #1
star-szrOn it.
Comment #2
star-szrFor the record I am still chipping away at this, I've started work on the tests. If anyone else wants to work on the docs that would be more than welcome!
Comment #3
star-szrFrom my notes, a nice description via @webchick:
Comment #4
star-szrAfter many delays here's a first pass at improving the docs, so far I haven't came up with a great example for hook_theme_suggestions_HOOK_alter(), ideas on that and improving the hook_theme_suggestions_HOOK() example welcome.
Comment #6
markhalliwell4: 2111079-4.patch queued for re-testing.
Comment #8
star-szr4: 2111079-4.patch queued for re-testing.
Comment #10
star-szrCleaning up tags and here is a new patch that should do everything it needs to. Should be ready for review as long as it's green.
Comment #11
star-szrA few minor fix-ups, and thought it might be worth fixing up the routing file here because I didn't do it correctly in the first place in #1751194: Introduce hook_theme_suggestions[_HOOK]() and hook_theme_suggestions[_HOOK]_alter().
Comment #12
star-szrRerolled.
Comment #13
star-szrRerolled again.
Comment #14
star-szrTrying out @thedavidmeister's core review bonus idea because I reviewed his patch at #2191101-7: Replace calls to theme() with drupal_render() in Views plugins.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedI have a suggestion here, if this is a code sample for hook_theme_suggestions_HOOK and a newbie might not have a clear understanding of what hook and HOOK are, the sample would be clearer if they weren't both "block" in the sample. MYMODULE_theme_suggestions_MYHOOK maybe?
Other than that, I think the docs are good.
For the tests, I see we're only testing the case that we add new items to the suggestions array in the alter hook. In reality, after reading the comments/docs I'd expect just about any array manipulation I want to do to arrays in the alter hook to work - not just addition of new suggestions.
That means that we should add tests around altering for both deleting existing suggestions from the array (probably reasonably common) or directly editing existing suggestions (likely less common but probably still justifies test coverage).
#2094585: [policy, no patch] Core review bonus for #2173655: Refactor theme() to _theme(); make it a private API to discourage module developers from circumventing the renderable build system.
Comment #16
star-szrThanks @thedavidmeister! Reassigning so I can have another go at this.
Comment #17
star-szrI haven't been able to get to this, so unassigning for the time being to free this up so I can focus on #2201781: Pass all theme hook suggestions to theme preprocess functions to allow for suggestion-specific overrides.
Comment #18
star-szrRerolled, only minor changes to make this make sense and work today, most of which can be seen in the interdiff other than adding
public
to the controller methods which I did as part of conflict resolution.Comment #19
star-szrComment #20
star-szrRerolled after #2430909: hook_theme_suggestions() hook_theme_suggestions_HOOK() documentation is incorrect about how the hook is invoked, minor changes to line up with that issue.
Comment #21
star-szrHelps if you attach the file :)
Comment #22
star-szrComment #24
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedI think this is the correct status for this issue.
Marking it to retest to see if we need a reroll
Comment #25
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedPatch does not apply anymore
Comment #26
hussainwebRerolling.
Comment #27
hussainwebSince #2632996: Remove @todo tags at functions hook_theme_suggestions_HOOK() and hook_theme_suggestions_HOOK_alter() in file Theme.api.php was closed as duplicate in favour of this one, I am updating the suggested commit message to include contributors from there.
Comment #31
Mile23No longer applies.
Comment #32
Mile23Comment #33
Lal_Patch re-rolled.
Comment #37
star-szrAdding the credits from #27 in our new fancy way :)
Updating the reroll to undo the short array syntax reverts.
Comment #38
Mac_Weber CreditAttribution: Mac_Weber as a volunteer commentedFixed array styles.
Comment #41
markhalliwellComment #42
mohit_aghera CreditAttribution: mohit_aghera as a volunteer and at Axelerant commentedPatch was not being applied to 8.6.x branch. Re-rolling for 8.6.x
Comment #43
markhalliwell8.x doesn't have
drupal_is_front_page()
.Can we, instead, just base this on a real use case like system_theme_suggestions_maintenance_page()?
This is modifying the existing code example for
hook_theme_suggestions_alter()
, which shouldn't include the base theme hook per the example.Furthermore, the
@todo Add @code sample
inhook_theme_suggestions_HOOK_alter()
hasn't actually been replaced with this patch.---
Also, this is just documentation changes and a couple more tests. These aren't "major" changes.
Comment #44
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved "Needs Reroll" tag.