[3.x] Add optional wrapper around field items and fix class structure

I'd like to second this request. I have a few fields that need to be absolutely positioned on the page (e.g., a field for sidebar content), and having the field label dissociated from the field content makes it impossible to move them both together.

Alternatively, an option to move the field label inside the fences wrapper for the field content would work well for my use case, too.

This would be heavily used on a site I'm working on.

I've just run into a spin off of the above use case I thought would be worth mentioning. It would be useful to have the ability to wrap the field title and the field value with their own unique tags.

I'm creating a machine parts list with each part as a node with different properties like max/min temp, burst pressure, as fields. Ideally we'll wrap a group of related fields in DL. In this case it would be great or the field title to in a DT and then field value wrapped the DD.

Example

<dl>
<dt>Field1 Title (Max Temperature)</dt>
<dd>Field1 Value (450 degrees)</dd>
<dt>Field2 Title (Min Temperature)</dt>
<dd>Field2 Value (-100 degrees)</dd>
</dl>

I think hook_field_attach_view_alter() is what we need.

/**
 * Implementation of hook_field_attach_view_alter().
 */
function MODULE_field_attach_view_alter(&$output, $context) {
  foreach (element_children($output) as $field_name) {
    $field = &$output[$field_name];
    $field['#theme_wrappers'][] = 'container';
    $field['#attributes']['class'] = 'field-'.$field['#field_name'].'-wrapper';
  }
}

This code adds wrapper to every field but I'm not sure if this covers all occurences of displaying a field. I've tested node page and default formatter for field in Views and both works.

Adding simple checkbox to field settings form and adding the #theme_wrappers property to field only when that checkbox is checked should be quite simple.

The code is only needed if the label is displayed inline, and it that case, it is necessary.
I can't come up with a way to put the label inline without it.

/**
 * Implementation of hook_field_attach_view_alter().
 */
function MODULE_field_attach_view_alter(&$output, $context) {
  foreach (element_children($output) as $field_name) {
    $field = &$output[$field_name];
    if ($field['#label_display'] == 'inline') {
      $field['#theme_wrappers'][] = 'container';
      $field['#attributes']['class'][] = 'field-label-inline-wrapper';
      $field['#attributes']['class'][] = 'field-'.$field['#field_name'].'-wrapper';
    }   
  }
}

The second class added here is overkill really.

The SCSS needed

/* Field layout - becuase we're using fences */
.field-label-inline-wrapper {
  @include clearfix();
  .field-label-inline {
    display: inline;
  }
}

This sounds interesting. I desire what appears to be the same thing:

Example:

<figure> <!--This is a field that Fences has modified-->
  <img src="/meow.jpg" alt="Meow">

  <!--This is a field desired to be wrapped inside the <figure> element-->
  <figcaption>I'm a caption. Semantic, aren't I?</figcaption>
</figure>

Really seems like a bad idea to remove wrappers on multi-value fields. Bummed that it works like that as-is.

The code is only needed if the label is displayed inline

As others have mentioned, that is not the only use case for this feature. In addition to the above examples of absolute positioning and multi-value fields, sometimes you may want to style the field label differently for certain fields, or group a form element and its label together for positioning or styling purposes. Since they're all currently h3s with no class or other way to associate it with the field, this becomes impossible. So this feature would be useful in many situations.

I second the request... Label and Value should definitely have a wrapper--or optionally selectable.

The only available option I see with Fences to achieve this (without rewriting things), is to wrap each field in it's own 'group' which would be way too cumbersome.

surely needed, I thought it was there all along, just realized on multivalue fields

EDIT: #7 works very good

What's the current thinking on this issue? There seem to be some options on the table but little movement. While the patch in #7 is a good stop-gap, I'd like to suggest two additional wrapper options per field and move the fences UI into a its own fieldset. See attached UI screenshot with suggested defaults.

I'm not married to the extra Field wrapper since this can be accomplished with Field Collections but I think a label wrapper is needed.

Thoughts?

This would be great but I'm wondering if those settings should be alterable on Manage Display tab or not. Sometimes you want to display field wrapped into a H2, sometimes not.

Maybe a default settings on Field settings page and specific settings on Display settings page (per view mode)?

@trawekp The beauty of fences is that field settings don't change across view modes. If you need per view mode settings then Display Suite may be more suitable.

