CommentFileSizeAuthor
#114 interdiff.txt825 bytesstar-szr
#114 1898480-114.patch64.08 KBstar-szr
#113 1898480-113.patch63.28 KBstar-szr
#111 1898480-111.patch65.26 KBstar-szr
#107 interdiff.txt3.79 KBjoelpittet
#107 1898480-107-twig-form.patch65.19 KBjoelpittet
#105 1898480-105-twig-form.patch63.82 KBjoelpittet
#105 interdiff.txt1.39 KBjoelpittet
#97 form-twig-1898480-97.patch63.55 KBhussainweb
#97 form-twig-1898480-intediff-95-97.txt5.4 KBhussainweb
#95 1898480-95-twig-theme-form.patch60.89 KBjoelpittet
#95 interdiff.txt12.07 KBjoelpittet
#92 drupal-twig-forminc--1898480-92.patch62.27 KBhussainweb
#89 drupal-twig-forminc--1898480-89.patch62.23 KBhussainweb
#89 drupal-twig-forminc--1898480-interdiff-86-89.txt788 byteshussainweb
#86 drupal-twig-forminc--1898480-86.patch62.03 KBhussainweb
#81 drupal-twig-forminc--1898480-81.patch61.95 KBhussainweb
#77 drupal-twig-forminc--1898480-77.patch62.1 KBhussainweb
#71 drupal-twig-forminc--1898480-71.patch62.36 KBdsdeiz
#71 interdiff.txt4.92 KBdsdeiz
#61 drupal-twig-forminc--1898480-61.patch62.67 KBHanpersand
#54 drupal-twig-forminc--1898480-54.patch62.94 KBshanethehat
#54 interdiff.txt1.02 KBshanethehat
#53 drupal-twig-forminc--1898480-53.patch62.94 KBshanethehat
#53 interdiff.txt21.68 KBshanethehat
#49 drupal-twig-forminc--1898480-49.patch59.57 KBshanethehat
#49 interdiff.txt2.55 KBshanethehat
#46 drupal-twig-forminc--1898480-46.patch59.5 KBshanethehat
#46 interdiff.txt3.42 KBshanethehat
#41 drupal-twig-views-form-pre-render.patch2.37 KBsteveoliver
#36 drupal-twig-forminc--1898480-32-36-interdiff.txt1.57 KBsteveoliver
#36 drupal-twig-forminc--1898480-36.patch57.13 KBsteveoliver
#34 drupal-twig-forminc--32-34--interdiff.txt1.57 KBsteveoliver
#34 drupal-twig-forminc--34.patch182.79 KBsteveoliver
#32 drupal-twig-forminc--1898480-32.patch57.87 KBsteveoliver
#28 drupal-twig-forminc--28.patch59.46 KBsteveoliver
#25 drupal-twig-forminc--1898480-25.patch59.45 KBsteveoliver
#23 drupal-twig-forminc--1898480-23.patch59.32 KBsteveoliver
#21 drupal-twig-forminc--1898480-21.patch57.28 KBsteveoliver
#19 drupal-twig-forminc-all-but-element-label--1898480-19.patch45.92 KBsteveoliver
#19 drupal-twig-forminc-element-label--1898480-19-do-not-test.patch4.94 KBsteveoliver
#18 drupal-twig-forminc--1898480-18.patch48.87 KBsteveoliver
#16 drupal-twig-forminc--1898480-16.patch48.67 KBsteveoliver
#14 drupal-twig-forminc--1898480-14.patch48.67 KBsteveoliver
#12 drupal-twig-forminc--1898480-12.patch52.98 KBsteveoliver
#10 drupal-twig-forminc--1898480-10.patch52.8 KBsteveoliver
#8 drupal-twig-forminc--1898480-8.patch52.3 KBsteveoliver
#5 drupal-form.inc-twig--1898480-5.patch35.13 KBsteveoliver
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

c4rl’s picture

Issue tags: +Twig

Tagging

c4rl’s picture

Based on #1905584: Move base theme system templates into /core/templates it seems this issue would be accommodated by #1898454: system.module - Convert PHPTemplate templates to Twig, though that may result in a large patch.

c4rl’s picture

Ignore what I said about system.module in #2, we'll proceed to convert this separately of system.module and put in /core/templates as described in #1905584: Move base theme system templates into /core/templates

steveoliver’s picture

Assigned: Unassigned » steveoliver

I'll tackle this.

steveoliver’s picture

Status: Active » Needs review
FileSize
35.13 KB

