Task

Convert PHPTemplate templates to Twig templates.

Remaining

  • theme_ function -> Twig conversions (and preprocess function additions or changes) moved over from the existing conversion issue

    specifically:
    Theme function name Conversion status
    theme_field converted in this issue.
    theme_field_multiple_value_form done. converted in this issue
    bartik_field__taxonomy_term_reference converted here instead of in #1938840: bartik.theme - Convert PHPTemplate templates to Twig for dependency reasons
  • Profiling. done in #23 and #41, nothing changed that would require reprofiling

To test this code:
Edit image fields on an article to include multiple images.
create an article
add a tag
add two images
add a body

#1898062: field.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Files: 
CommentFileSizeAuthor
#122 interdiff.txt689 bytesCottser
#122 1987398-122.patch15.95 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,626 pass(es). View
#121 interdiff.txt1.46 KBrteijeiro
#121 drupal-field_module_convert_theme_functions_to_twig-1987398-121.patch15.95 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,457 pass(es). View
#111 interdiff-1987398-106-111.txt1.9 KBmdrummond
#111 drupal-field_module_convert_theme_functions_to_twig-1987398-111.patch15.93 KBmdrummond
PASSED: [[SimpleTest]]: [MySQL] 63,686 pass(es). View
#106 drupal-field_module_convert_theme_functions_to_twig-1987398-106.patch15.93 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 63,060 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Title: field.module - Convert PHPTemplate templates to Twig » field.module - 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 rather than templates. See #1898062: field.module - Convert PHPTemplate templates to Twig for template conversion.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

c4rl’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Status: Active » Needs review
FileSize
13.91 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987398-2.patch. Unable to apply patch. See the log in the details link for more information. View

Here are just the theme_ function conversions. Taking a guess that the fix in #1898062-41: field.module - Convert PHPTemplate templates to Twig is needed here and not there.

Cottser’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Issue summary: View changes

Update info about theme_field

hanpersand’s picture

#2: 1987398-2.patch queued for re-testing.

bonus’s picture

Assigned: Unassigned » bonus

(removed)

lucascaro’s picture

Assigned: Unassigned » bonus

#2: 1987398-2.patch queued for re-testing.

carsonevans’s picture

I am unable to apply this patch to the current 8.x branch. Not profiling.

bonus’s picture

Assigned: bonus » Unassigned
lucascaro’s picture

Assigned: bonus » Unassigned
Status: Needs review » Needs work

confirmed by testbot that this patch fails

gnuget’s picture

Assigned: Unassigned » gnuget
gnuget’s picture

Assigned: gnuget » Unassigned
Status: Needs work » Needs review
FileSize
13.92 KB
PASSED: [[SimpleTest]]: [MySQL] 55,973 pass(es). View

A reroll of #2

joelpittet’s picture

#10 needs manual testing and steps to profile and steps to manual test. Tagging.

Cottser’s picture

Issue tags: +Needs manual testing

Fixing up tags.

thedavidmeister’s picture

Issue tags: +Novice

manual testing is a novice task

adamcowboy’s picture

I manually tested the following 3 things:

A field with single value

A taxonomy term reference field in bartik

A field with two values

adamcowboy’s picture

Status: Needs review » Reviewed & tested by the community

Changing status <3.

adamcowboy’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Lets get some profiling of this change...

jenlampton’s picture

Status: Needs work » Needs review

changing back to NR

jerdavis’s picture

Assigned: Unassigned » jerdavis

I was going to profile this, but it needs a re-roll so I'm re-rolling :)

jerdavis’s picture

FileSize
13.93 KB
FAILED: [[SimpleTest]]: [MySQL] 56,986 pass(es), 2 fail(s), and 0 exception(s). View

Re-roll of patch

Status: Needs review » Needs work

The last submitted patch, 1987398-19.patch, failed testing.

jerdavis’s picture

FileSize
12.84 KB
PASSED: [[SimpleTest]]: [MySQL] 57,492 pass(es). View

Re-roll to remove the following lines from tests

+ $this->installSchema('user', array('role_permission'));