@mducharme: Isn't field a data? I think that assigning a markup tag to a field (data) is not what we should expect as beatiful. You can never be sure where the field is going to be displayed or in what context. I'm still not conviced...

Display Suite is something I'd rather not use.

I understand your point. View mode overrides would solve that (or fences for defaults + display suite for overrides now). I'd like see this label markup issue resolved first though. Perhaps a new feature request to provide view mode overrides is in order.

** EDIT **
Feature request already exists for display mode overrides: http://drupal.org/node/1888224

Glad to hear that :] I can write a patch that would add a label settings to the field in a couple of days. It depends on free time I'd have.

According to #14 it is impossible to create patch for this. Every template file needs rewriting.

In case of providing markup for label, wrapper and value there must be change in template files. Displaying label would have to be removed from templates and put into a theme function. This is quite big change, everyone who has written his templates would have to refactor them.

Here is the patch that adds a optional wrapper but with no markup specific settings. It means there is only div available.

** EDIT **
This patch loses markup settings for all fields but the patch is simpler.

I'll take a look at this. I've also created a patch that adds a label handler and it works well but is messy. I'll post it up when I have a chance for you to see what I've done for comparison and see if there is a good path forward.

Is there any progress on this?

I would love to be able to output a definition list, for instance. Wrapping multiple fields in a single dl (as in #5) is out of scope for fences, I understand. But for a single field, I guess this should be possible:

<dl>
  <dt>Field label</dt>
  <dd>Field value 0</dd>
  <dd>Field value 1</dd>
  <dd>Field value 2</dd>
</dl>

Currently, I'm overriding the dl-template in my theme to achieve this:

<?php
/**
 * @file field--fences-dl.tpl.php
 * Wrap all field values in <dl><dt><dd> elements.
 *
 * @see http://developers.whatwg.org/grouping-content.html#the-dl-element
 */
?>

<dl class="<?php print $classes; ?>"<?php print $attributes; ?>>

  <dt class="field-label"<?php print $title_attributes; ?>>
    <?php print $label; ?>:
  </dt>

  <?php foreach ($items as $delta => $item): ?>
    <dd>
      <?php print render($item); ?>
    </dd>
  <?php endforeach; ?>

</dl>

Is the patch #21 likely to be committed?

Re-roll patch #21 for latest dev

But this line of code produce a notice:

+++ b/fences.admin.inc
@@ -221,14 +221,24 @@ function _fences_form_field_ui_field_edit_form_alter(&$form, &$form_state) {
+    '#default_value' => $suggestion['wrapper'] ? $suggestion['wrapper'] : 0,

Notice: Undefined variable: suggestion in include() (line 239 of /path/to/fences/fences.admin.inc).

And I can't understand why? Please help.

This usually means that $suggestion['wrapper'] is not set, so casting it to a boolean (evaluating it to be true or false) throws a notice. Try this instead:

'#default_value' => isset($suggestion['wrapper']) ? $suggestion['wrapper'] : 0,

Thanks @marcvangend, I follow your suggestion & notice disappear :)
Here updated patch

I had to re-work the patch from #28 slightly.

Like 198 of fences.module was using an undefined constant as the array key ('value' needed to be in single quotes) and was throwing a PHP notice.

Furthermore, the earlier patch was placing the field API code to add the checkbox outside of the _fences_form_field_ui_field_edit_form_alter function, not within it.

I also tweaked the label on the checkbox itself for clarity.

Neither of the patches at #28 & #30 will apply to 7.x-1.2 or the current 7.x-2.x-dev

Reverting back to 7.x-1.0 and using the patch at #21

Can this get committed pls? Fences is quite a pain to use without the option to wrap the label and content together, as there is no way to position the label.

I would really would like to see this committed.

It would be of no detriment to those that don't need it, but it adds much utility for those that do.

5 years is a long time to wait. Please

I ran into this issue and would very much like to see this fix get into 7.x-1.x as well.

Also needed in the D8 release. Extremly problematic as we are currently also not able to override the field.html.twig to fix it on our own (https://www.drupal.org/project/fences/issues/2572397).

Ok let's take this to Drupal 8 and get it running...

Attached you can find a patch written together with @thomas.frobieter with a working wrapper implementation for Drupal 8. No tests included yet to have a manual review first. If everything is OK for you, we should
Write / change tests
Backport to Drupal 7

We have wrapped the field items stead of the whole field which makes a lot more sense to us... Feedback is welcome.

Attached you can find a cleaner approach which corrects the class names to fit the BEM (http://getbem.com/naming/) and Drupal Core standard.

This *might be a separate issue* but would make sense to commit this together with the wrapper change in Drupal 8. Perhaps it would justify a new 8.x-3.x release because it fixes (but also changes) old, wrong markup.

We should consider to also add twig {block}'s around the single wrappers & values, so we are able to override just parts of the field template file.

@thomas.frobieter that's a great Idea. Would you mind to create a new patch accordingly? I guess it may be part of this issue and doesn't have to be separated.

The new patch adds the Twig blocks and an empty check for the new wrapper settings.

@thomas.frobieter. Patch from #45 seems to be broken. Can not be applied against latest dev.

Reworked patch from #45 and merged changes with #40. We still need to adjust the tests.

Ok this is still quite relevant. Because the patch for 8.x-2.x from #47 conflicts with the major issue in #3018654: Fences seems to be ignored by Layout Builder I'll recreate it after that one has been committed and then we should push things forward.

Ok, here we go. Attached is the patch to review after major #3018654: Fences seems to be ignored by Layout Builder has been fixed and committed! Until that the tests will fail.
I named the patch 8.x-3.x because this should definitely be a new branch because the code structure changes to be more flexible and cleaned up.

-- broken --

Reroll from #49 against latest 8.x-2.x-dev after https://www.drupal.org/files/issues/2019-07-12/3018654-16.patch applied.

I'd be happy to receive your reviews, I think we've improved the structure a lot.

Ok let's start testing now where #3018654: Fences seems to be ignored by Layout Builder has been fixed.

Fixed tests, the IntegrationTest is still a bit unclear because I don't know which Theme it uses (missing knowledge and infrastructure on my side) - I tested with Spark which has no "clearfix" on while the old test had...

If someone could help, that would be perfect.

+++ b/tests/src/Kernel/FieldOutputTest.php
@@ -131,6 +225,8 @@ class FieldOutputTest extends KernelTestBase {
+          'fences_field_items_wrapper_tag' => '',
+          'fences_field_items_wrapper_classes' => '',

Requiring these keys to be set indicates there'll be an issue with config created in previous installations. For BC reasons we need the module to work in exactly the same way before and after these settings were introduced, so we need some defensive checks in case they are missing and default to the pre-patch behavior.

The best way to demonstrate BC like this is to leave all of the existing test cases as they are and only introduce new tests for new functionality.

Hi @Sam152,

I completely understand your point, but from my perspective we have to introduce major changes (release from old burdens) to the markup structure which can't all be migrated from the old ones. Perhaps I'm wrong and it's even possible by moving configuration around, but on the first sight I guess it's not and I think the better structure should be our major focus, not the migration path between the major releases. Existing sites should be able to keep 8.x-2.x and should step by step upgrade to 8.x-3.x.

Anyway I agree that we should try to find ways to migrate old configuration and markup structures as good as possible, but this issue definitely needs a new major 8.x-3.x release. Keeping the old tests and see what is possible is a very good strategy, but perhaps that should include a config update / migration step. The code and variables should still express what they really do, even if it makes the migration harder.

We didn't even have a stable release yet so I think that should be possible, shouldn't it? See it like Drupal 7 to Drupal 8 major upgrade.

Backwards compatibility shouldn't be too hard here right? We can default to simply hiding the field item wrapper markup if no element has been explicitly selected.

Strictly speaking, we'd need both an update hook to adjust config and to also support non-updated config. That's unfortunately a consequence of: #1677258: Configuration system does not account for API version compatibility between modules.

This is not forgotten. We're still using this in all our projects and it works great. We just didn't have the time to finalize the patch and upgrade path yet.

@Sam152: If you'd be interested, I'd be interested in Co-Maintainership for a 3.x branch. But I'll reply here as soon as we have the time to proceed and finalize things. Sorry.

What needs to be done on this issue? Is it just an update hook or two? Do we need to handle updates from 7.x versions as well as earlier 8.x versions?

Hi @benjifisher,

thank you for your reply and my delay. I'm very busy currently. Yes, what has to be done is what Sam152 wrote in #57:

Strictly speaking, we'd need both an update hook to adjust config and to also support non-updated config.

plus

Anyway I agree that we should try to find ways to migrate old configuration and markup structures as good as possible, but this issue definitely needs a new major 8.x-3.x release. Keeping the old tests and see what is possible is a very good strategy, but perhaps that should include a config update / migration step. The code and variables should still express what they really do, even if it makes the migration harder.

Furthermore the patch has to be rerolled, I guess. I'll do that as soon as possible. If someone is willing to help to push things forward, I'd be happy.

If anyone is using this patch until reroll, simply use
"drupal/fences": "2.x-dev#ed9aed6", in composer to switch back to the alpha2 status.

Sorry, this is on my priority list, but I'm still very busy. I'll create a new patch to create a 3.x branch.
2.x should be kept for users who don't need the new features and 3.x should become new development branch, but not yet suggested branch until it's widely tested.

And yes there should be an upgrade path from 2.x to 3.x, preserving markup by migrated settings if possible. New installations receive the new default structure, I'd suggest.

If there is a Drupal 7 upgrade path to 2.x everything will work with the 3.x incremental update, because we should keep all hook_updates since older versions and simply add new ones to 3.x.
If there is no upgrade path yet, adding Drupal 7 upgrade path would be a separate, unrelated issue.

What do you think?

Ok let's reroll this against latest .dev and align tests.

Sorry, here's the interdiff.

@Sam152: Re #55/#57: I have 3 options in mind and would like to hear which you like most or if you see a better alternative:

a) Add fences_field_items_wrapper_tag and set it to "none" by default. The disadvantage is that you'd have to enable it on every single instance, where fences is used to get very helpful markup for themers, which is even standard for many themes and core bartik for example. The reason would be BC, but it means a lot of manual changes for every instance now and in the future, if you need it.

b) Create a new 3.x version and keep the 2.x version. In 3.x we add and enable field__items as new default, like many themes do. If people switch to 3.x and want the 2.x behaviour without field__items, this would mean a lot of manual change now and in the future (like (a), just the other way around).

c) Create a settings page for fences to set the four default tags globally and add a "Global default setting" option to each of the four selects so you can configure the defaults centrally. Settings default to 2.x standard.

(c) is my personal favourite because you can control the field tags defaults globally and only have to change the value if required. Me make no expensive assumptions here what the user requires. Between (a) and (b) I'd choose (b) because 2.x is kept and 3.x introduces better markup for the future.

A 3.x version should be created anyway for these new features, I think. To reflect the important change and tell website owners about the new option which is often relevant for themers. Furthermore we could allow testing the feature prior to stable release and introduce semver btw?

I'll have a look at the patch errors soon, was just a first shot. If anyone has some time, please provide feedback about a/b/c and help to fix the patch without changing existing IntegrationTests and FieldOutputTests, just add new ones. I'll revert that part too, as @Sam152 is right in #55 to ensure BC this way.

Keeping the old tests. Tests for new functionality will be added with next patch.

New patch with update hook proposal, please have a look, what you think:

/**
 * Add new wrapper_tag configuration to all entity_view_displays using fences.
 */
function fences_update_8301(&$sandbox) {
  // See https://www.drupal.org/docs/drupal-apis/update-api/updating-entities-and-fields-in-drupal-8#s-updating-entity-view-display-configs
  if ($view_displays = \Drupal::entityTypeManager()->getStorage('entity_view_display')->loadMultiple(NULL)) {
    // Loop through all entity view displays:
    foreach ($view_displays as $view_display) {
      $components = $view_display->getComponents();
      foreach ($components as $componentName => $component) {
        if(!empty($component['third_party_settings']['fences'])){
          // Explicitely set fences_field_items_wrapper_tag to 'none' if not existing yet.
          if(!isset($component['third_party_settings']['fences']['fences_field_items_wrapper_tag'])){
            $component['third_party_settings']['fences']['fences_field_items_wrapper_tag'] = TagManagerInterface::NO_MARKUP_VALUE;
          }
          // Explicitely set fences_field_items_wrapper_classes to 'none' if not existing yet.
          if(!isset($component['third_party_settings']['fences']['fences_field_items_wrapper_classes'])){
            $component['third_party_settings']['fences']['fences_field_items_wrapper_classes'] = TagManagerInterface::NO_MARKUP_VALUE;
          }
          // Save changes:
          $view_display->setComponent($componentName, $component)->save();
        }
      }
    }
  }
}

Does someone know if the testbot will run update hooks? In this case I named the hook 8301 because I presumed a 3.x version for this feature. The failing tests are related to the missing keys for the new items.

Patch #68 applies cleanly against latest dev and rc-1 FYI

Checking patch config/schema/fences.schema.yml...
Checking patch fences.module...
Checking patch field.html.twig...
Applied patch config/schema/fences.schema.yml cleanly.
Applied patch fences.module cleanly.
Applied patch field.html.twig cleanly.

@Sam152 & @benjifisher, could you perhaps have a look at #65 to #70 and give some feedback what you think?

I'm now back on this issue and will use issue forks for further implementation to make it easier to review the changes for all of us.

Thanks a lot :)