Work in progress. Just posting in case I don't get to this soon and/or someone else reeeallly wants to work on it. I'd rather finish it though, time permitting, since I've been working on it so far.

Of note is the idea that, like in #1812724-10: Consolidate all form element templates and add theme_hook_suggestons, I'm trying to drive a separation between Render/Form API and Theme APIs by having the preprocess/theme functions handle generic element rendering while Form API stays responsible for FAPI-specific things like tableselect, etc. More on that later.

Status: Needs review » Needs work

The last submitted patch, drupal-form.inc-twig--1898480-5.patch, failed testing.

steveoliver’s picture

Oh, that's right, there's no theme engine at install time...

steveoliver’s picture

Status: Needs work » Needs review
FileSize
52.3 KB

Alright, let's see. *Includes patch from #1942490: Make Twig engine available during install.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-8.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
52.8 KB

Should fix at least some fails for textareas.

steveoliver’s picture

Issue summary: View changes

Added git attribution code.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-10.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
52.98 KB

This (hacky) should take care of some form fails.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-12.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
48.67 KB

OK, going back on the #pre_render stuff because I can't figure out why/where node forms /don't/ seem to use #type 'form'. Maybe less than 10 fails? Still don't know what the cache issue is about.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-14.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
48.67 KB

This patch should fix all the exceptions. Still can't figure out that 1 fail. Translation seems to not be getting saved.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-16.patch, failed testing.

steveoliver’s picture

Assigned: steveoliver » Unassigned
FileSize
48.87 KB

I'd love someone to take a look at this patch and help me get that one last Locale test to pass.

I tested without changing theme_textarea or theme_input, and still no go. I didn't test without theme_form*, so that might be the next place to look. Unassigning until I can get back onto this or someone else can have a look.

steveoliver’s picture

theme_form_element_label() conversion is what causes the 1 fail above. What's strange is
1. The test seems to be testing successful form submission, which should be unaffected by labels (and is, as I removed the label altogether and the test passed)
2. The actual form element label tests pass for the "form-element-label.patch".

So the this main patch should pass, which excludes the form element label conversion.

The label patch needs work, but I can't tell what... What I do know is, based on the test output, the string actually isn't translated. The question in my mind now is Why the heck would that test fail based on a form element label. If anything I'd expect issues from form, submit, etc...

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc-all-but-element-label--1898480-19.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
57.28 KB

Drum roll, please... (this should be green).

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-21.patch, failed testing.

steveoliver’s picture

Status: Needs work » Needs review
FileSize
59.32 KB

