Updated: Comment #N

Problem/Motivation

#2201693-18: Field output supporting code should move out of field.module effectively reverted most some of #1987398: field.module - Convert theme_ functions to Twig.

Proposed resolution

Let's fix it! Re-do all applicable changes from #1987398: field.module - Convert theme_ functions to Twig to the files in the new locations. For example theme_field() and theme_field_multiple_value_form() should be gone.

Remaining tasks

  1. Create the patch.
  2. Patch needs review.

User interface changes

n/a

API changes

n/a

Files: 
CommentFileSizeAuthor
#22 2226233-22.patch724 bytesandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,880 pass(es). View

Comments

Cottser’s picture

Issue summary: View changes

The changes to field.html.twig at least are intact, so correcting the issue summary.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia
Manuel Garcia’s picture

Status: Active » Needs review
FileSize
916 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,839 pass(es). View

I believe most things were actualy moved into system module.

template_preprocess_field, theme_field, and template_preprocess_field_multiple_value_form was moved into includes/theme.inc
the return array of field_theme was added into drupal_common_theme

field-multiple-value-form.html.twig and field.html.twig was moved into core/system/templates

The only thing I believe stil left to do is that they left field_theme_suggestions_field in field.module, while they created it also on system.module as system_theme_suggestions_field, the attached patch removes it from field.module.

Manuel Garcia’s picture

They also removed css from field.module.css, which was moved into system.theme.css

Status: Needs review » Needs work

The last submitted patch, 3: drupal-2226233-2.patch, failed testing.

alexrayu’s picture

FileSize
916 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,727 pass(es). View

Proposed patch.

alexrayu’s picture

Status: Needs work » Needs review
alexrayu’s picture

Now I see patches are identical except for indices. Re-queuing Manual's original patch for testing.
3: drupal-2226233-2.patch queued for re-testing.

galooph’s picture

Patch in #6 looks ok and applies cleanly.

Cottser’s picture

Status: Needs review » Needs work
FileSize
129.75 KB
41.4 KB

The current patch is duplicating work being done at #2201693: Field output supporting code should move out of field.module and I'd rather leave that cleanup to that issue. I want to focus on re-removing the theme functions and making sure our preprocess changes are maintained. I went through and diffed everything before/after #2201693-18: Field output supporting code should move out of field.module and here's what I found that we need to do in this issue:

  1. Remove theme_field()
  2. Remove theme_field_multiple_value_form()
  3. Reapply a couple changes to template_preprocess_field():

Edited to linkify screenshots.

Cottser’s picture

And thank you @Manuel Garcia, @alexrayu, and @galooph for tackling this one :)

ckrina’s picture

Assigned: Manuel Garcia » ckrina

Starting to work on that.

Manuel Garcia’s picture

That makes total sense Cottser, and thanks ckrina for taking this on!

ckrina’s picture

Assigned: ckrina » Unassigned
FileSize
9.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,819 pass(es). View

Attaching the patch.
Anyway, there were still some references to theme_field() and theme_field_multiple_value_form():

  • On core/modules/field/field.form.inc

    "Callback for usort() within theme_field_multiple_value_form()." > "Callback for usort() within field-multiple-value-form.html.twig."

    I've made the change.
  • On core/modules/node/node.module, line 705. I'm not sure what to do here, just replace theme_field() by field.html.twig ?
    :
    /**
     * Returns HTML for the node title field.
     *
     * This is an override of theme_field() for the node title field. See that
     * function for documentation about its details and overrides.
     *
     * @param array $variables
     *   An associative array. See theme_field() for details.
     *
     * @see theme_field()
     *
     * @ingroup themeable
     */
    function theme_field__node__title($variables) {
      return '<span' . $variables['attributes'] . '>' . drupal_render($variables['items']) . '</span>';
    }
    
galooph’s picture

_field_sort_items_value_helper() is also used for a usort inside theme_file_widget_multiple() in core/modules/file/file.field.inc. Might be worth adding that in the docblock too, eg "Callback for usort() within field-multiple-value-form.html.twig and theme_file_widget_multiple()."

Cottser’s picture

Status: Needs work » Needs review

Don't worry about those - we're tackling any docs followup in #2226863: Update stale references to theme functions that have been converted to Twig :)

Let's see what the bot says. Patch looks good to me though.

galooph’s picture

Thanks @Cottser, I wasn't aware there was a separate cleanup issue.

Patch looks good and applies fine.

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @ckrina :)

The patch does what it's supposed to do, there is the following extra (bonus) change but it's not hurting anyone as far as I can see (and it's a correct change):

+++ b/core/modules/field/field.form.inc
@@ -8,7 +8,7 @@
- * Callback for usort() within theme_field_multiple_value_form().
+ * Callback for usort() within field-multiple-value-form.html.twig.
Cottser’s picture

Issue summary: View changes

Making the problem/motivation less dramatic ;)

  • Commit 5c7c259 on 8.x by webchick:
    Issue #2226233 by ckrina, alexrayu, Manuel Garcia, Cottser: Redo changes...
webchick’s picture

Status: Reviewed & tested by the community » Fixed

$clobbering--;

The only thing that threw me was all those nice docs getting deleted. But basically they're no longer needed, since theming fields now works like theming everything else.

Committed and pushed to 8.x. Thanks!

andypost’s picture

Title: Redo changes from field.module to Twig conversion issue that were clobbered » [follow-up] Redo changes from field.module to Twig conversion issue that were clobbered
Status: Fixed » Needs review
Related issues: +#1962846: Use field instance name for header of comment list, drop comment-wrapper template
FileSize
724 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,880 pass(es). View

Do not mix field name and type on one level of template suggestions.
Faced with that in #1962846-8: Use field instance name for header of comment list, drop comment-wrapper template
That leads to field__comment template could be used for for any field that named "comment" for example custom entity with base field named so

Cottser’s picture

Status: Needs review » Fixed

That looks entirely unrelated, please open a separate issue. Thanks!

andypost’s picture

Title: [follow-up] Redo changes from field.module to Twig conversion issue that were clobbered » Redo changes from field.module to Twig conversion issue that were clobbered

Status: Fixed » Closed (fixed)

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