Convert theme_form_element_label() to Twig

First try...

Tests where failing when both title and required where empty.

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.

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

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

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.

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

6: 2152215-6-twig-form_element_label.patch queued for re-testing.

6: 2152215-6-twig-form_element_label.patch queued for re-testing.

Re-rolling.

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

re: #18 here's an example

Hiding 19 and #17 is the one to review.

Re-rolled.

reroll.

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.

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

I will profile the patch.

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!

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!

I forgot to un-assign the issue.

Re-rolled because required marker is now a CSS style! #2152217: Remove theme_form_required_marker() from the theme system - use CSS instead

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

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

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

37: 2152215-theme_form_element_label-34.patch queued for re-testing.

Random testbot fail... ApcuBackendUnitTest

37: 2152215-theme_form_element_label-34.patch queued for re-testing.

Re-rolled

For real...

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

Thanks @martin107 back to RTBC

+++ 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: Are you thinking what I am thinking? The suggestion in #51 looks backwards to me.

@benjifisher yup

Rerolled with suggested syntax reversed.

Rerolled with suggested syntax

Any ideas as to why this patch failed so miserably?

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

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

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

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

thx @benjifisher

rerolled

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

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

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

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

@joelpittet: I think what you meant to say was

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

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

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

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

php -r "var_dump(empty('0'));"
bool(true)

How could I forget :)

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

Yeah because that makes sense!
PHP--

Thanks for committing, another one bites the dust!

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

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

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

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

Minor tag cleanup - please ignore.