Pushed #68 to Issue Fork branch 1343578-add-optional-wrapper:
https://git.drupalcode.org/issue/fences-1343578/-/tree/1343578-add-optio...

Added a (currently WIP) Merge Request: https://git.drupalcode.org/project/fences/-/merge_requests/2

You can track the MR changes as patch live here: https://git.drupalcode.org/project/fences/-/merge_requests/2.patch

This issue now has Tugboat live preview for the MR! Yay! :) That should make it a lot easier to test and discuss proposed changes.

Hopefully this triggers tests on the Issue fork MR!2...

EDIT: Sadly not. @Sam152 could you perhaps enable tests for issues / patches as written in https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

The merge request is tested via automated testing (if the project maintainer has set up automated testing for patches), and reviewed and tested by community members. This may result in additional commits being made in the fork and added to the merge request.

Removed the BC as the update hook allows us to do this non-breaking. I'll also add the further steps to issue summary.

I mentioned this before, but the safest way of adding keys to config schema is to both support updates via a hook and to have BC and defaults:

Strictly speaking, we'd need both an update hook to adjust config and to also support non-updated config. That's unfortunately a consequence of: #1677258: Configuration system does not account for API version compatibility between modules.

The update hooks don't cover config that may have been exported into a module or profile ./config/(install|optional). There is no system to update these exported objects, that don't exist in active configuration.

