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 #2152219 by joelpittet, mdrummond, rodrigoaguilera, sqndr, rteijeiro, InternetDevels, steveoliver, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_input() to Twig
Task
Convert theme_input() to a Twig template.
Remaining tasks
PatchPatch reviewManual testing- Profiling
How to test this code
- Create a content type with
<input>
fields - View the Add Node form for this content type
- Check the source
- Apply the patch
- Reload the Add node form
- Check the source
- Compare the before and after sources
Comment | File | Size | Author |
---|---|---|---|
#65 | 2152219-twig-theme_input-65.patch | 2.2 KB | joelpittet |
#65 | interdiff.txt | 654 bytes | joelpittet |
Comments
Comment #1
star-szrAdding a commit message to the issue summary so the folks who already worked on #1898480: [meta] form.inc - Convert theme_ functions to Twig and in the Twig sandbox get credit.
Comment #2
rteijeiro CreditAttribution: rteijeiro commentedLet see... Not sure about it but it's a try :)
Comment #3
sheilaj CreditAttribution: sheilaj commentedThere a space between input and attributes that should be removed.
Comment #4
sqndr CreditAttribution: sqndr commentedI remove the space as suggested in comment #3.
Comment #6
star-szrThanks everyone!
I tried to apply the patch from #4 locally and got this:
The patch from #2 still applies for me with an offset.
Comment #7
star-szrLooks like it might just be missing a newline (blank line) at the end of the file. That would be a good Novice task for someone starting out in the issue queues!
Also, 'Needs review' is a status, not a tag. Tagging needs review is redundant. I know the many dropdowns and fields are confusing :)
Comment #8
InternetDevels CreditAttribution: InternetDevels commentedPatch attached.
Comment #10
sqndr CreditAttribution: sqndr commented4: 2152219-convert-theme-input-4.patch queued for re-testing.
Comment #12
sqndr CreditAttribution: sqndr commented2: 2152219-convert-theme-input.patch queued for re-testing.
Comment #14
InternetDevels CreditAttribution: InternetDevels commented8: drupal_core-convert-theme-input-2152219-8.patch queued for re-testing.
Comment #15
sqndr CreditAttribution: sqndr commented4: 2152219-convert-theme-input-4.patch queued for re-testing.
Comment #17
sqndr CreditAttribution: sqndr commented8: drupal_core-convert-theme-input-2152219-8.patch queued for re-testing.
Comment #18
sqndr CreditAttribution: sqndr commentedThis last patch fixes the new line problem and removes the unnecessary space.
Comment #19
star-szrComment #20
joelpittetMissing the
* Default template: input.html.twig.
#children key is defined already in drupal_render(), don't do the empty() check which likely breaks printing 0s
I don't think this @todo is relevant.
Comment #21
RainbowArrayHere's a patch and an interdiff with the changes joelpittet suggested.
Comment #22
RainbowArrayComment #23
joelpittetPerfecto, thanks @mdrummond. Adding tags back on this one.
Comment #24
star-szrThanks @mdrummond! This could use some small tweaks.
This should be "Prepares" instead of "Preprocesses" per https://drupal.org/node/1354#themepreprocess.
We need to remove the line above @ingroup themeable as well so there isn't a blank line at the bottom of the docblock.
Until we figure out #2108771: Remove special cased title_attributes and content_attributes for Attribute creation there's no need to instantiate the attribute object here.
The attributes aren't for a wrapper element, they're for the input element.
Comment #25
RainbowArrayHere's a patch and interdiff with the changes Cottser suggested.
Comment #26
RainbowArrayThis patch is green. Manual testing and profiling up next?
Comment #27
joelpittetManually testing, and I'm surprised that removing the new Attribute() actually worked!! FAPI's render element's got some magic I am not aware of... magic-- $variables = $element somwhere...
Comment #28
RainbowArraySo what's next. Profiling?
Comment #29
joelpittetManual Testing first, then profiling. Just need markup comparison screenshots or diff files. I usually turn twig debugging on so I can see that it's rendering the template first and second so I can remove the markup before and after on both with and without the patch to compare the two a bit easier.
Comment #30
GoldI was looking at this this morning and spun up a simpletest.me instance. Input fields are working fine but I suspect there's more to testing this than just patching, hitting a page and viewing source.
What sort of report/testing should be done to verify issues like this? Is there a page outlining how to test and report the results of Twigifying things?
Comment #31
joelpittet@Gold, hi! For a second I didn't think we had anything formal:P http://drupalladder.org/lesson/05820dfa-a09d-9014-511f-5779d9915adc
and https://drupal.org/node/2048869
Comment #32
Gold@joelpittet, cheers. Spinning up a clean copy now. Will create a content type with all input fields and grab the source, apply the patch and repeat.
Have updated the issue with details from the link you provided also.
Comment #33
GoldThis has not worked as expected unfortunately.
The source of a page with as many different input fields as are available in a standard profile install, both before and after the patch here: http://dropbucket.org/node/1140
There is also a diff there.
Looking at the screencaps though it appears to be causing issues with;
Comment #34
longwaveDid you clear cache after applying the patch? The only way I can reproduce #33 is by applying the patch and then not clearing cache before testing - if I browse to /admin/config/people/accounts without clearing cache, I also see missing textfields, radio buttons, etc., but then if I clear the cache then all the input fields appear as expected.
edit: realised you will need to use Drush or the rebuild.php script to clear cache, as the button on the performance page disappears when the patch is applied!
Comment #35
Gold@longwave, I will retest.
I'm newish to the d8 testing process and didn't know about the rebuild.php script. Here at work I'm also tied to the current stable drush (which doesn't operate with D8) so couldn't cc all. Will try and fix that today too.
Comment #36
joelpittet@Gold feel free to ask any questions in #drupal-twig on IRC if you'd prefer. There is usually a couple of us hanging out there for any troubles you have. Thanks again for looking at this issue. Drupal 8's version of drush removed the '
drush cc all'
in favour of 'drush cr
'.A nice way to use drush is have it on your computer from github and switch branches if you need an older version but then you can keep it up to date too with a
git pull
.Comment #37
Gold@longwave, you rock. :)
After using the rebuild.php script and checking again all the fields look as they did before patching.
I've not bothered with a diff this time. Looking at it, the only items that fell out were the dates in the value attributes for the date & time fields. The reset were form_build_id and url tokens in the css/js in the head.
This is looking good for me now.
Comment #38
GoldFailed to update the status with #37. Or does this need more testing before this status is reached?
Comment #39
RainbowArrayWe need profiling before we can RTBC this, at least as far as I understand the process.
Comment #40
Gold@mdrummond Cheers. Knocked the Needs manual testing tag off at least. :)
Comment #41
RainbowArray@Gold Indeed! Great work! One step at a time.
Here's the info on profiling: https://drupal.org/contributor-tasks/profiling
The basic idea is to test the performance for before the patch is applied and after the patch is applied. Ideally performance is pretty close after the patch.
I found it challenging to get my computer set up to do profiling, but I did get it working eventually. Making sure my laptop was plugged in, and that I had all programs except for Terminal closed, after a reboot, was helpful. What took me a while was getting consistent performance over multiple tests for the before patch scenario. That has to be nearly identical in order to have a valid benchmark to measure the change in performance due to the Twig conversion. If you want to try profiling, hope those tips help!
Comment #42
joelpittetComment #43
joelpittetI'm surprised this worked, unless there is some magic I don't know about this should be:
$variables['attributes'] = $element['#attributes'];
Don't need the instantiation but still need to variable pass along from $element to $variables.
Comment #44
GoldYeah... Between my projects at home and the development environment here at work I doubt I have a system that is stable/consistant enough for profiling.
Hmm... Another project comes to mind. very cut down RaspberryPi running just the barebones for D8 profiling... That could work. :)
Comment #45
joelpittetThat would be a nice tool for profiling, I bet the results would be more accurate than my laptop. We should fix or figure out #43 before we start profiling though.
Comment #46
star-szrTagging for reroll.
Comment #47
rodrigoaguileraComment #48
rodrigoaguilerareroll
I'm not sure how to make the interdiff
Comment #50
rodrigoaguileraComment #51
rodrigoaguileraComment #52
joelpittet@rodrigoaguilera you don't need to do an interdiff for a re-roll though, just the changes you make that weren't part of the original.
Full instructions on how to do an interdiff: https://drupal.org/documentation/git/interdiff
The gist of it is a diff between commits, and the shortcut in git is:
git show > interdiff.txt
The slightly longer one would be
git diff HEAD~1..HEAD > interdiff.txt
And if you would like a workflow to understand try mine:) http://pittet.ca/drupal/sprint/patch
Comment #53
willieseabrook CreditAttribution: willieseabrook commentedComment #54
haithem_pro CreditAttribution: haithem_pro commentedComment #55
haithem_pro CreditAttribution: haithem_pro commentedPatch Needs profiling , I'm not so good at that !!
Comment #56
haithem_pro CreditAttribution: haithem_pro commentedComment #57
star-szrThe docblock for the preprocess function has regressed a bit. Compare to the patch in #25.
Should be:
Comment #58
joelpittetI'm just curious what ends up in children, the docs are vague so maybe the tests will give hints.
Comment #59
joelpittetHere's the fixes mentioned in #57 + a couple more.
Comment #60
star-szrThe children are "optional" here is the shortest explanation I think.
Comment #61
joelpittetSo though I'd like to just remove that, if they're is a template that needs {{ children }} they could just pass them through the preprocess or write them directly in the template.
But I don't want to break anything and this is just a conversion so maybe we can just RTBC #59 or add a better comment then RTBC;)
Comment #62
star-szrHere's an example of why children is needed from D7 contrib theme function land: #1980736: theme_link_field() only renders elements defined by this module
Comment #63
joelpittetThanks for the reference, I still think if need be contrib could add in that to preprocess and a template but that's for another issue.
Can we just change this to: "Optional additional rendered elements."?
Comment #64
star-szrWorks for me.
Comment #65
joelpittetCool, that's changed. Profiling next, stop me if we don't need it for this?
Comment #66
steinmb CreditAttribution: steinmb commentedTime for profiling and a bit monkey work.
Comment #67
steinmb CreditAttribution: steinmb commentedProfiling data
Run 538ce4363dd76 uploaded successfully for drupal-perf-steinmb.
Run 538ce46e85e63 uploaded successfully for drupal-perf-steinmb.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538ce4363dd76&...
Run 538ce4363dd76 uploaded successfully for drupal-perf-steinmb.
Run 538ce49a96f74 uploaded successfully for drupal-perf-steinmb.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538ce4363dd76&...
Comment #68
steinmb CreditAttribution: steinmb commentedMarkup compared and diffed
Comment #69
steinmb CreditAttribution: steinmb commentedComment #70
star-szrPatch looks good to me, updating suggested commit message.
Comment #71
alexpottCommitted 406da31 and pushed to 8.x. Thanks!