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.
Meta issue: #1843738: [meta] Convert views module to Twig
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Manual testing steps:
1. Add two articles
2. Edit front page view, set pager settings to unconditionally show the "more" link and show 1 article per page
3. Go to the front page
Comment | File | Size | Author |
---|---|---|---|
#58 | 1843742-58-twig-views-more.patch | 1.7 KB | joelpittet |
#58 | interdiff.txt | 994 bytes | joelpittet |
#49 | twig-views-more-1843742-49.patch | 1.73 KB | 2ndmile |
#49 | interdiff.txt | 574 bytes | 2ndmile |
#47 | twig-views-more-1843742-47.patch | 1.73 KB | 2ndmile |
Comments
Comment #1
joelpittetSeparated from meta patch, first draft
Comment #2
mbrett5062 CreditAttribution: mbrett5062 commentedTagging.
Comment #3
damiankloip CreditAttribution: damiankloip commentedOh,... all of these issues do exist :)
Comment #4
dawehnerIf we do that, we should remove the original theme function in the same issue, or?
Comment #5
joelpittetTrying to keep phptemplate engine running at the same time so try to keep the theme() working for now.
Comment #6
dawehnerThank you for that information!
Oh, twig doesn't hook into theme() yet (which afaik template engines should be already possible), never would have expected that.
From the actual template file this one looks perfect, though i have no idea of twig best practices etc.
Comment #7
joelpittetThe only ones I think need some love are the views templates I marked as Needs Work because I haven't gotten around to writing the preprocessor to provide the right variables to the template. But seeing as views is already using tpl.php files instead of theme() functions this should not be necessary just needs testing:)
Comment #8
FluxSauce CreditAttribution: FluxSauce commentedCommitted, thanks!
Comment #10
joelpittetMoving to core queue
Comment #11
chrisjlee CreditAttribution: chrisjlee commentedgiving it a first try
Comment #12
chrisjlee CreditAttribution: chrisjlee commentedFix some twig standard issues
Comment #14
joelpittet@chrisjlee the interdiff changes look good but I think the patch got messed, I have done that before too but I can't remember what I did.
Comment #15
damiankloip CreditAttribution: damiankloip commentedIt's probably because the files are added to the index but you didn't diff the cached index changes in git. So you need to use the --cached option with the diff, or use a branch to diff between that and 8.x. The failure is a random one too :)
Comment #16
chrisjlee CreditAttribution: chrisjlee commentedOr because it's an empty patch. Doh!
I reattached, hopefully, a non-empty patch. Reuploading #12
Comment #17
damiankloip CreditAttribution: damiankloip commentedThat is what I was saying, the reasons you would have an empty patch.
Comment #18
dawehnerIt's a views more link ... we should certainly name that in the @file.
Comment #19
joelpittet@chrisjlee Nice work! It's green:)
I agree with @dawehner to add "views more link" or is it "views' more link" ? English v Technology!
Comment #20
chrisjlee CreditAttribution: chrisjlee commentedThanks for the feedback and for the positive encouragement (@joelpettit).
Also capitalized views (Views) as per (http://drupal.org/files/1843746-views-view-field-9.patch) does it there.
Comment #21
chrisjlee CreditAttribution: chrisjlee commentedComment #22
dawehnerPerfect!
Comment #23
joelpittetComment #24
star-szr(Reviewing the second patch, http://drupal.org/files/1843742-convert-views-more-twig-20_0.patch)
Please replace "the url" with "The URL", and "the text" with "The text", per http://drupal.org/node/1354#drupal.
Great work @chrisjlee and @joelpittet!
Comment #25
star-szrCan go back to RTBC after #24 is fixed :)
Comment #26
chrisjlee CreditAttribution: chrisjlee commentedThanks cottser i will fix this
Comment #27
star-szrIt was brought up in #1843740-21: Convert views/templates/views-exposed-form.tpl.php to twig that the template should be changed from views-more.html.twig to views-view-more.html.twig.
@chrisjlee - can you take care of that too please? Just rename the file and update views_theme() I think.
There's also this CSS class in Drupal\views\Plugin\views\field\FieldPluginBase::render_text() that could be replaced like in #1843740-33: Convert views/templates/views-exposed-form.tpl.php to twig:
Thanks!
Comment #28
chrisjlee CreditAttribution: chrisjlee commentedYes sure thing.
Comment #29
chrisjlee CreditAttribution: chrisjlee commentedAttached are for changes 21-26 Did the docs changes.
Comment #30
chrisjlee CreditAttribution: chrisjlee commentedSeperated out the patches.
Changes as requested on #27
And not sure if it really matters should we edit the issue summary title?
Comment #31
steveoliver CreditAttribution: steveoliver commentedIt doesn't seem like this template is even being used?
Comment #32
star-szrI just grepped and found another 'views_more' string in Drupal\views\Plugin\views\display\DisplayPluginBase::renderMoreLink(), and I think that's the answer to @steveoliver's question as well.
So one more string to be changed. Thanks for your work on this @chrisjlee!
Comment #33
chrisjlee CreditAttribution: chrisjlee commentedThanks Cottser for your help!
@steveoliver out of curiousity, how did you know the template wasn't running and providing a false positive?
Comment #34
dawehnerThat's not the place where the theme function is called. The place is in DisplayPluginBase::renderMoreLink(). This here is another kind of more link.
Comment #35
dawehnerComment #36
star-szr@dawehner - So that class altered in FieldPluginBase should be left alone then?
Comment #37
webchickIs there a reason Views needs its own special template for this and can't just re-use http://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/th... ?
Comment #38
damiankloip CreditAttribution: damiankloip commentedtheme_more_link doesn't have configurable text, just a title. Although massive this could be added to that?
Comment #39
star-szrI think we should absolutely do that consolidation (#37/#38), not yet though. Convert first.
Comment #40
star-szrTagging.
Comment #41
joelpittetThis should probably just be a straight conversion, not renaming the files and such.
Try this on for size...
Comment #42
star-szrI agree with #41, let's do a follow-up if we want to rename this, but I think it makes sense as is - @webthingee put it well in #1843740-40: Convert views/templates/views-exposed-form.tpl.php to twig.
Comment #43
dawehnerBeside from the "needs manual testing" tag, this seems fine.
Comment #44
chrisjlee CreditAttribution: chrisjlee commentedComment #45
thedavidmeister CreditAttribution: thedavidmeister commentedreview for #41:
This looks very close to being ready for manual testing (which we need instructions on how to do).
+ * Default theme implementation for a Views more link.
Is it a
Views more link.
or aViews "more" link.
?Also, there's no template_preprocess_views_more() function but we still want a
@see template_preprocess()
and an@ingroup themeable
in the template documentation.As a followup issue we might want to look at the case when more_url is NULL (since that's the default value) - this isn't handled in the original tpl.php so I don't think we should touch it here. It makes sense to me that the whole template should not render anything unless more_url is not empty.
Comment #46
star-szrThanks @thedavidmeister, tagging the tweaks in #45 as a novice task.
If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!
Comment #47
2ndmile CreditAttribution: 2ndmile commentedImplemented changes from #45 to patch from #41. Please Review.
Comment #48
thedavidmeister CreditAttribution: thedavidmeister commentedGreat, thanks!
Missed a couple of "Views more link" references but this is looking pretty good from a docs perspective now.
This issue needs steps to manually test added to the issue summary.
Comment #49
2ndmile CreditAttribution: 2ndmile commentedImplemented #48. Please review.
Comment #50
thedavidmeister CreditAttribution: thedavidmeister commented#49 looks good to me. Cottser was saying in IRC earlier that
+ * @ingroup views_templates
lines might be unwanted later, but for now I'm happy.As I said in #48 this now needs manual test steps written in the summary and for somebody to actually do the markup testing.
Comment #51
star-szr@dawehner/@damiankloip - should we be keeping
@ingroup views_templates
? I think the Views and Views UI templates should definitely have@ingroup themeable
but not sure if we're keeping the other group.Comment #52
dawehnerThe functionality itself is broken, so we have to fix that: #1970136: Read more link isn't rendered
Comment #53
thedavidmeister CreditAttribution: thedavidmeister commented#52 - so you're saying this patch needs work, or that we can't manually test it until that linked issue is resolved?
Comment #54
dawehnerYeah the second point.
Comment #55
thedavidmeister CreditAttribution: thedavidmeister commentedsure.
Comment #56
star-szrWe need to get this in, whether the functionality behind it is broken or not. Let's remove @ingroup views_templates and lowercase 'View' and 'Views' please :)
If necessary we can do something like manually call the theme function in page.tpl to ensure the markup matches. theme('views_more', …) or create a render array and drupal_render() it.
Comment #57
joelpittetComment #58
joelpittetFixes from #56 - lowercase v and no @group views_templates
Comment #59
thedavidmeister CreditAttribution: thedavidmeister commentedSeems fair. I'll do some manual testing and hopefully RTBC this.
Comment #60
thedavidmeister CreditAttribution: thedavidmeister commentedalrighty then.
Steps to test:
1. Add two articles
2. Edit front page view, set pager settings to unconditionally show the "more" link and show 1 article per page
3. Go to the front page
HTML with patch in #58 applied:
Note that I set the 'more' text through the views UI to be "asdfasdf" and also left it blank and the output was not modified. Editing the template to replace {{ link_text }} does make something appear.
This is pretty broken.
Give me a minute to check the functionality pre-patch...
Comment #60.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #61
thedavidmeister CreditAttribution: thedavidmeister commentedHTML from pre-patch with both empty "more" settings and "asdfasdf" for the more text:
so, the problem isn't being introduced by Twig. RTBC then? ;)
Comment #62
thedavidmeister CreditAttribution: thedavidmeister commentedFWIW I don't think this would help with the problem here, apparently the problem is actually the view calling the theme function and sending the wrong variables.
Comment #63
thedavidmeister CreditAttribution: thedavidmeister commentedComment #64
star-szrI think as long as it's broken in the same way. @dawehner?
Edit: read too quickly, I will do a page.tpl test as proposed.
Comment #65
thedavidmeister CreditAttribution: thedavidmeister commented#64 - I'm totally happy to have this RTBC since the breakage has zero to do with the Twiggy-ness of the template.
Comment #66
star-szrThe template conversion itself is fine and doesn't need to be blocked on #1970136: Read more link isn't rendered, I agree with the RTBC.
Code added to page.tpl.php:
Before:
After:
Comment #67
star-szrTagging for profiling.
Comment #68
mr.baileysAssigning to me for profiling.
Comment #69
mr.baileys#1970136: Read more link isn't rendered was fixed, so the read more link is working again.
Results from profiling a front page with full node listing, 1 per page and a read more link:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0a969ad5d&...
Comment #70
mr.baileysComment #71
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #72
alexpottCommitted 9414794 and pushed to 8.x. Thanks!
Comment #73.0
(not verified) CreditAttribution: commentedadded manual testing steps