Thank you @Sam152,

I added further checks to fall back gracefully, if the configuration is not existing due to the reasons given in #81. I guess it's feature-complete now?

Could you please have a look and tell if any further changes are required or this is ready for RTBC?

Unassigning for review. In my eyes this is ready. Please test and provide feedback. Still think a 3.x semver release for this would make sense to be 100% safe...

As there's a bunch of work in this issue and we still think this is very useful and a huge improvement to the module, I'd like to ask the maintainers of this module for further feedback and steps.

I wrote @Sam152 a PM some weeks ago, but didn't receive a reply, guess he's busy. So I'd like to ask the maintainers, if they're planning to proceed here in the near future.

Otherwise, we'll have to create a fences fork, which I don't like to do, but patching this in for more than three years also isn't a good final solution.

Great work once again @Grevil! RTBC and as written in #84 I'd vote to create a 3.x release for this change.

Needs rebase after the latest changes.

Please review carefully! As this was quite a big rebase!, also I haven't applied the patch from https://www.drupal.org/project/fences/issues/1781940#comment-13649024, maybe someone with twig experience can tell me, if the current twig file will add the extra spaces or not.

Tests need a rewrite, as of these changes!

Current state as patch. Still working on the whitespace issues -.-

Ok finally I think we have a nice intendation now. That was pain... ;) Let's see what the testbot says.

