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.
Issue #1843752 by joelpittet, Sean Charles | tostinni: Convert views-view-grouping.tpl.php to Twig.
Meta issue: #1843738: [meta] Convert views module to Twig
Manual testing steps:
1. Copy the twig template into Bartik so that it actually gets used
2. Create 2 articles
3. Edit front page view to have grouping on *at least two fields* - grouping on one field won't trigger the view-grouping markup.
4. View the front page.
Comment | File | Size | Author |
---|---|---|---|
#45 | interdiff.txt | 807 bytes | joelpittet |
#45 | 1843752-45-twig-views-grouping.patch | 2.89 KB | joelpittet |
#38 | 2013-05-20 01.54.57 am.png | 17.59 KB | intergalactic overlords |
#36 | 1843752-27-twig-views-grouping.patch | 2.89 KB | Sean Charles |
#32 | 1843752-groupings-fail.png | 59.2 KB | jwilson3 |
Comments
Comment #1
joelpittetComment #2
mbrett5062 CreditAttribution: mbrett5062 commentedTagging.
Comment #3
FluxSauce CreditAttribution: FluxSauce commentedCommitted, thanks!
Comment #5
joelpittetMoving to core queue
Comment #6
joelpittetOk for real this time:) Seemed to pass related tests locally, let's open this baby up.
Left some doc todos in there if someone want's to step up to the plate?
Comment #7
joelpittettitle update
Comment #8
jastraat CreditAttribution: jastraat commentedHm - I think I included this in the patch here: http://drupal.org/node/1912606
Comment #9
star-szrYes, we ended up with a dupe. Closing this one since #1912606: Remove theme_views_view_grouping function is RTBC.
Comment #10
joelpittetResurrected. Only tpl converted. Grabbed pieces from #1912606: Remove theme_views_view_grouping function which has been stripped down to only the theme function removal.
Comment #11
joelpittettagging
Comment #12
joelpittetComment #13
joelpittetweird, tagging again
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentednitpicks:
+ * - view: The View object.
We should avoid calling it an "object" in the template.
+ * - grouping_level: Integer indicating the hierarchical level of the grouping.
I know "integer" means something in math, not just as a PHP data type, but can we call it "number" instead?
I'm assigning myself to do some manual testing.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedWhy on earth did we get rid of this? it's still true. I just spent 15 minutes trying to figure out why the template in the patch wasn't doing anything.
Anyway, steps to test:
1. Copy the twig template into Bartik so that it actually gets used
2. Create 2 articles
3. Edit front page view to have grouping on *at least two fields* - grouping on one field won't trigger the view-grouping markup.
4. View the front page.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commentedNote that the Pre-patch markup for this theme function is totally broken (it prints Array for content) and throws errors: "Notice: Array to string conversion in theme_views_view_grouping()"
Pre-patch markup (whitespace edited for easier diffing):
Post patch markup (not broken):
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commentedComment #18
thedavidmeister CreditAttribution: thedavidmeister commented@Cottser suggested in IRC that if I use the tpl.php file in HEAD it might fix the Array printing problem + errors, but it doesn't help at all. HEAD is broken however you look at it :(
Comment #19
thedavidmeister CreditAttribution: thedavidmeister commentedI'm not sure this is right since I managed to trigger the template without turning aggregation on at all.
Comment #20
star-szrThanks for all your work on this @thedavidmeister!
This is a novice task and 'needs work' for #14 and #15.
The fact that the functionality doesn't work properly or as documented is a separate issue altogether and should be handled separately from the conversion. It looks like Twig makes things less broken though :)
Comment #21
star-szrTagging for profiling.
Comment #22
joelpittet#1912606: Remove theme_views_view_grouping function May have something to do with it, but now doesn't exist in head. So re: #15 it is now the case that it is always used.
#14 should be done in this.
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedDon't forget the weird comment in #19.
I'm going to manually try #15 again but the template was definitely not getting called when I was testing on the 11th of May, which is a few days after when that issue was committed on the 8th.
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedCool, I can confirm #22 in that this template is now always called. I must have not pulled or something before I did the testing in #16.
So it's just the weird comment about aggregation settings now.
Comment #25
joelpittet@thedavidmeister not sure what #19 is trying to say, could you clarify?
Also, you're not crazy, that fix just got in yesterday into core so it would have been 'broken' when you tested.
Comment #26
thedavidmeister CreditAttribution: thedavidmeister commentedThis says the content is set via aggregation settings on the view, but if you look at the manual testing steps it's possible to trigger this template without aggregation enabled at all.
Comment #27
joelpittetHmm... ok I removed that note. I would have thought you would have needed aggregation on for that but I guess that makes sense.
Comment #28
thedavidmeister CreditAttribution: thedavidmeister commentedmissed one, in the preprocess comment.
Comment #29
deanbowles CreditAttribution: deanbowles commentedAssigning Issue from COH
Comment #30
jwilson3I've been using views for years but "grouping instruction" has zero meaning to me, and this is particularly confusing in the template, because the variable isn't used or demonstrated how it could be used.
Granted, this is the exact pre-existing documentation but, while eyeballs are on this, would it be too much to ask for some clarifications about what exactly is a grouping instruction?
Also, "The View object", could use some attention per comment #14, though, i presume that the view object is used in other templates from views, so make this a follow-up doc cleanup issue so all templates are documented consistently.
Feel free to shoot me for throwing up road blocks at the last minute. I'd be happy to open these two things as follow-up issues.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commented#30 - jwilson3, would you mind sticking the grouping variable in the template and seeing what comes out the other end?
Comment #32
jwilson3Interesting, in my test the {{ grouping }} variable is always empty, (and using {{ dump(grouping) }} causes warnings.
And what's more, the {{ grouping_level }} variable is always zero.
Should these be filed as separate follow-up issues, or be fixed here?
Comment #33
thedavidmeister CreditAttribution: thedavidmeister commentedFollowup for sure. These docs were pulled from tpl.php directly. If the variables are empty that's a problem but it doesn't have anything to do with the Twig template conversion.
@jwilson3 if you wouldn't mind creating a followup issue, postponing it and linking to it from here and the Twig conversion meta issue that would be great.
So we just need profiling and doc updates as per #28 and #14.
I'm unassigning this just because it's been a few days with no patch. Feel free to reassign @deanbowles if you're still working on this.
Comment #34
jwilson3Follow-up added: #1997866: Erroneous Views grouping and grouping_level template variables.
Though, I see no reason to mark it postponed...
After testing against the tpl.php, it's clear that the grouping field is *not* empty for PHPTemplate, but is empty for Twig template only. In PHPTemplate it spits out the following information:
So this seems like at least the empty
grouping
variable is a bug introduced in this conversion code above.Comment #35
thedavidmeister CreditAttribution: thedavidmeister commented#34 - mmk, well that's something we should fix here then.
Comment #36
Sean Charles CreditAttribution: Sean Charles commentedRe-rolled per #33
Comment #37
intergalactic overlords CreditAttribution: intergalactic overlords commentedtpl.php doesn't show any content in the view. I also get following notice: Notice: Array to string conversion in include() (line 23 of core/themes/bartik/templates/views-view-grouping.tpl.php).
The twig version shows the view as it should be.
{{ dump(grouping) }} :
{{ dump(grouping_level) }} :
int(0)
var_dump($grouping) :
var_dump($grouping_level) :
int(0)
Comment #38
intergalactic overlords CreditAttribution: intergalactic overlords commentedScreenshot of the source from tpl.php version
Comment #39
thedavidmeister CreditAttribution: thedavidmeister commented#37 - I'm not sure why this was set to "needs work"? What exactly are we doing here if the grouping field contains the same data in Twig and tpl.php?
Comment #40
joelpittet@intergalactic o... That is saying to me that the PHP template version is broken, not the twig version... so could you open a followup for that PHP template issue with VDC tags and since this one is printing out the expected output it should be fine.
Comment #41
thedavidmeister CreditAttribution: thedavidmeister commentedComment #43
star-szr#36: 1843752-27-twig-views-grouping.patch queued for re-testing.
Comment #45
joelpittetCouldn't find the reference in #14 @thedavidmeister. But did the other one #28
@Sean Charles sorry something happened with the patch, it didn't apply and testbot failed and there was no interdiff.
Comment #46
RobLoachLooks good!
Comment #47
geoffreyr CreditAttribution: geoffreyr commentedProfiling.
Comment #48
geoffreyr CreditAttribution: geoffreyr commentedhttp://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c03b86b1ec&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c03b86b1ec&...
Comment #49
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #49.0
alexpottadded manual testing steps
Comment #50
alexpottCommitted ed042be and pushed to 8.x. Thanks!
Comment #51.0
(not verified) CreditAttribution: commentedUpdated issue summary -- add git commit message.