role_permission table was dropped with the commit from https://drupal.org/node/1872876

jerdavis’s picture

Status: Needs work » Needs review
jerdavis’s picture

Assigned: jerdavis » Unassigned
Issue tags: -Needs profiling

Scenario:

- Set image field on Article to multiple value
- Generate 50 articles

=== 8.x..8.x compared (51ec623000e0c..51ec6582d0962):

ct  : 793,342|793,342|0|0.0%
wt  : 2,931,835|2,934,401|2,566|0.1%
cpu : 2,894,807|2,896,008|1,201|0.0%
mu  : 20,495,008|20,495,008|0|0.0%
pmu : 20,729,944|20,729,944|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec623000e0c&...

=== 8.x..1987398-field.module compared (51ec623000e0c..51ec66a5a877d):

ct  : 793,342|797,488|4,146|0.5%
wt  : 2,931,835|2,934,185|2,350|0.1%
cpu : 2,894,807|2,898,250|3,443|0.1%
mu  : 20,495,008|20,574,496|79,488|0.4%
pmu : 20,729,944|20,802,744|72,800|0.4%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51ec623000e0c&...

Cottser’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
12.79 KB
PASSED: [[SimpleTest]]: [MySQL] 57,937 pass(es). View

Re-rolled

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

The last submitted patch, 1987398-25-twig-theme-field.patch, failed testing.

joelpittet’s picture

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

#25: 1987398-25-twig-theme-field.patch queued for re-testing.

joelpittet’s picture

Status: Needs review » Needs work
  1. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
    @@ -0,0 +1,32 @@
    + @todo Remove even/odd striping once http://drupal.org/node/1649780 lands.
    

    We can't have a @todo in here, if this is going to land then we should get that one going, but in the meantime I think we just remove the @todo.

  2. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
    @@ -0,0 +1,32 @@
    +      {# @todo remove type casting once http://drupal.org/node/1970960 lands #}
    

    This @todo and it's related variable to string cast ('' ~) should no longer be needed due to #1975462: Allow and test for NULL and integer 0 values in Twig templates.

adsw12’s picture

Assigned: Unassigned » adsw12

Working on it.

adsw12’s picture

Working on it.

adsw12’s picture

Status: Needs work » Needs review
FileSize
862 bytes
12.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,648 pass(es). View

Re-relloded the patch addressed all the comments from @joelpittet #28

adsw12’s picture

Assigned: adsw12 » Unassigned
FileSize
666 bytes
12.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,854 pass(es). View

Removed string cast ('' ~) #25

joelpittet’s picture

Thanks @adsw12 and nice to meet you at drupalcon prague.

This issue is ready for manual testing and that should be it for this one:)

Cottser’s picture

Status: Needs review » Needs work

If we are removing theme_field() then we should also update field.html.twig to remove the HTML comment:

<!--
THIS FILE IS NOT USED AND IS HERE AS A STARTING POINT FOR CUSTOMIZATION ONLY.
See http://api.drupal.org/api/function/theme_field/8 for details.
After copying this file to your theme's folder and customizing it, remove this
HTML comment.
-->

The profiling results actually look reasonable to do this from what I can see!

joelpittet’s picture

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,39 @@
+ * @see template_process_field()

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -0,0 +1,30 @@
+ * @see template_process_field()

These don't exist either.

dsayswhat’s picture

FileSize
12.63 KB
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es). View

Re-roll of #32.

dsayswhat’s picture

FileSize
10.75 KB
FAILED: [[SimpleTest]]: [MySQL] 51,392 pass(es), 2,633 fail(s), and 228 exception(s). View

Edited documentation to address #34 and #35.

dsayswhat’s picture

Status: Needs work » Needs review

Setting to needs review

joelpittet’s picture

Assigned: Unassigned » joelpittet
Status: Needs review » Needs work
Issue tags: +Needs profiling

Just going to check that I get the same results as #23. Crossing fingers because that would be great:)

Cottser’s picture

Awesome @dsayswhat, thanks! An interdiff is always good to include when updating patches - in this case we lost a couple files between #36 and #37:

core/modules/field/templates/field-multiple-value-form.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue tags: -Needs profiling

Ok bench is the same scenario as #23 and I used the patch from #36.

Looks great still. Just need #37 fixed up and provide an interdiff.

=== 8.x..8.x compared (5251f592ab279..5251f691abb85):

ct  : 182,060|182,060|0|0.0%
wt  : 725,809|727,727|1,918|0.3%
cpu : 665,838|675,383|9,545|1.4%
mu  : 20,484,360|20,484,360|0|0.0%
pmu : 20,716,112|20,716,112|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5251f592ab279&...

=== 8.x..1987398-twig-field compared (5251f592ab279..5251f6e1e32d5):

ct  : 182,060|185,223|3,163|1.7%
wt  : 725,809|727,234|1,425|0.2%
cpu : 665,838|667,528|1,690|0.3%
mu  : 20,484,360|20,585,640|101,280|0.5%
pmu : 20,716,112|20,853,792|137,680|0.7%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5251f592ab279&...

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman
Status: Needs review » Needs work
FileSize
3.91 KB
13.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,284 pass(es). View

I took the missing files:

core/modules/field/templates/field-multiple-value-form.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

from the patch on #36 and added them to the patch on #37.

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Status: Needs work » Needs review

errrgh

Cottser’s picture

Assigned: Tim Bozeman » Unassigned

Nice @Tim Bozeman! Let's also remove the @see template_process_field() lines mentioned by @joelpittet in #35.

Tim Bozeman’s picture

Assigned: Unassigned » Tim Bozeman

Okay

Tim Bozeman’s picture

FileSize
3.39 KB
14.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,971 pass(es). View

A few changes to the #43 patch.

As requested by @Cottser in #45:
Removed the @see template_process_field() lines mentioned by @joelpittet in #35.

Also, @Cottser mentioned in #34 that theme_field() is gone so I removed a line in the comment of field.html.twig and a sentence from a comment on line 694 of field.module that referenced theme_field().

Tim Bozeman’s picture

Status: Needs work » Needs review

errrgh

Cottser’s picture

Status: Needs review » Needs work

I think we should change the reference in field.module to theme_field() to field.html.twig instead of removing it.

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
0 bytes
9.67 KB
FAILED: [[SimpleTest]]: [MySQL] 54,280 pass(es), 1,273 fail(s), and 170 exception(s). View

Third times the charm!

Tim Bozeman’s picture

FileSize
8.07 KB

errrgh

Status: Needs review » Needs work

The last submitted patch, drupal8.field-system.1987398-50.patch, failed testing.

Tim Bozeman’s picture

Status: Needs work » Needs review
FileSize
2.61 KB
14.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,489 pass(es). View

Sorry for the spam! : /

joelpittet’s picture

@Tim Bozeman now worries for the spam, thank you very much for doing those changes:)

There has been manual testing in #14 but we should probably do another round just to be safe we didn't make any mistakes. @Tim Bozeman you're welcome to give that a try too or someone else can pick this issue up from here.

Tim Bozeman’s picture

Assigned: Tim Bozeman » Unassigned
Tim Bozeman’s picture

Issue summary: View changes

Added to test section :).

YesCT’s picture

Issue summary: View changes

trying to clarify and update cause refereneced issues are closed and such

Tim Bozeman’s picture

FileSize
14.01 KB
PASSED: [[SimpleTest]]: [MySQL] 59,520 pass(es). View

#53 was stale, rerolled.

markcarver’s picture

@joelpittet, could you double check this this? Thanks!

joelpittet’s picture

Assigned: Unassigned » joelpittet

Looking:)

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing
FileSize
40.61 KB
38.98 KB

Reviewed the markup from #56

The following templates were rendered onto the article page I tested:

core/modules/field/templates/field.html.twig
core/modules/field/templates/field-multiple-value-form.html.twig
core/themes/bartik/templates/field--taxonomy-term-reference.html.twig

