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

  • Patch
  • Patch review
  • Manual testing
  • Profiling

How to test this code

  1. Create a content type with <input> fields
  2. View the Add Node form for this content type
  3. Check the source
  4. Apply the patch
  5. Reload the Add node form
  6. Check the source
  7. Compare the before and after sources

Comments

Cottser’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
2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 55,969 pass(es), 268 fail(s), and 247 exception(s). View

Let see... Not sure about it but it's a try :)

sheilaj’s picture

+++ b/core/modules/system/templates/input.html.twig
@@ -0,0 +1,17 @@
+<input {{ attributes }} />{{ children }}

There a space between input and attributes that should be removed.

sqndr’s picture

Issue tags: -Needs manual testing, -Needs profiling +Needs Review
FileSize
2.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2152219-convert-theme-input-4.patch. Unable to apply patch. See the log in the details link for more information. View

I remove the space as suggested in comment #3.

Status: Needs review » Needs work

The last submitted patch, 4: 2152219-convert-theme-input-4.patch, failed testing.

Cottser’s picture

Thanks everyone!

I tried to apply the patch from #4 locally and got this:

fatal: corrupt patch at line 76

The patch from #2 still applies for me with an offset.

Cottser’s picture

Issue tags: -Needs Review +Novice

Looks 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 :)

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
PASSED: [[SimpleTest]]: [MySQL] 63,214 pass(es). View

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_core-convert-theme-input-2152219-8.patch, failed testing.

sqndr’s picture

The last submitted patch, 4: 2152219-convert-theme-input-4.patch, failed testing.

sqndr’s picture

The last submitted patch, 2: 2152219-convert-theme-input.patch, failed testing.

InternetDevels’s picture

Status: Needs work » Needs review
sqndr’s picture

The last submitted patch, 4: 2152219-convert-theme-input-4.patch, failed testing.

sqndr’s picture

sqndr’s picture

This last patch fixes the new line problem and removes the unnecessary space.

Cottser’s picture

