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 #1963764 by widukind, pwieck, thedavidmeister: Convert theme_views_view_mapping_test() to Twig.
Meta issue: #1843738: [meta] Convert views module to Twig
Comment | File | Size | Author |
---|---|---|---|
#42 | views-convert-theme_views_view_mapping_test-twig-1963764-42.patch | 1.73 KB | pwieck |
#42 | 1963764-35-42.patch-interdiff.txt | 467 bytes | pwieck |
#36 | 1963764-35.patch | 1.76 KB | pwieck |
#27 | 1963764-27.patch | 1.76 KB | pwieck |
#23 | 1963764-23.patch | 1.73 KB | widukind |
Comments
Comment #1
star-szrTagging.
Comment #2
star-szrActually, this one probably doesn't need manual testing because this is only used in automated tests.
Comment #3
star-szrThis already has a preprocess function:
http://api.drupal.org/api/drupal/core%21modules%21views%21tests%21views_...
The theme function is a one-liner:
http://api.drupal.org/api/drupal/core%21modules%21views%21tests%21views_...
In theory we just need to create a dead simple .html.twig template and remove the theme function, the only thing is I can't see where the theme function is defined. I found this, but not sure if we need to change it to pick up a template file - not familiar with the plugin annotation yet.
Comment #4
star-szrOkay, @tim.plunkett was able to clear this up for me and explain how this works. Thanks Tim!
So the theme implementation is defined in the style plugin annotation (code in comment #3).
Then views_theme() adds hook_theme() definitions for all style plugins:
The last part checks for a template file based on the theme definition name (if there is no theme function), in this case views-view-mapping-test.html.twig.
So this should be a very straightforward conversion. Remove the theme function and add a simple template - done :)
Comment #5
widukind CreditAttribution: widukind commentedComment #6
widukind CreditAttribution: widukind commentedPatch attached.
The variable description in the code docs for the template_preprocess function and the twig template are in part guesswork, any suggestions for more accurate descriptions are much appreciated.
Thanks!
Comment #7
widukind CreditAttribution: widukind commentedsloppiness in the previous patch, missed to close the docblock properly in the template.
fixed in this updated patch.
Comment #8
star-szrThis is looking very good. Thank you, @widukind! :)
@see template_preprocess()
Comment #9
widukind CreditAttribution: widukind commentedThanks @Cottser, fixed the code comment.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedOne nitpick, should be RTBC after this.
+ * - element: The view content.
Capital V for View.
Comment #11
widukind CreditAttribution: widukind commentedcapitalized View in code docs.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedThe docs look good to me. I'm assigning to myself as I'm going to apply the patch locally and test it out.
Comment #13
thedavidmeister CreditAttribution: thedavidmeister commented@Cottser, thanks for the explanation in #3 and #4 as to how this even works, I was thoroughly confused for a bit >.<
For anyone who is wondering *which* tests this is relevant to (since there's quite a lot of Views tests), look in /core/modules/views/lib/Drupal/views/Tests/Plugin/StyleMappingTest.php
They're under "Views Plugins" in SimpleTest.
The template renders what ends up in the $output variable twice in the testMappedOutput() method.
Original markup - output 1:
Original markup - output 2:
New markup - output 1:
New markup - output 2:
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commentedComment #15
dawehnerThis line somehow differs from other patches, but I think we can clean this up as follow ups.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commented#15 - as in, it should say "The View object"?
Comment #17
widukind CreditAttribution: widukind commented"The View object" it is.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedComment #19
Fabianx CreditAttribution: Fabianx commented#17: 1963764-17.patch queued for re-testing.
Comment #21
star-szrTagging for reroll.
Comment #22
widukind CreditAttribution: widukind commentedComment #23
widukind CreditAttribution: widukind commentedrerolled patch from 1963764-17
Comment #24
star-szrThanks @widukind!
Comment #25
joelpittetBack to RTBC from #18
Comment #26
alexpottThe views test modules have moved :)
Comment #27
pwieck CreditAttribution: pwieck commentedrerolled.
Comment #29
pwieck CreditAttribution: pwieck commentedGot to kick #27 back to the coders. I'm just a reroll monkey. Seems like an odd place to fail.
One-time link is no longer valid. Other UserPasswordResetTest.php 81
Drupal\user\Tests\UserPasswordResetTest->testUserPasswordReset()
Comment #30
pwieck CreditAttribution: pwieck commented#27: 1963764-27.patch queued for re-testing.
Comment #31
pwieck CreditAttribution: pwieck commentedReady for review! The test bots have been flaky today .
Comment #32
pwieck CreditAttribution: pwieck commentedRemoving tag -Needs Reroll
Comment #33
dawehnerSeems fine, back to RTBC.
Comment #34
star-szrWe could lowercase 'V' in 'View' and 'Views' throughout the patch though :)
Comment #35
pwieck CreditAttribution: pwieck commentedFixing patch to reflex #34
Comment #36
pwieck CreditAttribution: pwieck commentedFixed docs with capital V's to lowercase in View and Views.
Comment #37
pwieck CreditAttribution: pwieck commentedForgot status change
Comment #38
star-szrChanges look good, thanks @pwieck. I will come back to this tonight and re-RTBC unless someone beats me to it.
Comment #39
star-szrOkay, I'm happy :)
Comment #40
star-szrLet's remove this line per #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.
Comment #41
pwieck CreditAttribution: pwieck commentedFixing this
Comment #42
pwieck CreditAttribution: pwieck commentedMade new policy change as per #40
Comment #43
pwieck CreditAttribution: pwieck commented#42 passed and ready for review
Comment #44
dawehnerBack to RTBC
Comment #45
alexpottCommitted e294988 and pushed to 8.x. Thanks!
Comment #46.0
(not verified) CreditAttribution: commentedAdd commit message to summary