Other than whitepace differences there was issues.
The two files uploaded were used for comparison, the modifications were to the hash keys and whitespace to make the diffs easier to parse.

Thanks @Tim Bozeman for finishing that up.
Profiling done in #41 is still good, no changes to speak of that would make a difference between #36 and #56

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

Nigel_S’s picture

Assigned: Unassigned » Nigel_S
Nigel_S’s picture

FileSize
13.86 KB
FAILED: [[SimpleTest]]: [MySQL] 59,720 pass(es), 164 fail(s), and 47 exception(s). View

Changes to the FieldDefinitionInterface and the placing of '#cardinality_multiple' as a key in $variables broke the old patch. The patch also had snippets of $output being constructed so I've stripped those too.

Nigel_S’s picture

Status: Needs work » Needs review

The last submitted patch, drupal8.field-system.1987398-62.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

theme_field() hasn't been removed (yet)

Nigel_S’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
13.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.field-system.1987398-65_0.patch. Unable to apply patch. See the log in the details link for more information. View

Missed out a line from #62, oops. Between the patch on 2013-10-22 and now CARDINALITY_UNLIMITED was moved to the FieldDefinitionInterface and a '#cardinality_multiple' key has been added to $element alongside cardinality.

Status: Needs review » Needs work

The last submitted patch, 65: drupal8.field-system.1987398-65.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 65: drupal8.field-system.1987398-65.patch, failed testing.

Nigel_S’s picture

Status: Needs work » Needs review
FileSize
15.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,987 pass(es). View

Since the last reroll the format of the taxonomy field has changed within Bartik - introduced bartik_preprocess_field to conditionally apply .visually-hidden - this lets screen readers still read the label even when it's hidden.

Status: Needs review » Needs work

The last submitted patch, 70: drupal8.field-system.1987398-70.patch, failed testing.

Nigel_S’s picture

Status: Needs work » Needs review
Nigel_S’s picture

FileSize
15.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,088 pass(es). View

Same patch that passed testing above (first fail was due to broken HEAD) but with some comment errors corrected.

joelpittet’s picture

@Nigel_S thanks for the patches, great to see them green. Here's a little nit-pick doc change as well. And I think this is ready for manual testing again.

+++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
@@ -0,0 +1,28 @@
+ * - title_attributes: HTML attributes for the title.

This is not being used. we should remove it.

Nigel_S’s picture

FileSize
15.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,089 pass(es). View

@joelpittet - thanks :) have removed that line of the comment - everything else unchanged

Cottser’s picture

Interdiff please! Thanks @Nigel_S :)

Cottser’s picture

double post.

Status: Needs review » Needs work

The last submitted patch, 75: drupal8.field-system.1987398-75.patch, failed testing.

Nigel_S’s picture

Status: Needs work » Needs review
Nigel_S’s picture

FileSize
871 bytes

Oh alright :P apparently I also killed some whitespace

The last submitted patch, 75: drupal8.field-system.1987398-75.patch, failed testing.

Nigel_S’s picture

The last submitted patch, 75: drupal8.field-system.1987398-75.patch, failed testing.

Nigel_S’s picture

Sorry for the re-test spam; it seems testbots are dying due to out-of-memory errors - will hold off on re-testing again until https://drupal.org/node/2141501 is dealt with.

Nigel_S’s picture

Jeff Burnz’s picture

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,38 @@
+{% if multiple_cardinality %}

Could we just use "multiple"?

Haven't tested but great patch, looks good.

joelpittet’s picture

@Jeff Burnz that sounds like a slightly easier to understand variable name. Cardinality is a $100 word for a 5cent concept. Let's make that change.

Nigel_S’s picture

FileSize
16.02 KB
PASSED: [[SimpleTest]]: [MySQL] 58,968 pass(es). View
2.65 KB

OK :) These changes switch the variable to 'multiple' - I also noticed when looking at it that check_plain is now deprecated so I've switched template_preprocess_field to a direct call to String::checkPlain.

Cottser’s picture

Status: Needs review » Needs work

