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
To Test: Both the admin/content and admin/people view are now views forms. Try using them (unpublish nodes, block users).
Comment | File | Size | Author |
---|---|---|---|
#38 | 1912600-38-twig-views-form-views-form.patch | 3.81 KB | joelpittet |
#38 | interdiff.txt | 897 bytes | joelpittet |
#30 | 1912600-30-twig-views-form-views-form.patch | 3.81 KB | joelpittet |
#21 | views.view_.actions.yml_.txt | 4.51 KB | dawehner |
#13 | twig-views-form-views-form-1912600-13.patch | 3.85 KB | joelpittet |
Comments
Comment #1
bzitzow CreditAttribution: bzitzow commentedComment #2
bzitzow CreditAttribution: bzitzow commentedComment #3
steveoliver CreditAttribution: steveoliver commentedLooks good. Only two small nitpicks:
Remove this line.
No newline here, just ) { . See Drupal coding standards.
Comment #4
bzitzow CreditAttribution: bzitzow commentedApplied changes from above feedback ...
Comment #5
joelpittetComment #7
joelpittetI have a sneaky suspicion that this doesn't need to be in the theme layer at all.
nitpik: should be underscores.
Though I don't think the fails are your fault at all: tpl.php is getting appended to the end.
views/templates/views-form-views-form.html.twig.tpl.php
Comment #8
tim.plunketttheme_views_form_views_form() could possibly be moved to a #pre_render callback
Comment #9
bzitzow CreditAttribution: bzitzow commentedThe tests are all failing because it is searching for:
and the file in the patch is named:
Which is intended to be correct under the new architecture? I do not know ...
In #drupal-twig:
I admit I am not very familiar with drupal. This twig conversion just renders the output from the drupal core function related to form rendering:
drupal_render_children()
So the question in my mind is - what is the relationship between twig and the drupal form api?
Comment #10
joelpittetThis should have the changes to make it fly, but I am going to do another one for #pre_render callback as suggested by tim in #8
Comment #11
joelpittetSo this probably will pass, but I don't think it's right... I couldn't find a test for substitutions. but this is an attempt at #8
Comment #13
joelpittetOk using real variables this time:)
Comment #14
star-szrTagging.
Comment #15
joelpittetComment #16
dawehnerYes, the theme function is not needed, but i'm wondering whether people actually want to have a theme function, so they can adapt if needed, which is much harder if you just have a form + preprocess function.
Comment #17
thedavidmeister CreditAttribution: thedavidmeister commenteddoing some manual testing.
Comment #18
thedavidmeister CreditAttribution: thedavidmeister commentedActually, I'm not 100% sure what the manual testing steps are here. Could somebody with a bit more experience with this issue help me out here?
Is this for a "front end" form like an exposed filter?
Comment #19
clemens.tolboomI agree with @thedavidmeister as it is not clear were to test this visually.
@dawehner in #16: I guess we should make it as easy as possible for themers. But then again where is this patch visible.
Comment #20
thedavidmeister CreditAttribution: thedavidmeister commentedCan't be reviewed until we know where the template is.
Comment #21
dawehnerEnable the actions module and import the following yml file into your drupal installation. This will provide you a view with a bulk operations form.
Comment #22
thedavidmeister CreditAttribution: thedavidmeister commentedgreat! thanks @dawehner.
Comment #23
StryKaizerImported view from dawehner #21 and did daisydiff on clean/patched. Manual testing did not gave any issues.
Comment #24
joelpittetGreat, on to profiling!
Comment #25
adamcowboy CreditAttribution: adamcowboy commentedI have no idea what to test for. I opened a form and everything was normal. Can someone post a more detailed description of the modification?
Comment #26
tim.plunkettBoth the admin/content and admin/people view are now views forms. Try using them (unpublish nodes, block users).
Comment #27
adamcowboy CreditAttribution: adamcowboy commentedI tested by adding and changing published content to unpublished content. I did a similar thing for an admin. It works well. Good work.
Comment #27.0
adamcowboy CreditAttribution: adamcowboy commentedUpdated issue summary.
Comment #28
star-szrThanks for testing @adamcowboy, this still needs profiling before we can RTBC it though.
Comment #29
star-szrThis needs a reroll before it can be profiled.
Comment #30
joelpittetRerolled and retitled as this no longer is a Twig conversion.
Comment #32
star-szr#30: 1912600-30-twig-views-form-views-form.patch queued for re-testing.
Comment #34
joelpittet#30: 1912600-30-twig-views-form-views-form.patch queued for re-testing.
Comment #35
dawehnerI guess we also need some manual testing?
Comment #36
joelpittet@dawehner there was some manual testing but it wouldn't hurt to do another round. Is there any chance you know of a good way to test this? Most of what it does is for "substitutions" which I am not really sure what they are...
Comment #37
dawehnerYou are totally right! Here is a final nitpick.
This comment line is > 80 chars.
Comment #38
joelpittetOh the shame! :P
Does this need profiling? Anybody know of a view with lots of 'substitutions'? or any for that matter...
Comment #39
dawehnerThe same code is executed and just moved ... so I don't see really a need for profiling.
Comment #40
thedavidmeister CreditAttribution: thedavidmeister commentedComment #41
webchickAwesome clean up!
Committed and pushed to 8.x. Thanks!
Comment #42.0
(not verified) CreditAttribution: commentedAdded "To Test" section