Okay the failing tests indeed point to an issue, but as the test output containing HTML is stripped, here's the result with HTML to be easier to compare:

There were 7 failures:

1) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "No field markup" (array('none', '', 'none', '', 'none', ''), true, 'lorem ipsum')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'lorem ipsum'
+'field_testlorem ipsum'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

2) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Only a field tag" (array('article', '', 'none', '', 'none', ''), true, '<article class="field field--...ticle>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<article class="field field--name-field-test field--type-text field--label-above field__items">lorem ipsum</article>'
+'<article class="field field--name-field-test field--type-text field--label-above">field_testlorem ipsum</article>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

3) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Only a field and label tag" (array('article', '', 'none', '', 'h3', ''), true, '<article class="field field--...ticle>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<article class="field field--name-field-test field--type-text field--label-above field__items"><h3 class="field__label">field_test</h3>lorem ipsum</article>'
+'<article class="field field--name-field-test field--type-text field--label-above"><h3 class="field__label">field_test</h3>lorem ipsum</article>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

4) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Only a field and field item tag" (array('article', '', 'h2', '', '', ''), true, '<article class="field field--...ticle>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<article class="field field--name-field-test field--type-text field--label-above field__items"><div class="field__label">field_test</div><h2 class="field__item">lorem ipsum</h2></article>'
+'<article class="field field--name-field-test field--type-text field--label-above"><div class="field__label">field_test</div><h2 class="field__item">lorem ipsum</h2></article>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

5) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Default field, no label" (array('', '', '', '', '', ''), false, '<div class="field field--name...</div>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<div class="field field--name-field-test field--type-text field--label-hidden field__items"><div class="field__item">lorem ipsum</div></div>'
+'<div class="field field--name-field-test field--type-text field--label-hidden"><div class="field__item">lorem ipsum</div></div>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

6) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Default field, with label" (array('', '', '', '', '', ''), true, '<div class="field field--name...</div>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<div class="field field--name-field-test field--type-text field--label-above field__items"><div class="field__label">field_test</div><div class="field__item">lorem ipsum</div></div>'
+'<div class="field field--name-field-test field--type-text field--label-above"><div class="field__label">field_test</div><div class="field__item">lorem ipsum</div></div>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

7) Drupal\Tests\fences\Kernel\FieldOutputTest::testFieldOutput with data set "Classes and tags" (array('ul', 'item-list', 'li', 'item-list__item', 'li', 'item-list__label'), true, '<ul class="item-list field fi...></ul>')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'<ul class="item-list field field--name-field-test field--type-text field--label-above field__items"><li class="item-list__label field__label">field_test</li><li class="item-list__item field__item">lorem ipsum</li></ul>'    
+'<ul class="item-list field field--name-field-test field--type-text field--label-above"><li class="item-list__label field__label">field_test</li><li class="item-list__item field__item">lorem ipsum</li></ul>'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96
/var/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:177
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

FAILURES!
Tests: 8, Assertions: 24, Failures: 7.

What's happening and what's the cause (to be fixed):
Class field__items is missing on field_tag where it was before:
<{{ field_tag|default('div') }}{{ attributes.addClass(classes, 'field__items') }}> (Before)
<{{ field_tag|default('div') }}{{ attributes.addClass(classes) }}> (After)

as it was moved down to:

{% if display_items_wrapper_tag ~%}
  <{{ field_items_wrapper_tag|default('div') }}{{ field_items_wrapper_attributes.addClass('field__items') }}>
{%- endif -%}

Core also sets field__items on the field_tag if there are multiple items, see https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/themes/class... for example
or
https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/themes/olive...

Perhaps we should also implement the multiple check to be as close as possible to the core results?

So if display_items_wrapper_tag is TRUE the field__items class should be at the wrapper
If display_items_wrapper_tag is FALSE, it should be at field_tag like in 2.x and in core.

So we don't have to make a breaking change.

That could be done in PHP or in twig, but we should also consider people may have overwritten the twig file, so I guess one of both options is better. I think it might be the twig solution.

Let's discuss this with @thomas.frobieter and look for the missing additional test for now.

So back to NW.

Furthermore, I'm wondering where our additional tests with wrappers are gone... Hope we'll find them and I didn't just forget to push the changes when I wrote them.

When this issue is resolved, we should also add an example screenshot for the items field wrapper on the module page, and add an items field wrapper description in #3303663: Add descriptions for the field settings.

Added field__items if wrapper is unused to keep backwards compatible.

@thomas.frobieter you forgot to push

I guess the implementation in this issue fixes the issue found in #3303655: [2.x INFO] Setting Field label "none" hides the label value (BC). Let's rebase this to see if the tests go green then!

Retriggered tests!

Yay! Great work, thanks all here, let's merge this into 3.x and create a 3.0.0-alpha0!

Thank you all soooo much, this is amazing! :)