Thanks for your continued work on this issue @Nigel_S! Here are some things that can be fixed up:

  1. +++ b/core/modules/field/templates/field-multiple-value-form.html.twig
    @@ -0,0 +1,38 @@
    + * @see template_preprocess_field()
    

    @see template_preprocess_field_multiple_value_form()

  2. +++ b/core/themes/bartik/bartik.theme
    @@ -173,25 +174,19 @@ function bartik_menu_tree__shortcut_default($variables) {
    + * Implements theme_preprocess_HOOK() for field templates
    + * @see template_preprocess_field()
    + * @todo Use hook_theme_prepare_theme_hook when issue is resolved https://drupal.org/node/2035055
      */
    

    This docblock needs some more spacing out (blank line after the summary line and around the @see), and the summary line needs to end in a period per http://drupal.org/node/1354#drupal.

  3. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
    @@ -0,0 +1,27 @@
    + * @see template_preprocess_field()
    + * @see bartik_preprocess_field()
    + * @ingroup themeable
    

    Blank line before @ingroup line please, similar to the above.

  4. +++ b/core/themes/bartik/templates/field--taxonomy-term-reference.html.twig
    @@ -0,0 +1,27 @@
    +  <h3 {{ label_attributes }}>{{ label }}: </h3>
    

    Extra space in here, should be <h3{{ label_attributes}}>. The attribute object will print out the leading space if there are any attributes, otherwise you get a nice clean <h3> :)

Nigel_S’s picture

FileSize
16.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.field-system.1987398-89.patch. Unable to apply patch. See the log in the details link for more information. View
1.74 KB

OK, done :)

Nigel_S’s picture

Status: Needs work » Needs review
joelpittet’s picture

Issue tags: +Needs reroll

@Nigel_S wanna grab the re-roll on this one?

joelpittet’s picture

The last submitted patch, 89: drupal8.field-system.1987398-89.patch, failed testing.

Nigel_S’s picture

Yeah sure, I shall take a look at it

Nigel_S’s picture

FileSize
16.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,240 pass(es). View
915 bytes

Re-rolling

Cottser’s picture

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

Thanks @Nigel_S!

