Problem/Motivation

The Drupal field template is quite verbose and can be hard to read. We start off checking for the label, then put in a conditional for if there are more than 1 field item (whether it's a multivalue field or not), then output certain divs depending on if the field has multiple values, then give an else version if not, and back to checking again about that label ... it gets tiresome.

Steps to reproduce

Open field.html.twig and see for yourself

Proposed resolution

Wouldn't it be great if we could simplify this to:

Be easier to read

  • Out the same markup, whether there is one item in my field or multiple (so we don't have different CSS/JS depending on how many values we output)
  • Have a conditional for styling differently if there is multiple (but this place as a modifier on the .field class, not as an additional .field__items class

Remaining tasks

Rewrite the field.html.twig template

User interface changes

None

API changes

None - my patch is against the starterkit_theme, but I'd like if we could (in D10) have this approach in our other themes too.

Data model changes

None

Release notes snippet

We have simplified the field.html.twig template. The only breaking change here is that we have a .field__items class wrapping the .field__item in all cases now, whether the field outputs multiple items or not. This is to make sure you can write the same CSS for single and multiple items. If you need to style something differently because it has multiple items, there is a .field--has-multiple-items modifier class at the .field level

This is based on a blog post I wrote called Simplified Drupal Field Template (but the same markup rendered)

CommentFileSizeAuthor
#2 simplify-field-template-3308309-2.patch1.52 KBmarkconroy

Issue fork drupal-3308309

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

markconroy created an issue. See original summary.

markconroy’s picture

markconroy commented
Status: Active » Needs review
StatusFileSize
new simplify-field-template-3308309-2.patch1.52 KB

Adding patch.

markconroy’s picture

markconroy commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 2: simplify-field-template-3308309-2.patch, failed testing. View results

markconroy’s picture

markconroy commented
Status: Needs work » Needs review

Setting back to needs review for the tests to run again.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

FWIW, the test failures look legit and we should probably address those.

markconroy’s picture

markconroy commented

@lauriii Can I presume those test failures are not because of my patch? I can't see any thing in it that would cause those failures, or is there something else you think I need to do to get the tests passing?

chi’s picture

chi commented 
{% if multiple %}
  <div class="field__items">
{% endif %}

That's not specific to this issue but I'd like to indicate that IDEs have problems with syntax highlighting of such code.
Also having non-closed tags in the codition blocks makes it hurder to read and understantd even though it seems less verbose.

markconroy’s picture

markconroy commented

I think the tests might be failing because our mark up for single items is changed now (with forcing the extra .field__items div to always be present), so the test for a single field item can’t check for .field > .field__item it needs to check for .field > .field__items > .field__item

Let's figure it out in Prague next week

sarahjean’s picture

sarahjean commented

I think this is great, I almost always wind up overriding the default field templates to pare back all the extra cases, not this exact way but similar idea.

wesruv’s picture

wesruv commented

Love this for starter kit, think it could be good for classy, but I wouldn't want this for lowest level field template & stable theme.

I'd prefer the default & stable templates prioritizing output over dev experience (without abandoning it).

bserem made their first commit to this issue’s fork.

bserem opened merge request !2786

tonypaulbarker’s picture

tonypaulbarker
he/him
commented

I think we are looking at

Line 52 of testEntityViewController (core/modules/system/tests/src/Functional/Entity/EntityViewControllerTest.php)

and I think we want to achieve something like

<div class="field field--name-name field--type-string field--label-hidden"><div class="field__items"><div class="field__item">' . $label . '</div></div></div>

But, we seem to have some other issues with the tests locally, so going to see how we go and send work in progress.

kostask made their first commit to this issue’s fork.

bserem’s picture

bserem commented
Status: Needs review » Needs work
tonypaulbarker’s picture

tonypaulbarker
he/him
commented
Status: Needs work » Needs review

-> Needs review for running tests

bserem’s picture

bserem commented
markconroy’s picture

markconroy commented

Great work everyone. Thanks for moving this forward.

andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs review » Reviewed & tested by the community

The patched version of the twig is definitely easier to read, and there's no cost or risk associated with making this change in starterkits, but I fear it may be too late to get this change into D10.

The string returned from the test is has some ugly indentation, but it's a test and if it passes then I suppose ¯\_(ツ)_/¯

Pending successful tests, RTBC.

bserem’s picture

bserem commented
Status: Reviewed & tested by the community » Needs work

Several other areas are affected by this, lots of tests to check (or change those tests to use stark)

tonypaulbarker’s picture

tonypaulbarker
he/him
commented

Some context to the tests.

They do not test anything in starterkit. They should be tested against Stark. They were changed to test against starterkit as a quick workaround when other themes were moved out of core. That is not really related to this issue (and no regression is caused) so it should not hold this up, we should open a new issue to tackle running against Stark.

We should only be concerned about coding standards of the indentation. If it passes coding standards, then I think this is good to go. Otherwise the indentation should be the only remaining thing to address.

This should ship with D10, otherwise we could start to run into backward compatibility issues.

Moving to review for check of coding standards of the indentation.

tonypaulbarker’s picture

tonypaulbarker
he/him
commented
Status: Needs work » Needs review
andy-blum’s picture

andy-blum
he/him/his
Primary language English
Location Ohio, USA
commented
Status: Needs review » Needs work

Tests are failing.

tonypaulbarker’s picture

tonypaulbarker
he/him
commented

Sorry @andy-blum you're quite right - we've more tests to work through.

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

kostask’s picture

kostask commented
Issue tags: +GreeceSpringSprint2024

kostask opened merge request !7416

kostask opened merge request !7417

kostask closed merge request !7416

markconroy’s picture

markconroy commented
markconroy’s picture

markconroy commented

At DrupalCon Prague we decided that field--has-multiple-items and field--has-single-item were not communicating the correct information. We need to communicate the field cardinality in the database. When a user sees has-multiple or has-single they will connect this information what is rendered. A multivalue field can have just one entry, but in the original patch it would get a has-multiple-items class.

Just for clarity, the way field.html.twig currently works is that if it's a multivalue field - even if it only has one item in it - it gets the .field__items wrapper. If it has only one item, that item is wrapped in a div and if it has more than one item it uses ul.

The class we are intending to add here field--multiple will denote that this is a multivalue field, not how many items are in the field.
Perhaps we could add another class field--has-N-items to denote how many items we actually have.

Suggestion for classes:

  'field--name-' ~ field_name|clean_class,
  'field--type-' ~ field_type|clean_class,
  'field--label-' ~ label_display,
  multiple ? 'field--multivalue-field' : 'field--single-value-field',
  multiple ? 'field--items-count' ~ items|length|clean_class : 'field--items-count-1',
  label_display == 'inline' ? 'clearfix',

Note: we will need to update the issue summary with new notes for the change record.

===
Thanks to Code Enigma for sponsoring my time to work on this.

vensires’s picture

vensires
Location Greece
commented
Issue tags: -GreeceSpringSprint2024 +GreeceWinterSprint2024
bserem’s picture

bserem commented
Assigned: Unassigned » bserem

bserem changed the visibility of the branch 3308309-simplify-the-html to hidden.

bserem’s picture

bserem commented

LB related tests fixed, working on CKEditor ones (the ones we haven't solved in years)

In the meanwhile, I don't really like the naming conventions and some of the classes that we are introducing here

- field--multivalue-field -> field--multiple-value-field, or field--multiple which is what already exists in Core
- field--single-value-field -> as is, or field--single which is what already exists Core
- field--has-multiple-items
- field--has-single-item -> field--has-one-item
- field--items-count-X -> is this really needed? Do we care about something like that or do we just pollute the DOM?

bserem’s picture

bserem commented

PS, `field_items` class is introduced to single value fields with the work on the branch. Thinking about it too

bserem’s picture

bserem commented
Status: Needs work » Needs review
bserem’s picture

bserem commented
Assigned: bserem » Unassigned
vensires’s picture

vensires
Location Greece
commented
Issue tags: +Needs Review Queue Initiative

Since it's a long standing issue I also add the "Needs Review Queue Initiative" tag for others to check the solution provided too.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Before reviewing think this needs a rebase, 600+ commits

Will need some screenshots during testing to make sure nothing has broke.

The tests that had to be updated to pass, surprised they are using starterkit_theme maybe follow ups are needed to correct that.

vensires’s picture

vensires
Location Greece
commented

I went on with the rebase of the commits. I think the tests do need a check from someone who knows what he's doing (currently failing) + also check what @smustgrave said about the use of the starterkit_theme.

bserem’s picture

bserem commented

I believe we need to change the target branch to 11.1 or something. The commits are awfully wrong in Gitlab after the rebase.
It's been less than 10 commits, we are seeing things already merged in Core.

vensires’s picture

vensires
Location Greece
commented
Issue tags: -GreeceWinterSprint2024, -Needs Review Queue Initiative +GreeceSprints2025

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.