Woops, forgot the install.core.inc patch (necessary until #1942490: Make Twig engine available during install lands). Here it is

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-23.patch, failed testing.

steveoliver’s picture

Assigned: Unassigned » steveoliver
Status: Needs work » Needs review
FileSize
59.45 KB

Thrashmaster! Need to figure out how to not have to do this:

$element = $variables['element']['#element'];

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-25.patch, failed testing.

steveoliver’s picture

I /will/ get this thing green... I /will/ get this thing green...

steveoliver’s picture

Status: Needs work » Needs review
FileSize
59.46 KB

Added whitespace modifier dashes to get label tags and contents all on one line. Should fix those two fails. Show me green!

steveoliver’s picture

OK, after all that thrashing, here's an overview of what's going on in this patch and a few comments on pieces of the code.

What this does, besides straight conversion:

  1. Removes theme_date(), and uses #theme 'input__date'
  2. Removes theme_tableselect(), and uses #theme 'table__tableselect', with tableselect preparation handling in a #pre_render callback instead
  3. Removes FAPI-specific preparation from preprocess functions and moves into into #pre_render as well
  4. This of course means that we have theme/preprocess functions for HTML elements that aren't tied to Drupal's Forms API, which allows us to use #type for FAPI elements and #theme for just HTML elements, if we wanted to produce them on their own.

Here are some comments on specific pieces of code:

 /**
- * Prepares a #type 'radio' render element for theme_input().
+ * Adds form-specific attributes to a 'radio' #type element.
  *
  * @param array $element
  *   An associative array containing the properties of the element.
  *
  * @return array
- *   The $element with prepared variables ready for theme_input().
+ *   The $element with variables prepared for #theme 'input__radio'.
  */
 function form_pre_render_radio($element) {

In the comments change here and throughout this patch I'm trying to decouple and clarify the relationship between Form and Theme APIs.

+++ b/core/includes/theme.inc
@@ -3271,51 +3271,58 @@ function drupal_common_theme() {
-    ),
-    'tableselect' => array(
-      'render element' => 'element',
+      'template' => 'textarea',

This removes theme_tableselect(). #type 'tableselect' element now does its preparation in it's own #pre_render and uses #theme 'table__tableselect'. Again, separating Forms/element handling from HTML element themeing.

--- a/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
+++ b/core/modules/system/lib/Drupal/system/Tests/Form/FormTest.php
@@ -94,7 +94,7 @@ function testRequiredFields() {
     $elements['file']['empty_values'] = $empty_strings;
 
     // Regular expression to find the expected marker on required elements.
-    $required_marker_preg = '@<label.*<abbr class="form-required" title="This field is required\.">\*</abbr></label>@';
+    $required_marker_preg = '@<label.*<abbr class="form-required" title="This field is required\.">\*</abbr>\s*?</label>@';

The addition of \s*? is required because, since Twig (PHP) files need a trailing newline, the form-label-required-marker.html.twig produces a newline within the label. I saw no other way around this, and seems fine to me.

+++ b/core/modules/system/system.module
@@ -301,7 +301,7 @@ function system_element_info() {
     '#limit_validation_errors' => FALSE,
     '#process' => array('form_process_button', 'ajax_process_form'),
     '#pre_render' => array('form_pre_render_button'),
-    '#theme_wrappers' => array('input__submit'),
+    '#theme' => 'input__submit',
   );
   $types['button'] = array(
     '#input' => TRUE,
     '#limit_validation_errors' => FALSE,
     '#process' => array('form_process_button', 'ajax_process_form'),
     '#pre_render' => array('form_pre_render_button'),
-    '#theme_wrappers' => array('input__button'),
+    '#theme' => 'input__button',
   );
   $types['image_button'] = array(
     '#input' => TRUE,
     '#has_garbage_value' => TRUE,
     '#src' => NULL,
     '#pre_render' => array('form_pre_render_image_button'),
-    '#theme_wrappers' => array('input__image_button'),
+    '#theme' => 'input__image_button',
   );

I'm honestly not sure why I needed to change these from #theme to #theme_wrappers, but I believe this was the only thing that worked.

--- a/core/modules/system/templates/datetime.html.twig
+++ b/core/modules/system/templates/datetime.html.twig
@@ -1,6 +1,7 @@
 {#
 /**
- * Returns HTML for a date / time.
+ * @file
+ * Default theme implementation for a date / time element.
  *
  * Available variables
  * - timestamp: (optional) A UNIX timestamp for the datetime attribute.
  */
 #}
 {# @todo Revisit once http://drupal.org/node/1825952 is resolved. #}
-<time class="{{ attributes.class }}" {{ attributes }}>{{ html ? text|raw : text|escape }}</time>
+<time{{ attributes }}>{{ html ? text|raw : text|escape }}</time>

This template existed before this form.inc conversion. I'm just cleaning up the comment and attributes.

--- /dev/null
+++ b/core/modules/system/templates/textarea.html.twig
@@ -0,0 +1,18 @@
+{#
+/**
+ * @file
+ * Default theme implementation for a textarea element.
+ *
+ * Available variables:
+ * - attributes: An array of HTML attributes for the textarea element.
+ * - value: Plain text value of the textarea.
+ *
+ * @see template_preprocess()
+ * @see template_preprocess_textarea()
+ *
+ * @ingroup themeable
+ */
+#}
+<div{{ wrapper_attributes }}>
+  <textarea{{ attributes }}>{{ value }}</textarea>
+</div>
\ No newline at end of file

Woops, I need a newline here.

+++ b/core/includes/form.inc
@@ -4779,39 +4739,40 @@ function theme_form_required_marker($variables) {
+    // If the element is required, a required marker is appended to the label.
+    $variables['label'] = (isset($element['#title'])) ? filter_xss_admin($element['#title']) : NULL;
+    $variables['required'] = (!empty($element['#required'])) ? array('#theme' => 'form_required_marker', '#element' => $element) : '';

This render array (above)....

+++ b/core/includes/form.inc
@@ -4779,39 +4739,40 @@ function theme_form_required_marker($variables) {
+function template_preprocess_form_element_label(&$variables) {
+  // @todo: Figure out how to do this differently (calling from form_element).
+  $element = $variables['element']['#element'];

...requires me to target the element like this, instead of just $variables['element'], which I'm pretty sure is what we want to be doing. Could use an extra set of eyes here.

steveoliver’s picture

Re #29, point #4: This doesn't actually get us all the way to being able to use those theme functions without knowing about a render element in preprocess, so maybe I'll post another patch that pulls $element knowledge out of the preprocess functions and all the way into #pre_render.

joelpittet’s picture

@steveoliver merged your change for the datetime doc into the datetime twig conversion
#1939080: Convert theme_datetime() to Twig

If it doesn't affect this patch you could take it out.

type=>tableselect is already on the chopping block @see #1876714: Remove #type 'tableselect' but it's waiting on the type=>table conversion @see #1876712: [meta] Convert all tables in core to new #type 'table' to be completed. So may be a bit early to drop it because it's used in locale, comment, system, rest, update modules and a few tests.

small typo ending period instead of comma:
+ * Default template: select.html.twig,

Not sure about the backtick ` if that was intended or not? @see form_pre_render_date

Otherwise great work!

steveoliver’s picture

Re-roll without installer part, in light of #1942490: Make Twig engine available during install.

star-szr’s picture

Agreed with #31, let's make it so this patch doesn't touch datetime.html.twig. See also #1898454-13: system.module - Convert PHPTemplate templates to Twig.

Fantastic work on this by the way, @steveoliver.

steveoliver’s picture

steveoliver’s picture

Woops-- accidentally included all kinds of other stuff -- looks like I forgot to rebase... new patch coming up

steveoliver’s picture

Status: Needs review » Needs work
Issue tags: -Twig

The last submitted patch, drupal-twig-forminc--1898480-36.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: +Twig
star-szr’s picture

Status: Needs review » Needs work

It looks like the only conversion missing in this issue is theme_exposed_filters(), and it looks like it has been converted in the sandbox already. Great work @steveoliver :)

star-szr’s picture

Also theme_exposed_filters() is weird.

The theme function is defined in drupal_common_theme() in theme.inc under the form.inc heading, but the theme function itself is in system.module. So we should remove the theme function from system.module but add the preprocess function (if necessary) to form.inc.

star-szr’s picture

Issue summary: View changes

Formatting.

steveoliver’s picture

...something like this...

star-szr’s picture

star-szr’s picture

Issue summary: View changes

Add conversion summary table

star-szr’s picture

Issue tags: +Needs manual testing

Tagging.

star-szr’s picture

Assigned: steveoliver » Unassigned
Issue tags: +Novice

Got the okay from @steveoliver to unassign for the remaining conversion.

Bringing over the theme_exposed_filters() conversion from the sandbox would be a good Novice task. I recommend taking a look at @c4rl's screencast: http://www.youtube.com/watch?v=HS4yKJjrb2E

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
3.42 KB
59.5 KB
star-szr’s picture

Status: Needs review » Needs work

That's great, thanks @shanethehat. The below items can be done to move this issue forward:

  1. +++ b/core/includes/form.incundefined
    @@ -4762,6 +4762,30 @@ function template_preprocess_form_element_label(&$variables) {
    + * Preprocess variables for exposed filters templates.
    ...
    + * @param $variables
    

    "Prepares" instead of "Preprocess" and @param array $variables please per #1913208: [policy] Standardize template preprocess function documentation.

  2. +++ b/core/includes/form.incundefined
    @@ -4762,6 +4762,30 @@ function template_preprocess_form_element_label(&$variables) {
    +      $items[] = drupal_render($form['current'][$key]);
    ...
    +    $variables['items'] .= theme('item_list', array('items' => $items, 'attributes' => array('class' => array('clearfix', 'current-filters'))));
    

    We should try and remove the drupal_render() and theme() calls here per http://drupal.org/node/1920746.

    For $variables['items'] you should be able to just treat it as an array, i.e:

    $variables['items'][] = array(
      '#theme' => 'item_list',
      …
    );

    And set $variables['items'] to an empty array instead of an empty string before the if statement.

  3. +++ b/core/modules/system/templates/exposed-filters.html.twigundefined
    @@ -0,0 +1,20 @@
    + * Available variables:
    + *   - form: An associative array containing the structure of the form.
    

    We should unindent the available variables list here per http://drupal.org/node/1354#lists, and document the 'items' variable that is being added in the preprocess function.

  4. Not part of the theme_exposed_filters() conversion, but this can be fixed as well:
    +++ b/core/modules/system/templates/textarea.html.twigundefined
    @@ -0,0 +1,18 @@
    \ No newline at end of file
    

    textarea.html.twig needs a blank line at the end of the file per http://drupal.org/coding-standards#indenting.


And a note for other reviewers:
+++ b/core/includes/form.incundefined
@@ -4762,6 +4762,30 @@ function template_preprocess_form_element_label(&$variables) {
+  $variables['form'] = drupal_render_children($form);

I think this is fine for now, we are working on getting rid of these calls in #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead but for conversion purposes this works. Since this doesn't need special handling like #1898432: node.module - Convert PHPTemplate templates to Twig I don't think we need to add a @todo here.

star-szr’s picture

Issue summary: View changes

Update summary table

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
2.55 KB
59.57 KB
shanethehat’s picture

This triggers a whitespace error when applied, but I think it's caused by #47 point 4.

star-szr’s picture

Status: Needs review » Needs work

Excellent work @shanethehat!

+++ b/core/includes/form.incundefined
@@ -4762,24 +4762,30 @@ function template_preprocess_form_element_label(&$variables) {
+        '#attributes' => array(
+            'class' => array('clearfix', 'current-filters')
+        )

The removal of drupal_render() and conversion to a render array look good here.

Minor coding standards nitpicks:

The 'class' line should be indented by 2 spaces (http://drupal.org/coding-standards#indenting), and these array items need a trailing comma per http://drupal.org/coding-standards#array.

+++ b/core/modules/system/templates/exposed-filters.html.twigundefined
@@ -0,0 +1,20 @@
+ * Available variables:
+ * - form: An associative array containing the structure of the form.
+ * - items: An array of current filters.
+ * @see template_preprocess()
+ * @see template_preprocess_exposed_filters()

This needs a blank line with only * between the variables and the two @see lines.

We are also trying to avoid using the word 'array' in template files, if you can reword these to whatever you think makes sense that would be great. If you're not sure just ping me :)

@shanethehat - if you want to go through the preprocess function docblocks and Twig template docblocks updated/added in this patch and rework them as best as you can to match the standards that would be a huge help.

Relevant links for reference:
#1913208: [policy] Standardize template preprocess function documentation
Twig coding standards

shanethehat’s picture

Assigned: Unassigned » shanethehat
shanethehat’s picture

Assigned: shanethehat » Unassigned
Status: Needs work » Needs review
FileSize
21.68 KB
62.94 KB
shanethehat’s picture

Stupid whitespace fail

shanethehat’s picture

Issue summary: View changes

Update commit message

star-szr’s picture

Issue summary: View changes

Update conversion table

c4rl’s picture

Title: Convert form.inc to Twig » form.inc - Convert theme_ functions to Twig

Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.

intergalactic overlords’s picture

Issue tags: -Novice, -Needs manual testing, -Twig

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-54.patch, failed testing.

Hanpersand’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Twig

The last submitted patch, drupal-twig-forminc--1898480-54.patch, failed testing.

Hanpersand’s picture

Assigned: Unassigned » Hanpersand

Going to re-roll this.

Hanpersand’s picture

Assigned: Hanpersand » Unassigned
Status: Needs work » Needs review
FileSize
62.67 KB

Re-rolled!

socketwench’s picture

Status: Needs review » Reviewed & tested by the community

Works for me! RTBC!

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice, -Needs manual testing, -Twig

The last submitted patch, drupal-twig-forminc--1898480-61.patch, failed testing.

Hanpersand’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +Twig

The last submitted patch, drupal-twig-forminc--1898480-61.patch, failed testing.

Hanpersand’s picture

This passed for me and @socketwench at the DrupalCon PDX sprint in both our local testing environments, but the bot failed it on BlockRenderOrderTest.php.

star-szr’s picture

Thanks @hanpersand and @socketwench :) See #1987952-15: Blocks are not rendered in order by weight for an explanation of the test failures, unrelated.

star-szr’s picture

Status: Needs work » Needs review
star-szr’s picture

BTW, found the other issue by running git log -S BlockRenderOrderTest after I couldn't find the test locally.

joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +needs profiling

Reviewing the code we have a few improvements to make because of other issues related to performance. See below:

+++ b/core/includes/form.inc
@@ -4112,37 +4111,21 @@ function form_process_autocomplete($element, &$form_state) {
-  return '<input' . $attributes . ' />' . drupal_render_children($element);
+function template_preprocess_input(&$variables) {
+  $variables['attributes'] = new Attribute($variables['element']['#attributes']);

All of the
+ $variables['attributes'] = new Attribute($variables['element']['#attributes']);

in this patch should be replaced with
+ $variables['attributes'] = $variables['element']['#attributes'];

due to #1982024: Lazy-load Attribute objects later in the rendering process only if needed

+++ b/core/includes/form.inc
@@ -4112,37 +4111,21 @@ function form_process_autocomplete($element, &$form_state) {
+  $variables['attributes'] = new Attribute($variables['element']['#attributes']);
+  $variables['children'] = drupal_render_children($variables['element']);

The drupal_render_children() is no longer needed due to #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead

dsdeiz’s picture

Status: Needs review » Needs work
FileSize
4.92 KB
62.36 KB

Per #70, I've removed new Attribute(...) from all 'attributes' and 'title_attributes' (didn't find 'content_attributes' though). Also removed drupal_render_children() but not entirely sure if I got it right.

dsdeiz’s picture

Status: Needs work » Needs review

The last submitted patch, drupal-twig-forminc--1898480-71.patch, failed testing.

joelpittet’s picture

Here's a few changes on the last patch to help it pass and a bit more nitpiks.

@dsdeiz thank you vrery much for the patch, if you want some details on what's going on there jump on IRC and ping me in #drupal-twig.

+++ b/core/includes/form.inc
@@ -2870,11 +2870,11 @@ function template_preprocess_fieldset(&$variables) {
     $variables['title'] = $element['#title'];
-    $variables['title_attributes'] = new Attribute(array());
+    $variables['title_attributes'] = array();
     if (isset($element['#title_display']) && $element['#title_display'] == 'invisible') {

Doesn't need an empty array, if it's not set it won't get instantiated later, which is good:)

+++ b/core/includes/form.inc
@@ -2966,7 +2966,7 @@ function form_pre_render_radio($element) {
   $element = $variables['element'];
-  $variables['attributes'] = new Attribute(array());
+  $variables['attributes'] = array();

same

+++ b/core/includes/form.inc
@@ -4067,9 +4067,9 @@ function form_pre_render_vertical_tabs($element) {
-  $variables['attributes'] = new Attribute(array(
+  $variables['attributes'] = array(
     'class' => 'vertical-tabs-panes'

needs an ending comma

+++ b/core/includes/form.inc
@@ -4121,8 +4121,7 @@ function form_process_autocomplete($element, &$form_state) {
-  $variables['attributes'] = new Attribute($variables['element']['#attributes']);
-  $variables['children'] = drupal_render_children($variables['element']);
+  $variables['attributes'] = $variables['element']['#attributes'];
 }

Either $variables['children'] = drupal_render_children($variables['element']); needs to be
$variables['children'] = $variables['element'];

Or in twig it needs to be {{ children }}

+++ b/core/includes/form.inc
@@ -4761,7 +4760,7 @@ function template_preprocess_form_element_label(&$variables) {
-    $variables['attributes'] = new Attribute(array());
+    $variables['attributes'] = array();
     // Associate the label with the field it is for.
     if (!empty($element['#id'])) {

No need to initialize the array.

joelpittet’s picture

Issue tags: +Needs reroll

#71 needs a re-roll and #74 changes

hussainweb’s picture

Assigned: Unassigned » hussainweb

Working on the changes...

hussainweb’s picture

Status: Needs work » Needs review
FileSize
62.1 KB

Attaching the rerolled patch.

@joelpittet, in #74, I assume you meant that the twig file (input.html.twig) should contain {{ element }} instead of {{ children }}. The new relevant portion of code in input.html.twig is:

<input{{ attributes }}/>
{% if element is not empty %}
  {{ element }}
{% endif %}

Status: Needs review » Needs work
Issue tags: -Novice, -Needs manual testing, -needs profiling, -Twig, -Needs reroll

The last submitted patch, drupal-twig-forminc--1898480-77.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +Needs manual testing, +needs profiling, +Twig, +Needs reroll

The last submitted patch, drupal-twig-forminc--1898480-77.patch, failed testing.

hussainweb’s picture

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

Fixing the issue with installation. I also tested manually that the installation works. The problem was with a lingering call to get_t().

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-81.patch, failed testing.

hussainweb’s picture

I just tested for the failing test locally and it passed. I even manually verified the count of disabled elements. Queueing for retest.

UPDATE: I mixed up the testing instance. I will try manual testing again...

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs manual testing, -needs profiling, -Twig

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-81.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
62.03 KB

I am just rerolling the patch, also taking in account #2031341: Move theme_container into theme.inc, which moved one of the functions to core/includes/theme.inc. I just want to see what testbots think.

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-86.patch, failed testing.

hussainweb’s picture

Ok, it seems to be the same failure. The problem is with this test:

Drupal\system\Tests\Form\FormTest->testDisabledElements():521

The test expects to find exactly 39 disabled elements. However, with the change in the patch, there are 41. The two extra disabled elements are <div> wrappers to checkboxes and radios elements.

The reason is this twig file (it is same for both radios.twig.html and checkboxes.twig.html:

<div{{ attributes }}>{{ children }}</div>

This brings the attribute #disabled to the wrapping <div> element. This increases the total count of disabled elements.

These are the possible fixes:

  1. Update the test to expect 41 disabled elements, as I don't think
    makes any difference (as the children are disabled anyway). This might affect JavaScript though.
  2. Update the test to not read disabled on div tags. This has the same problem has above.
  3. Explicitly unset #disabled in template_preprocess_radios and template_preprocess_checkboxes.
  4. I will try coming up with a patch for the third method, but I am not sure what could be other effects. Any suggestions?

hussainweb’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -needs profiling, -Twig
FileSize
788 bytes
62.23 KB

Here is the fix as per approach 3 from #88. I did some manual testing and it looks fine. Let's see the testbot's findings.

jenlampton’s picture

Issue tags: +Novice, +needs profiling, +Twig

Can we get some results from the manual testing posted here? Screen shots or copy/paste of the source code.
Also, I think dreditor removed some tags.

star-szr’s picture

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

Needs another reroll. Thanks for your work on this issue @hussainweb!

hussainweb’s picture

Issue tags: -Needs reroll
FileSize
62.27 KB

Thanks for the mention!

Here is the reroll. Fortunately, git did the whole job. I didn't have to manually resolve anything.

hussainweb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-twig-forminc--1898480-92.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
12.07 KB
60.89 KB

@hussainweb thanks for the re-roll. I think the fail is whitespace related.
I cleaned up some nitpicky items, and hopefully got the profiling attributes in a better place for form_element.

FYI, for the whitespace I added a comment control to the end to remove the newline character that EOF wants. Any control will do, I just added a comment because it should be the least process intensive.

thedavidmeister’s picture

Status: Needs review » Needs work

there's still a reference to:

- theme_fieldset() in core/modules/system/tests/modules/design_test/form/fieldset.inc
- theme_details() in core/modules/system/tests/modules/design_test/form/details.inc
- theme_tableselect() in core/includes/form.inc
- theme_form_element() in core/includes/form.inc and core/modules/system/system.api.php
- theme_form_element_label() in core/modules/system/system.api.php

Preapres an element's children details for vertical tabs templates.

Prepares.

these still have @themeable on it, but shouldn't.

- template_preprocess_container()
- template_preprocess_form()

why does vertical-tabs.html.twig not have the same @todo as radios and checkboxes?

$variables['value'] = check_plain($element['#value']);

should be String::checkPlain()

+ $variables['label'] = (isset($element['#title'])) ? filter_xss_admin($element['#title']) : NULL;

should be Xss::filterAdmin();

hussainweb’s picture

Status: Needs work » Needs review
FileSize
5.4 KB
63.55 KB

Attaching changes from the review...

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -needs profiling, -Twig

The last submitted patch, form-twig-1898480-97.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review

#97: form-twig-1898480-97.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +needs profiling, +Twig

The last submitted patch, form-twig-1898480-97.patch, failed testing.

hussainweb’s picture

The test has broken due to swapped order of HTML attributes. The test is expecting:

<label class="visually-hidden" for="edit-langcodes-de">Update German</label>

but it is generated as:

<label for="edit-langcodes-de" class="visually-hidden">Update German</label>

They are the same thing and I can as well update the test case. It was bound to happen as the label template has changed. I was wondering if there was a better way to test for this so that this is future-proof. Any thoughts?

star-szr’s picture

@hussainweb - in some cases we've converted tests to use XPath instead of string matching, I can't find an example offhand but that might be reasonable to do here.

Thanks for your work on this issue :)

hussainweb’s picture

@Cottser, I was thinking along those lines. I just couldn't think of an example. I will look again tomorrow, if possible, but if you find a sample, please let me know.

star-szr’s picture

If you grep for '$this->xpath' that should find you many examples :)

API docs here: https://api.drupal.org/api/drupal/core%21modules%21simpletest%21lib%21Dr...

joelpittet’s picture

Assigned: hussainweb » Unassigned
Status: Needs work » Needs review
FileSize
1.39 KB
63.82 KB

This could use manual testing if it passes.

@hussainweb have a look at the test, change in the interdiff, it's quite handy to be able to use xpath to find tags.

joelpittet’s picture

Component: other » forms system
Status: Needs review » Needs work
  1. +++ b/core/includes/install.core.inc
    @@ -14,6 +14,7 @@
    +use Symfony\Component\DependencyInjection\Definition;
    

    I think this got added in by accident.

  2. +++ b/core/modules/system/templates/container.html.twig
    @@ -0,0 +1,15 @@
    +<div{{ attributes }}>{{ children }}</div>{# Comment removes trailing newline #}
    

    Change the test, add \n

  3. +++ b/core/modules/system/templates/form-required-marker.html.twig
    @@ -0,0 +1,14 @@
    +<abbr{{ attributes }}>*</abbr>{# Comment removes trailing newline #}
    

    change the test.

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

    Let's get this todo out and move this to 'container__radios'

  5. +++ b/core/modules/system/templates/textarea.html.twig
    @@ -0,0 +1,17 @@
    +</div>{# Comment removes trailing newline #}
    

    We should change the test to add the whitespace on here.

  6. +++ b/core/modules/system/templates/vertical-tabs.html.twig
    @@ -0,0 +1,16 @@
    + * @see template_preprocess_vertical_tabs()
    ...
    +<div{{ attributes }}>{{ children }}</div>
    

    Can we consolidate this and remove the theme function for 'container__vertical_tabs'?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
65.19 KB
3.79 KB

from #106 I did item 1, 2, 3 and 5. And left the container__ bit alone because I didn't get very far with those conversions. Anybody is welcome to tackle those.

joelpittet’s picture

Status: Needs review » Needs work

Another review. Has the remaining items from #106

  1. +++ b/core/includes/form.inc
    @@ -4803,44 +4763,72 @@ function theme_form_required_marker($variables) {
    +  // @todo: Figure out how to do this differently (calling from form_element).
    

    Is this possible to fix?

  2. +++ b/core/includes/form.inc
    @@ -4803,44 +4763,72 @@ function theme_form_required_marker($variables) {
    +      // @todo: Use a more semantic class name like 'layout-inline-after'?
    

    Should this be a follow up issue?

  3. +++ b/core/modules/system/templates/checkboxes.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
    +#}
    +<div{{ attributes }}>{{ children }}</div>
    

    #theme => container__checkboxes?

  4. +++ b/core/modules/system/templates/radios.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
    +#}
    +<div{{ attributes }}>{{ children }}</div>
    

    #theme => container__radios?

  5. +++ b/core/modules/system/templates/vertical-tabs.html.twig
    @@ -0,0 +1,16 @@
    + @todo: remove this file once http://drupal.org/node/1819284 is resolved.
    + This is identical to core/modules/system/templates/container.html.twig
    +#}
    +<div{{ attributes }}>{{ children }}</div>
    

    #theme => container__vertical_tabs?

joelpittet’s picture

Issue summary: View changes

Update remaining, remove sandbox link

joelpittet’s picture

Status: Needs work » Needs review

107: 1898480-107-twig-form.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 107: 1898480-107-twig-form.patch, failed testing.

star-szr’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
65.26 KB

Just a reroll and incorporated a change from #2089631: Showing/hiding of CKEditor plugin settings is fragile, automate it (breaks in narrow viewports and when enabling CKEditor) for vertical tabs so they can have attributes passed in.

Probably a terrible idea but I'm seeing what happens when you hide all the patches but the latest from the issue summary.

Status: Needs review » Needs work

The last submitted patch, 111: 1898480-111.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing, -needs profiling
FileSize
63.28 KB

Here's a reroll in preparation for splitting this into sub-issues to make this more manageable.

Mostly an update for #2121775: Make the markup associated with the required star on field items silent and #2087239: Remove theme_exposed_filters().

star-szr’s picture

FileSize
64.08 KB
825 bytes

Adding back the change to \Drupal\system\Tests\Form\FormTest::testRequiredFields().

star-szr’s picture

The last submitted patch, 113: 1898480-113.patch, failed testing.

The last submitted patch, 114: 1898480-114.patch, failed testing.

star-szr’s picture

Title: form.inc - Convert theme_ functions to Twig » [meta] form.inc - Convert theme_ functions to Twig
Issue summary: View changes
Status: Needs review » Active

Converting to a meta.

joelpittet’s picture

.

joelpittet’s picture

Issue summary: View changes

Shuffling.

star-szr’s picture

Super awesome, only #2152227: Convert theme_tableselect() to #theme table__tableselect left now! Meta-ing this really helped I think :)

star-szr’s picture

Status: Active » Fixed

All child issues now fixed, go team! :)

Status: Fixed » Closed (fixed)

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