Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Issue #2152215 by joelpittet, mr.baileys, benjifisher, martin107, rteijeiro, hussainweb, shanethehat, jenlampton, kpa, AnythonyR, EVIIILJ, kgoel, Cottser, dsdeiz, hanpersand: Convert theme_form_element_label() to Twig
Task
Convert theme_form_element_label() to a Twig template.
Remaining tasks
- Patch
- Patch review
- Manual testing
- Profiling
Steps to test
@todo
Comment | File | Size | Author |
---|---|---|---|
#50 | 2152215-50.patch | 4.2 KB | star-szr |
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 commentedFirst try...
Comment #4
mr.baileysTests where failing when both title and required where empty.
Comment #6
joelpittetThanks @mr.baileys and @rteijeiro for getting this one going.
I added
is not empty
so the label would accept 0's.Also removed the direct call to theme() and indented the if statement with whitespace modifiers to help the markup get closer.
Comment #7
rteijeiro CreditAttribution: rteijeiro commentedJust one silly question: What are do the hyphens
-%
and%-
do here?Comment #8
star-szrWhitespace control! http://twig.sensiolabs.org/doc/templates.html#whitespace-control
Comment #9
joelpittetThose are whitespace modifiers in twig. They remove the two spaces and ending newline to get the markup close to the original.
http://twig.sensiolabs.org/doc/templates.html#whitespace-control
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commentedWorks as expected.
Comment #11
star-szrThis one still needs profiling. Thanks @michamilz!
Comment #12
joelpittetI'm moving this to combine it to #2152213: Convert theme_form_element() to Twig because it's the only template that uses this template.
Comment #13
joelpittetOk un-closing this one, seems there is a quite a few uses in contrib and a new use-case is coming from #1938926: Convert simpletest theme tables to table #type
Comment #14
joelpittet6: 2152215-6-twig-form_element_label.patch queued for re-testing.
Comment #16
joelpittet6: 2152215-6-twig-form_element_label.patch queued for re-testing.
Comment #17
joelpittetRe-rolling.
Comment #18
joelpittetThis is the only thing i'm concerned about, the required marker doesn't use this at all... but since it is a "render element" the FAPI does some weird things to that property and kinda rebuilds it... Maybe we pass in some variables, or just convert that required_marker to 'variables' and make life easier?
Comment #19
joelpittetre: #18 here's an example
Comment #21
star-szrComment #22
joelpittetHiding 19 and #17 is the one to review.
Comment #23
star-szrTagging for reroll.
Comment #24
joelpittetRe-rolled.
Comment #25
martin107 CreditAttribution: martin107 commentedreroll.
Comment #26
zaphoyd CreditAttribution: zaphoyd commentedI've tested the patch in #25 for markup equivalence against the 8.x git branch at commit be94d4e017785ad0b63780c399287fcfc4eaeeff from Sun Mar 30 15:08:27 2014 +0200. Everything I looked at manually checked out. Twig debug was used to confirm that the twig theme was actually being used. Some details:
Title field (required)
Before:
After:
Body field (not required, can be collapsed/expanded with summary field)
Before (expanded summary):
After (expanded summary)
Before (collapsed summary):
After (collapsed summary):
Tags field (not required, nothing special)
Before:
After (+twig_debug):
Other
I also looked at labels for checkboxes (new revision), radio buttons (comment settings), inline un-editable fields (author), select lists (filter file status), and visually hidden labels (comments -> update options). All of these have the same markup as well.
Comment #27
joelpittetAwesome manual testing @zaphoyd, I'm removing that tag.
Comment #28
benjifisherI will profile the patch.
Comment #29
benjifisherThe patch in #25 needed a re-roll. The conflicting patch comes from #2089433: Remove uses of deprecated XSS filter functions. The patch in #25 already used the new
Xss::filterAdmin()
instead offilter_xss_admin()
, but it removed a line that the other patch was trying to update. So re-rolling the patch was pretty easy. I ran a few tests locally, and they passed. Let's see what the testbot thinks!Comment #30
benjifisherProfiling completed:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=534ad8dec7444&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=534ad8dec7444&...
Looking at the XHProf results,
__TwigTemplate_2fd348da3df0021653c75db706bfbc964d51daef0e20d41b9be683b7c4ee690a::doDisplay()
in the patched version, 3 calls totheme_form_element_label()
in the unpatched version: good!form-element-label.html.twig
. Something is wrong!Comment #31
benjifisherI forgot to un-assign the issue.
Comment #32
star-szrI suspect the discrepancy might be related to render caching because I think I'm running into that while trying to profile #2231505: Convert theme_field__node__title() to Twig.
Comment #33
star-szrRelevant (for disabling render caching): https://gist.github.com/Cottser/10997441
Comment #34
joelpittetRe-rolled because required marker is now a CSS style! #2152217: Remove theme_form_required_marker() from the theme system - use CSS instead
Comment #35
joelpittetAnother round of profiling.
Scenario:
Anonymous has full permissions
Turned off render caching re #33
Pointed the front page to node/1 and added a test article.
Added a second login block to the second sidebar
Had 9 labels on the page.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=536ffbe232db1&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=536ffbe232db1&...
Comment #37
joelpittetWhoops, wrong patch (crossed repo) for the re-roll. Let's try this one.
Comment #39
joelpittetThat test fails without this patch. Head is broken or something? Anyways this still applies and RTBC.
Comment #40
webchick37: 2152215-theme_form_element_label-34.patch queued for re-testing.
Comment #42
joelpittetRandom testbot fail... ApcuBackendUnitTest
Comment #43
xjm37: 2152215-theme_form_element_label-34.patch queued for re-testing.
Comment #45
joelpittetRe-rolled
Comment #46
joelpittetFor real...
Comment #48
martin107 CreditAttribution: martin107 commented46: 2152215-twig-form_element_label-46.patch queued for re-testing.
extract from ViewAjaxTest
error writing to disk -- looks like testbot failure -- retesting
Only 0 of 476 bytes written, possibly out of free disk spacefile_put_contents('public://.htaccess', '# Turn off all options we don't need. Options None Options +FollowSymLinks # Set the catch-all handler to prevent scripts from being executed. SetHandler Drupal_Security_Do_Not_Remove_See_SA_2006_006 # Override the handler again if we're run later in the evaluation list. SetHandler Drupal_Security_Do_Not_Remove_See_SA_2013_003 # If we know how to do it safely, disable the PHP engine entirely. php_flag engine off ') file_save_htaccess('public://', ) file_ensure_htaccess() install_base_system(Array) install_run_task(Array, Array) install_run_tasks(Array) install_drupal(Array) Drupal\simpletest\WebTestBase->setUp() Drupal\views\Tests\ViewTestBase->setUp() Drupal\views\Tests\ViewAjaxTest->setUp() Drupal\simpletest\TestBase->run() simpletest_script_run_one_test('886', 'Drupal\views\Tests\ViewAjaxTest')
Comment #49
joelpittetThanks @martin107 back to RTBC
Comment #50
star-szrUpdating the suggested commit message.
Since we're changing this hunk, @ingroup themeable doesn't belong here. Removing it, still RTBC.
Comment #51
alexpottCleaner if
$variables['title'] = empty($element['#title']) ? Xss::filterAdmin($element['#title']) : '';
Comment #52
bayousoft CreditAttribution: bayousoft commentedComment #53
benjifisher@bayousoft: Are you thinking what I am thinking? The suggestion in #51 looks backwards to me.
Comment #54
bayousoft CreditAttribution: bayousoft commented@benjifisher yup
Comment #55
bayousoft CreditAttribution: bayousoft commentedRerolled with suggested syntax reversed.
Comment #56
star-szr@alexpott's code looks right to me. It's checking to see if the $element array has a key of #title and is not an empty string, and THEN running an XSS filter on it. No point in XSS filtering an empty string and no guarantees that the #title key is there.
Comment #57
bayousoft CreditAttribution: bayousoft commentedRerolled with suggested syntax
Comment #58
bayousoft CreditAttribution: bayousoft commentedComment #61
bayousoft CreditAttribution: bayousoft commentedAny ideas as to why this patch failed so miserably?
Comment #62
bayousoft CreditAttribution: bayousoft commentedComment #63
benjifisher#51 says
and I think it should be
but the interdiff in #57 has
I disagree with #56. I do not know PHP well enough to say for sure how
empty(...)
evaluates if there is no'#title'
key, but I think it is the same asempty(NULL)
orTRUE
.I wish I could preview before posting ...
Comment #64
star-szrI think I read it as !empty, then it would make more sense :)
Comment #65
bayousoft CreditAttribution: bayousoft commentedComment #66
bayousoft CreditAttribution: bayousoft commentedSo is the process to keep making new branches every time I upload a patch?
Comment #67
bayousoft CreditAttribution: bayousoft commentedNew branches to create a useful interdiff.txt that is.
Comment #68
benjifisher@bayousoft: How strong is your git-fu? This is more or less what I would do if I were making multiple attempts at creating a simple patch:
(Just personal preference: I do not like all those identically-named interdiff.txt files, with auto-generated _1234 extensions.) Then, to start over and try again,
(assuming that I posted the first try as #68, and the testbot or someone else pointed out a problem in #69 so the next comment would be #70).
Comment #69
bayousoft CreditAttribution: bayousoft commentedthx @benjifisher
Comment #71
bayousoft CreditAttribution: bayousoft commentedrerolled
Comment #72
bayousoft CreditAttribution: bayousoft commentedComment #74
benjifisherYou could try running
Drupal\system\Tests\Form\ElementsLabelsTest
locally; that is the one that is failing. Maybe even look at the code, add some extradebug()
lines to figure out what is wrong. Possibly the suggestion in #51 is wrong; note my disclaimer in #63.It is also possible that something has gone wrong in HEAD. You could try
and then run the test without applying the patch ...
Comment #75
bayousoft CreditAttribution: bayousoft commentedThat path does not exist in my codebase. I'm wondering where to find that test.
Comment #76
joelpittetEnable the simpletest module (called "Testing" on the extend/modules page)
Then you can go to the ui to run the test:
/admin/config/development/testing
Comment #77
joelpittetOh I should have stepped in early, sorry. Please don't use
!empty
because we are testing to make sure we can allow 0 as a field label. And!empty(0) == FALSE
vs(isset(0) && 0 != '') == TRUE
@alexpott #50 is RTBC, unless you guys change anything else?
That was a lot of comments, hopefully you all got something out of that? eek...
Comment #78
star-szrMight be worth a code comment then.
Comment #79
benjifisher@joelpittet: I think what you meant to say was
Note the use of
!==
(as in the original version) rather than!=
.Comment #80
bayousoft CreditAttribution: bayousoft commentedComment #81
bayousoft CreditAttribution: bayousoft commented@joelpittet Okay. Well at least I got to practice the process. On to the next issue I go.
Comment #82
joelpittet@bayousoft you could add a comment above that line to help clarify as @Cottser mentioned?
because it did confuse everybody and the only reason I know is because I did that change too in earlier patches. Thank god we already have test coverage here.
@benjifisher thanks yeah, I wrote that hastily to get the point across, totally needs !== as it was before.
Comment #83
alexpottHow could I forget :)
Committed #50 b962ed4 and pushed to 8.x. Thanks!
Comment #85
joelpittetYeah because that makes sense!
PHP--
Thanks for committing, another one bites the dust!
Comment #86
benjifisherThe example in #83 does not work for me. It looks as though
empty()
changed in PHP 5.5. For earlier versions, useNote that I switched single and double quotes, so that bash would not interpret
$foo
as an environment variable. If you are using a different shell, then YMMV. Alternatively,php -r "\$foo = '0'; var_dump(empty(\$foo));"
Comment #87
joelpittet@benjifisher that's bizarre, the PHP docs only say they added expressions instead of just variable arguments in 5.5 to empty. And #83 and your two examples in #86 all produce
Which is the expected unexpected result of PHP. Do you see
bool(false)
on your machine?Comment #88
benjifisher@joelpittet: No, I see a parse error:
This is what I expect, since I am using PHP 5.4.8. According to http://us1.php.net/empty,
Since
'0'
is an expression, not a variable,empty('0')
does not parse.Comment #89
joelpittetAh I see what you mean now, thanks for clarifying. I thought you were using 5.5 too... The light has turned on;)
Comment #91
alimac CreditAttribution: alimac commentedMinor tag cleanup - please ignore.