Issue tags: +Twig conversion
joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/includes/form.inc
    @@ -2167,33 +2167,18 @@ function form_process_autocomplete($element, &$form_state) {
    + * Preprocesses variables for input templates.
      *
      * @param array $variables
    

    Missing the
    * Default template: input.html.twig.

  2. +++ b/core/includes/form.inc
    @@ -2167,33 +2167,18 @@ function form_process_autocomplete($element, &$form_state) {
    +  $variables['children'] = (!empty($element['#children'])) ? $element['#children'] : '';
    

    #children key is defined already in drupal_render(), don't do the empty() check which likely breaks printing 0s

  3. +++ b/core/modules/system/templates/input.html.twig
    @@ -0,0 +1,17 @@
    + @todo: remove this file once http://drupal.org/node/1819284 is resolved.
    + This is identical to core/modules/system/templates/container.html.twig
    

    I don't think this @todo is relevant.

mdrummond’s picture

FileSize
2.28 KB
PASSED: [[SimpleTest]]: [MySQL] 64,220 pass(es). View
1.32 KB

Here's a patch and an interdiff with the changes joelpittet suggested.

mdrummond’s picture

Status: Needs work » Needs review
joelpittet’s picture

Perfecto, thanks @mdrummond. Adding tags back on this one.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work

Thanks @mdrummond! This could use some small tweaks.

  1. +++ b/core/includes/form.inc
    @@ -2167,33 +2167,20 @@ function form_process_autocomplete($element, &$form_state) {
    - * Preprocesses variables for theme_input().
    + * Preprocesses variables for input templates.
    

    This should be "Prepares" instead of "Preprocesses" per https://drupal.org/node/1354#themepreprocess.

  2. +++ b/core/includes/form.inc
    @@ -2167,33 +2167,20 @@ function form_process_autocomplete($element, &$form_state) {
      *
    - * @ingroup themeable
    

    We need to remove the line above @ingroup themeable as well so there isn't a blank line at the bottom of the docblock.

  3. +++ b/core/includes/form.inc
    @@ -2167,33 +2167,20 @@ function form_process_autocomplete($element, &$form_state) {
    +  $variables['attributes'] = new Attribute($element['#attributes']);
    

    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.

  4. +++ b/core/modules/system/templates/input.html.twig
    @@ -0,0 +1,15 @@
    + * - attributes: A list of HTML attributes for the wrapper element.
    

    The attributes aren't for a wrapper element, they're for the input element.

mdrummond’s picture

Status: Needs work » Needs review
FileSize
2.2 KB
PASSED: [[SimpleTest]]: [MySQL] 64,236 pass(es). View
1.38 KB

Here's a patch and interdiff with the changes Cottser suggested.

mdrummond’s picture

This patch is green. Manual testing and profiling up next?

joelpittet’s picture

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

mdrummond’s picture

So what's next. Profiling?

joelpittet’s picture

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

Gold’s picture

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

joelpittet’s picture

@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

Gold’s picture

Issue summary: View changes

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

Gold’s picture

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

  • Title
  • checkboxen
  • Entity Reference
  • File
  • Number fields (all types)
  • Text
  • Submit
longwave’s picture

Status: Needs work » Needs review

Did 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!

Gold’s picture

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

joelpittet’s picture

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

Gold’s picture

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

Gold’s picture

Status: Needs review » Reviewed & tested by the community

Failed to update the status with #37. Or does this need more testing before this status is reached?

mdrummond’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs manual testing

We need profiling before we can RTBC this, at least as far as I understand the process.

Gold’s picture

@mdrummond Cheers. Knocked the Needs manual testing tag off at least. :)

mdrummond’s picture

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

joelpittet’s picture

Issue tags: -Novice
joelpittet’s picture

+++ b/core/includes/form.inc
@@ -2167,33 +2167,18 @@ function form_process_autocomplete($element, &$form_state) {
-  $variables['attributes'] = new Attribute($element['#attributes']);

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

Gold’s picture

Yeah... 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. :)

joelpittet’s picture

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

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

rodrigoaguilera’s picture

Assigned: Unassigned » rodrigoaguilera
rodrigoaguilera’s picture

Assigned: rodrigoaguilera » Unassigned
Status: Needs work » Needs review
FileSize
2.01 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/includes/form.inc. View

reroll
I'm not sure how to make the interdiff

Status: Needs review » Needs work

The last submitted patch, 48: convert-theme-input-2152219-48.patch, failed testing.

rodrigoaguilera’s picture

FileSize
2.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,810 pass(es). View
rodrigoaguilera’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue tags: -Needs reroll

@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

willieseabrook’s picture

Issue tags: +TUNIS_2014_MARCH
haithem_pro’s picture

Assigned: Unassigned » haithem_pro
haithem_pro’s picture

Patch Needs profiling , I'm not so good at that !!

haithem_pro’s picture

Assigned: haithem_pro » Unassigned
Cottser’s picture

Status: Needs review » Needs work

The docblock for the preprocess function has regressed a bit. Compare to the patch in #25.

Should be:

Prepares variables for input templates.

Default template: input.html.twig.

@param array etc.
joelpittet’s picture

Status: Needs work » Needs review
FileSize
2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,034 pass(es). View

I'm just curious what ends up in children, the docs are vague so maybe the tests will give hints.

joelpittet’s picture

FileSize
1.09 KB
2.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,991 pass(es). View

Here's the fixes mentioned in #57 + a couple more.

Cottser’s picture

The children are "optional" here is the shortest explanation I think.

joelpittet’s picture

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

Cottser’s picture

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

joelpittet’s picture

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

+++ b/core/modules/system/templates/input.html.twig
@@ -0,0 +1,15 @@
+ * - children: The rendered child elements.

Can we just change this to: "Optional additional rendered elements."?

Cottser’s picture

Works for me.

joelpittet’s picture

FileSize
654 bytes
2.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,038 pass(es). View

Cool, that's changed. Profiling next, stop me if we don't need it for this?

steinmb’s picture

Assigned: Unassigned » steinmb

Time for profiling and a bit monkey work.

steinmb’s picture

Issue tags: -TUNIS_2014_MARCH

Profiling data

Run 538ce4363dd76 uploaded successfully for drupal-perf-steinmb.
Run 538ce46e85e63 uploaded successfully for drupal-perf-steinmb.

=== 8.x..8.x compared (538ce4363dd76..538ce46e85e63):

ct  : 28,621|28,621|0|0.0%
wt  : 245,677|245,486|-191|-0.1%
cpu : 223,421|223,704|283|0.1%
mu  : 18,718,744|18,718,744|0|0.0%
pmu : 18,784,568|18,784,568|0|0.0%

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.

=== 8.x..2152219-twig-theme_input compared (538ce4363dd76..538ce49a96f74):

ct  : 28,621|28,905|284|1.0%
wt  : 245,677|247,614|1,937|0.8%
cpu : 223,421|225,372|1,951|0.9%
mu  : 18,718,744|18,738,400|19,656|0.1%
pmu : 18,784,568|18,804,272|19,704|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=538ce4363dd76&...

steinmb’s picture

Status: Needs review » Reviewed & tested by the community

Markup compared and diffed

--- /Users/steinmb/Downloads/foo.html Mon Jun  2 16:05:36 2014
+++ /Users/steinmb/Downloads/foo.html Mon Jun  2 16:12:28 2014
@@ -1,6 +1,7 @@
 <div class="form-item form-type-textfield form-item-title-0-value">
       <label class="form-required" for="edit-title-0-value">Title</label>
       <input type="text" aria-required="true" required="required" placeholder="" maxlength="255" size="60" value="" name="title[0][value]" id="edit-title-0-value" aria-describedby="edit-title-0-value--description" class="text-full form-text required">
+
           <div id="edit-title-0-value--description" class="description">
       The title of this node, always treated as non-markup plain text.
     </div>
steinmb’s picture

Assigned: steinmb » Unassigned
Issue tags: -Needs profiling
Cottser’s picture

Issue summary: View changes

Patch looks good to me, updating suggested commit message.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 406da31 and pushed to 8.x. Thanks!

  • Commit 406da31 on 8.x by alexpott:
    Issue #2152219 by joelpittet, mdrummond, rodrigoaguilera, sqndr,...

Status: Fixed » Closed (fixed)

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