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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

star-szr’s picture

Issue summary: View changes

Adding 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.

rteijeiro’s picture

Status: Active » Needs review
FileSize
4.13 KB

First try...

Status: Needs review » Needs work

The last submitted patch, 2: 2152215-convert-theme-form-element-label-2.patch, failed testing.

mr.baileys’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
3.97 KB

Tests where failing when both title and required where empty.

Status: Needs review » Needs work

The last submitted patch, 4: 2152215-4-form-element-label-template.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.06 KB
1.55 KB

Thanks @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.

rteijeiro’s picture

+++ b/core/modules/system/templates/form-element-label.html.twig
@@ -0,0 +1,19 @@
+{% if title is not empty or required -%}
+  <label{{ attributes }}>{{ title }}{{ required }}</label>
+{%- endif %}

Just one silly question: What are do the hyphens -% and %- do here?

star-szr’s picture

joelpittet’s picture

Those 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

Anonymous’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +#SprintWeekend2014

Works as expected.

star-szr’s picture

Status: Reviewed & tested by the community » Needs work

This one still needs profiling. Thanks @michamilz!

joelpittet’s picture

Status: Needs work » Closed (won't fix)
Related issues: +#2152213: Convert theme_form_element() to Twig

I'm moving this to combine it to #2152213: Convert theme_form_element() to Twig because it's the only template that uses this template.

joelpittet’s picture

Status: Closed (won't fix) » Needs review

Ok 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

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 6: 2152215-6-twig-form_element_label.patch, failed testing.

joelpittet’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

Re-rolling.

joelpittet’s picture

+++ b/core/includes/form.inc
@@ -2826,40 +2827,38 @@ function theme_form_required_marker($variables) {
+      '#element' => $element,

This 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?

joelpittet’s picture

re: #18 here's an example

Status: Needs review » Needs work

The last submitted patch, 19: 2190427-form-render-element-breaking.patch, failed testing.

star-szr’s picture

Issue tags: +Twig conversion
joelpittet’s picture

Status: Needs work » Needs review

Hiding 19 and #17 is the one to review.

star-szr’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
4.03 KB

Re-rolled.

martin107’s picture

zaphoyd’s picture

I'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:

<label for="edit-title-0-value">Title<span class="form-required" aria-hidden="true">*</span></label>

After:

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element_label') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->
<label for="edit-title-0-value">Title<span class="form-required" aria-hidden="true">*</span></label>
<!-- END OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->

Body field (not required, can be collapsed/expanded with summary field)

Before (expanded summary):

<label for="edit-body-0-value">Body</label>

After (expanded summary)

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element_label') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->
<label for="edit-body-0-value">Body</label>
<!-- END OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->

Before (collapsed summary):

<label for="edit-body-0-value">
    "Body"
    <span class="field-edit-link">
        " ("
        <button type="button" class="link link-edit-summary">Edit summary</button>
        ")"
    </span>
</label>

After (collapsed summary):

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element_label') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->
<label for="edit-body-0-value">
    "Body"
    <span class="field-edit-link">
        " ("
        <button type="button" class="link link-edit-summary">Edit summary</button>
        ")"
    </span>
</label>
<!-- END OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->

Tags field (not required, nothing special)
Before:

<label for="edit-field-tags">Tags</label>

After (+twig_debug):

<!-- THEME DEBUG -->
<!-- CALL: _theme('form_element_label') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->
<label for="edit-field-tags">Tags</label>
<!-- END OUTPUT from 'core/modules/system/templates/form-element-label.html.twig' -->

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.

joelpittet’s picture

Issue tags: -Needs manual testing

Awesome manual testing @zaphoyd, I'm removing that tag.

benjifisher’s picture

Assigned: Unassigned » benjifisher

I will profile the patch.

benjifisher’s picture

The 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 of filter_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!

benjifisher’s picture

Issue tags: -needs profiling

Profiling completed:

=== 8.x..8.x compared (534ad8dec7444..534ad972c8c0d):

ct  : 87,661|87,661|0|0.0%
wt  : 621,041|619,587|-1,454|-0.2%
cpu : 563,923|563,336|-587|-0.1%
mu  : 14,278,500|14,278,500|0|0.0%
pmu : 14,415,976|14,415,976|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=534ad8dec7444&...

=== 8.x..2152215_twig_form_element compared (534ad8dec7444..534ad9c94e95b):

ct  : 87,661|87,749|88|0.1%
wt  : 621,041|619,393|-1,648|-0.3%
cpu : 563,923|563,424|-499|-0.1%
mu  : 14,278,500|14,302,256|23,756|0.2%
pmu : 14,415,976|14,440,276|24,300|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=534ad8dec7444&...

Looking at the XHProf results,

  • The "bottom line" numbers show negligible differences (like +-0.3% or less).
  • I see 3 calls to __TwigTemplate_2fd348da3df0021653c75db706bfbc964d51daef0e20d41b9be683b7c4ee690a::doDisplay() in the patched version, 3 calls to theme_form_element_label() in the unpatched version: good!
  • The funny thing is that, when I turn on Twig debugging in the patched version, I see 8 references to form-element-label.html.twig. Something is wrong!
benjifisher’s picture

Assigned: benjifisher » Unassigned

I forgot to un-assign the issue.

star-szr’s picture

I 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.

star-szr’s picture

Relevant (for disabling render caching): https://gist.github.com/Cottser/10997441

joelpittet’s picture

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Another 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.

=== 8.x..8.x compared (536ffbe232db1..536ffc2aeacea):

ct  : 102,157|102,157|0|0.0%
wt  : 644,653|647,729|3,076|0.5%
cpu : 635,652|639,026|3,374|0.5%
mu  : 42,082,368|42,082,368|0|0.0%
pmu : 42,220,088|42,220,088|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=536ffbe232db1&...

=== 8.x..2152215-theme_form_element_label compared (536ffbe232db1..536ffcbacf96d):

ct  : 102,157|102,493|336|0.3%
wt  : 644,653|646,236|1,583|0.2%
cpu : 635,652|637,452|1,800|0.3%
mu  : 42,082,368|42,117,704|35,336|0.1%
pmu : 42,220,088|42,255,504|35,416|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=536ffbe232db1&...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 34: 2264753-theme-update-last-check.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
4.2 KB

Whoops, wrong patch (crossed repo) for the re-roll. Let's try this one.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 2152215-theme_form_element_label-34.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

That test fails without this patch. Head is broken or something? Anyways this still applies and RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 2152215-theme_form_element_label-34.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Random testbot fail... ApcuBackendUnitTest

xjm’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 2152215-theme_form_element_label-34.patch, failed testing.

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
0 bytes

Re-rolled

joelpittet’s picture

For real...

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 46: 2152215-twig-form_element_label-46.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review

46: 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')

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @martin107 back to RTBC

star-szr’s picture

Updating the suggested commit message.

+++ b/core/includes/form.inc
@@ -2962,53 +2962,48 @@ function template_preprocess_form_element(&$variables) {
  * @ingroup themeable
  */

Since we're changing this hunk, @ingroup themeable doesn't belong here. Removing it, still RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/form.inc
@@ -2962,53 +2962,46 @@ function template_preprocess_form_element(&$variables) {
+  $variables['title'] = (isset($element['#title']) && $element['#title'] !== '') ? Xss::filterAdmin($element['#title']) : '';

Cleaner if $variables['title'] = empty($element['#title']) ? Xss::filterAdmin($element['#title']) : '';

bayousoft’s picture

Assigned: Unassigned » bayousoft
benjifisher’s picture

@bayousoft: Are you thinking what I am thinking? The suggestion in #51 looks backwards to me.

bayousoft’s picture

@benjifisher yup

bayousoft’s picture

FileSize
4.17 KB
781 bytes

Rerolled with suggested syntax reversed.

star-szr’s picture

@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.

bayousoft’s picture

FileSize
4.17 KB
781 bytes

Rerolled with suggested syntax

bayousoft’s picture

Status: Needs work » Needs review

The last submitted patch, 55: 2152215-53.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 57: 2152215-53.patch, failed testing.

bayousoft’s picture

Any ideas as to why this patch failed so miserably?

bayousoft’s picture

Assigned: bayousoft » Unassigned
benjifisher’s picture

#51 says

$variables['title'] = empty($element['#title']) ? Xss::filterAdmin($element['#title']) : '';

and I think it should be

$variables['title'] = empty($element['#title']) ? '' : Xss::filterAdmin($element['#title']);

but the interdiff in #57 has

$variables['title'] = Xss::filterAdmin($element['#title']) ? empty($element['#title']) : '';

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 as empty(NULL) or TRUE.

I wish I could preview before posting ...

star-szr’s picture

I think I read it as !empty, then it would make more sense :)

bayousoft’s picture

Assigned: Unassigned » bayousoft
bayousoft’s picture

So is the process to keep making new branches every time I upload a patch?

bayousoft’s picture

New branches to create a useful interdiff.txt that is.

benjifisher’s picture

@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:

$ git checkout master
$ git pull
$ git checkout -b my_issue
$ git apply --index ....patch
$ git commit "applied patch from issues queue"
<hack>
$ git diff master > my_new_patch.diff
$ git diff > interdiff_51_68.txt

(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,

$ git reset --hard
<hack>
$ git diff master > my_new_patch.diff
$ git diff > interdiff_51_70.txt

(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).

bayousoft’s picture

thx @benjifisher

The last submitted patch, 55: 2152215-53.patch, failed testing.

bayousoft’s picture

FileSize
4.17 KB
751 bytes

rerolled

bayousoft’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 71: 2152215-71.patch, failed testing.

benjifisher’s picture

You could try running Drupal\system\Tests\Form\ElementsLabelsTest locally; that is the one that is failing. Maybe even look at the code, add some extra debug() 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

$ git checkout master
$ git pull

and then run the test without applying the patch ...

bayousoft’s picture

That path does not exist in my codebase. I'm wondering where to find that test.

joelpittet’s picture

Enable 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

joelpittet’s picture

Status: Needs work » Reviewed & tested by the community

Oh 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...

star-szr’s picture

Might be worth a code comment then.

benjifisher’s picture

@joelpittet: I think what you meant to say was

(isset(0) && 0 !== '') == TRUE

Note the use of !== (as in the original version) rather than !=.

bayousoft’s picture

Assigned: bayousoft » Unassigned
bayousoft’s picture

@joelpittet Okay. Well at least I got to practice the process. On to the next issue I go.

joelpittet’s picture

@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.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
php -r "var_dump(empty('0'));"
bool(true)

How could I forget :)

Committed #50 b962ed4 and pushed to 8.x. Thanks!

  • Commit b962ed4 on 8.x by alexpott:
    Issue #2152215 by joelpittet, mr.baileys, benjifisher, martin107,...
joelpittet’s picture

Yeah because that makes sense!
PHP--

Thanks for committing, another one bites the dust!

benjifisher’s picture

The example in #83 does not work for me. It looks as thoughempty() changed in PHP 5.5. For earlier versions, use

php -r '$foo = "0"; var_dump(empty($foo));'

Note 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));"

joelpittet’s picture

@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

bool(true)

Which is the expected unexpected result of PHP. Do you see bool(false) on your machine?

benjifisher’s picture

@joelpittet: No, I see a parse error:

$ php -r "var_dump(empty('0'));"
PHP Parse error:  syntax error, unexpected ''0'' (T_CONSTANT_ENCAPSED_STRING) in Command line code on line 1

Parse error: syntax error, unexpected ''0'' (T_CONSTANT_ENCAPSED_STRING) in Command line code on line 1

This is what I expect, since I am using PHP 5.4.8. According to http://us1.php.net/empty,

Prior to PHP 5.5, empty() only supports variables; anything else will result in a parse error.

Since '0' is an expression, not a variable, empty('0') does not parse.

joelpittet’s picture

Ah I see what you mean now, thanks for clarifying. I thought you were using 5.5 too... The light has turned on;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alimac’s picture

Issue tags: -#SprintWeekend2014 +SprintWeekend2014

Minor tag cleanup - please ignore.