Here's another review :)

  1. +++ b/core/modules/field/field.module
    @@ -535,14 +539,10 @@ function field_theme_suggestions_field(array $variables) {
    +  // Always set the field label - allow themes to decide whether to display
    +  // In addition the label should be rendered but hidden to support screen readers
    +  $variables['label'] = String::checkPlain($element['#title']);
    

    These comments should be complete sentences and the second line is a couple characters over 80.

  2. +++ b/core/themes/bartik/bartik.theme
    @@ -173,25 +174,21 @@ function bartik_menu_tree__shortcut_default($variables) {
    + * @todo Use hook_theme_prepare_theme_hook when issue is resolved https://drupal.org/node/2035055
    

    This could be more specific/instructive and say "Use bartik_theme_prepare_field__taxonomy_term_reference when https://drupal.org/node/2035055 is resolved".

  3. +++ b/core/themes/bartik/bartik.theme
    @@ -173,25 +174,21 @@ function bartik_menu_tree__shortcut_default($variables) {
    +function bartik_preprocess_field(&$variables) {
    +    $element = $variables['element'];
    +    if ($element['#field_type'] == 'taxonomy_term_reference') {
    +        $label_attributes['class'] = array('field-label');
     
    -  // Render the top-level DIV.
    -  $variables['attributes']['class'][] = 'clearfix';
    -  $output = '<div ' . $variables['attributes'] . '>' . $output . '</div>';
    +        if ($variables['label_hidden']) {
    +            $label_attributes['class'][] = 'visually-hidden';
    +        }
     
    -  return $output;
    +        $variables['label_attributes'] = new Attribute($label_attributes);
    +    }
    

    The indenting is off in the body of bartik_preprocess_field(), it mostly uses 4 spaces instead of 2.

Cottser’s picture

Assigned: Nigel_S » Unassigned

This also needs a reroll, unassigning since it's been a while. Once rerolled we can work on the tweaks mentioned in #96.

@Nigel_S, if you want to work on the reroll and tweaks just reassign :) thanks!

InternetDevels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.97 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1987398-95-98.patch. Unable to apply patch. See the log in the details link for more information. View
16.15 KB
FAILED: [[SimpleTest]]: [MySQL] 59,755 pass(es), 191 fail(s), and 155 exception(s). View

Re-rolling end fix with respect to comment #96

The last submitted patch, 98: interdiff-1987398-95-98.patch, failed testing.

Status: Needs review » Needs work
InternetDevels’s picture

Status: Needs work » Needs review
FileSize
16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-field_module_convert_theme_functions_to_twig-1987398-101.patch. Unable to apply patch. See the log in the details link for more information. View

The reason for the failure became #732022: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached after the patch which drupal_add_tabledrab function was not valid. In this patch fixes bugs and made ​​reroll.

Status: Needs review » Needs work
asenenko’s picture

Status: Needs work » Needs review
FileSize
15.93 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-field_module_convert_theme_functions_to_twig-1987398-103.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll

ipo4ka704’s picture

Status: Needs review » Needs work
InternetDevels’s picture

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

#103 was stale, rerolled.

Status: Needs review » Needs work
maggo’s picture

Status: Needs work » Needs review
joelpittet’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

Couple little things and I'll do another round of manual testing.

  1. +++ b/core/modules/field/field.form.inc
    @@ -69,8 +84,8 @@ function theme_field_multiple_value_form($variables) {
    +      '#theme' => 'table',
    

    This should say #type=>table. Just realized that. It may need to be re-worked to be a proper type table without #rows, but I'll leave that out of this conversion.

  2. +++ b/core/themes/bartik/bartik.theme
    @@ -175,25 +176,20 @@ function bartik_menu_tree__shortcut_default($variables) {
    + * @todo Use bartik_theme_prepare_field__taxonomy_term_reference when
    + * https://drupal.org/node/2035055 is resolved
    

    Can you remove this @todo. I'd rather not have that in with core.

Cottser’s picture

mdrummond’s picture

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

Here's a revised patch with the suggested changes. I also fixed a deprecated function, element_children. Sorry if that's out of scope.

joelpittet’s picture

@mdrummond Thanks for the revised patch! I'm fine with those changes, there is probably another issue outside this one dealing with it, which will cause that to be re-rolled or may be closed as duplicate, but same thing for String::checkPlain(). It's either this gets in first or it gets re-rolled.

+++ b/core/modules/field/field.form.inc
@@ -85,19 +101,15 @@ function theme_field_multiple_value_form($variables) {
+    foreach (Element::children($element) as $key) {
+      $variables['elements'][] = $element[$key];
     }

+++ b/core/modules/field/templates/field-multiple-value-form.html.twig
@@ -0,0 +1,38 @@
+  {% for element in elements %}
+    {{ element }}
+  {% endfor %}

Also love this, makes things much more useful in the template. No drupal_render_children() FTW!

Cottser’s picture

thedavidmeister’s picture

Status: Needs review » Needs work
Issue tags: -Core Review Bonus
+  if ($variables['multiple']) {
 
-  if ($element['#cardinality_multiple']) {
-    $form_required_marker = array('#theme' => 'form_required_marker');

This extra space below the "if" makes me feel weird in an OCD way. Can we remove it?

-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => $element['#title'],
+          ),
+          '#suffix' => '</h4>',
+        ),

Is there a reason we no longer run #title through t()? I can see why required is not, as it's just a '*' character.

-  $output .= ($variables['element']['#label_display'] == 'inline') ? '<ul class="links inline">' : '<ul class="links">';

This looks like the conditional "inline" class would be broken now in the bartik template.

#2094585: [meta] [experimental] Core review bonus for #2181507: Remove the deprecated usages of element_* methods.

Cottser’s picture

Issue tags: +Needs reroll

Tagging for reroll.

mdrummond’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
16.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-field_module_convert_theme_functions_to_twig-1987398-116.patch. Unable to apply patch. See the log in the details link for more information. View

Here's a reroll that also addresses the concerns raised in #114.

joelpittet’s picture

Status: Needs review » Needs work

Some nit picky notes:

  1. +++ b/core/modules/field/field.form.inc
    @@ -85,19 +99,15 @@ function theme_field_multiple_value_form($variables) {
    +    $variables['description'] = $element['#description'] ? $element['#description'] : '';
    

    This can likely be just
    $variables['description'] = $element['#description'];
    The condition doesn't help the statement any.

  2. +++ b/core/modules/field/field.module
    @@ -7,8 +7,9 @@
    -use Drupal\Core\Template\Attribute;
    ...
    +use Drupal\Core\Template\Attribute;
    +use Drupal\Component\Utility\String;
    

    The Attribute order doesn't need to change here they were in the right order and Component\String should go before Xss.

  3. +++ b/core/modules/field/templates/field-multiple-value-form.html.twig
    @@ -0,0 +1,38 @@
    +    {% spaceless %}
    ...
    +    {% endspaceless %}
    

    Any chance we can do without the spaceless? I'd rather tweak the spaces in the tests if they fail.

LewisNyman’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
LewisNyman’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
15.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,319 pass(es). View

Rerolled and addressed Joel's comments in 117.

rteijeiro’s picture

FileSize
15.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,457 pass(es). View
1.46 KB

I checked @LewisNyman patch and works well. Just fixed some indentation and missing periods.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
Related issues: +#1623574: Remove trailing space from form element labels and field labels (HTML nbsp)
FileSize
15.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,626 pass(es). View
689 bytes

Rolling in a very minor docs fix. I manually tested these templates again and looking good! Performance has been tested a couple times and shown to be quite comparable: #23 and #41. I think it's time for theme_field() to retire :)

+++ b/core/modules/field/templates/field.html.twig
@@ -25,17 +25,10 @@
     <div class="field-label"{{ title_attributes }}>{{ label }}:&nbsp;</div>

When manually testing I noticed that to be consistent with theme_field() output we'd need to remove the :&nbsp; here but I think we should leave that to #1623574: Remove trailing space from form element labels and field labels (HTML nbsp) (or a follow-up issue from it) to clean up. See #1623574-50: Remove trailing space from form element labels and field labels (HTML nbsp) and after.

webchick’s picture

+++ b/core/modules/field/field.form.inc
@@ -7,46 +7,55 @@
-        'data' => '<h4 class="label">' . t('!title !required', array('!title' => $element['#title'], '!required' => $required)) . "</h4>",
+        'data' => array(
+          '#prefix' => '<h4 class="label">',
+          'title' => array(
+            '#markup' => t($element['#title']),
+          ),
+          '#suffix' => '</h4>',
+        ),

This is just reformatting existing code, so not a commit-blocker. But I'm wondering why we don't move that <h4> and other associated markup to field-multiple-value-form.html.twig? Seems unfortunate to have to override a preprocess function in order to get at that, when everything else is so nicely "Twiggy."

webchick’s picture

Status: Reviewed & tested by the community » Fixed

According to Cottser, this is because we're putting things in the format that #type table expects. Hm. Still seems unfortunate, but also seems to be the status quo, so...

Committed and pushed to 8.x. Thanks!

  • Commit bead76a on 8.x by webchick:
    Issue #1987398 by Nigel_S, Tim Bozeman, InternetDevels, adsw12,...
markcarver’s picture

To help further explain: it's used to wrap the title in the table header. I am guessing this was to get a desired font size and indicate some importance for this header. The table is rendered before being sent to the template and only provided as a single {{ table }} variable in the template to be printed. It is next to impossible to inject this small bit of markup in this particular template.

I agree though, using #markup, #prefix and #suffix is definitely not the best approach or Best Practices™ anymore. A better approach would be to simply add a class for that header and then styling it via CSS if it needs to look "bigger" (similar to an h4).

FWIW, this was a "convert" issue. We can create a follow-up "consolidate/cleanup" issue for field.module if it is really warranted.

Cottser’s picture

Status: Fixed » Closed (fixed)

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