Doesn't work with themes overwriting field.html.twig (also core like Bartik)

Same problem like we already had in fences_blocks, isn't it?

I really don't like the idea to add a field--fences.html.twig here too, but thinking a step further, Fences as a UI driven tool might should work also for novice users - out of the box.

However, this will be a breaking change, so IF we decide to do this, we need to schedule this to the next major release?

I added a configuration option to allow the override of the field template only if it came from the core or allow to override of the field template for all themes.

@Anybody and @thomas.frobieter would adding Fences module setting to allow to override of the field template for all themes with the default option set to only allowing core theme override avoid a breaking change?

@Anybody, regarding,

Should we also add a line to the status report explaining this option and showing its status? I think that wouldn't be too bad?

Indeed, it would have been very helpful to see a warning displayed after a Bootstrap5 theme update that added field.html.twig template, resulting in Fences module not working. However, I am not sure how to reliably check if currently enabled non-core theme has a field.html.twig template to show a warning in the status report that an issue is detected. Also, I have not checked if there are any contributed themes, that would not work with Fences module. When I checked if Fences module is working Bartik theme, it looked that Fences is working, contrary to what is stated in the issue description above (maybe the issues with Bartik theme was resolved with one of the other issues patches).

Considering that Fences module checks for field.html.twig template via code strpos($theme_registry['field']['path'], 'core') === 0 it seems that Fences module will only override the field.html.twig template and not any of the more specific field templates, such as, field--entity-reference.html.twig. This seems to allow to override specific fields or group of fields via the respective field--*.html.twig templates, while allowing the base field field.html.twig template to be overridden by Fences module.

Regarding,

What we'll definitely need here are tests. For the option disabled and enable to ensure the expected functionality. Could you add them in the next step please?

1. For the disabled option, all of the existing tests are already sufficient, it should behave exactly as before.
2. For the enable option, there are following potential test cases to consider:

• if the theme is from core, nothing should change, and all existing tests are sufficient
• if the theme is not from core, then all of the tests could be re-run with some non-core theme, and test input/outputs should produce the same code as with core theme.

From the above, the biggest issue is that existing tests do not check that the Fences module would stop working if contributed non-core theme is used. Most likely, because it is not easy to add a dependency on some contributed non-core theme, that could potentially add/remove field twig template from the theme with any of the theme updates. For example, in a recent Bootstrap5 theme update, the field.html.twig template was added, and Fences module stopped working without providing any notification, warning or error messages.

Therefore, could someone clarify if it is a common practice to add test where it is required to install, enable, and check functionality with a contributed non-core theme? If it is not common practice then we could proceed without adding any additional tests, as existing tests arealdy check that it works as expected.

Merge request patch works on Drupal 10. Thanks!

#10 patch working well on Drupal 10

Apologies, for the unnecessary commits, I'll resume the review and clean-up tomorrow!

Alrighty! This works great! :)

The adjustments I did on this issue's branch are attached to this comment as a diff. I think we can also remove the update hook entirely, as config inspector has no problem, updating the module without setting the conf, as it isn't a removal of a config!

Here is the HTML output pre-patch with bootstrap5 enabled:

And here is the HTML output after the patch with bootstrap5 enabled AND our newly added config enabled:

I'll add the tests now.

I put all my hopes and dreams on #2409811: Kernel tests should explicitly install themes and tried adding a theme in Kernel Tests like described in the issue, but unfortunately this doesn't seem to work. 😢

Alright, I did it! 🙂

I even created a test class testing the bootstrap theme! The only problem now, is that every theme comes with their own twig field definitions. MEANING, we can NOT use the same expected output for every theme (since the HTML output is quite different for each theme).

So we should discuss, if manual testing is enough for now, we comment out the newly added test classes and create a follow-up issue for finishing these test classes (since this will take a bunch of time), or if we should wait if we (or someone else) finds the time to finish these tests.

Otherwise I'd say this issue is done!

Ok, we internally discussed this issue heavily and came to the conclusion that the current code could be merged, if the newly added setting is properly explained:

• What does the setting do
• When should the setting be used
• What does the setting do when it is unchecked

This should be explained throughout on the settings page, so that new users exactly know what the consequences of enabling this setting are and when to enable it, but thanks @martins.bruvelis, for providing the code and get this whole thing started!

We moved the test creation to #3368195: Refactor Kernel Tests to explicitly use a Theme, so they don't bloat this issue. Also, they don't really have any connection to the newly added setting, so that's that! 🙂

the #24 patch works for Drupal 10

#26 works correctly
The issue with condition was fixed.

@Foxy-vikvik, please provide any changes to the code in this issue fork's issue branch, so it is visible in the MR, thx!

In Drupal 10, both the Stable and the Classy themes were moved to contrib (out of core). Each theme provides a field.html.twig template, so the contrib versions of these themes are affected by this issue, too. (As a quick fix, I wrote a local patch for each themes that simply removed their respective field.html.twig files.)

I tested out MR-22. Testing steps:

1. Install vanilla Drupal 10 site with Bartik contrib theme and Fences module installed.
2. On Article content type, set the label for Body field to display.
3. With Fences, configure the label to display as <h1>.
4. Create an Article node (node/1).
5. Observe label renders as a <div> element.
6. Deploy MR-22 for Fences module.
7. Navigate to admin/config/content/fences and check the 'Override the field template for all themes' checkbox.
8. Reload node/1 and observe label renders as a <div> element.
9. Clear cache.
10. Reload node/1 and observe label renders as a <h1> element.

Summary - In my testing, the MR works as expected. I think we'll want to clear the render cache when the fences.settings config is updated in addition to invalidating the theme registry caches with $this->themeRegistry->reset();.

Re adding test coverage, we don't need to add any dev dependencies for testing (e.g. the Bartik contrib theme). We can add a test theme (fences/tests/themes/fences_test_theme_b) that has a field.html.twig template. The Twig UI module does this. We'd be looking at a Functional test. It'd probably be a good idea to add a second test theme, too, without a field.html.twig template (fences_test_theme_a).

Proposed testing below (Functional):

Set-up

1. Enable the Fences module.
2. Create a content type with a body field.
3. Configure the default display with Fences to add a test class to the body field.
4. Create test content with the body field populated (node/1).

Test 1 (Core without field.html.twig)

1. Make Stark the default theme.
2. Load node/1 and verify test class IS rendered on body field.
3. Via the config form, check the 'Override the field template for all themes' checkbox and save.
4. Load node/1 and verify test class IS rendered on body field.

Test 2 (Core with field.html.twig)

1. Make Olivero the default theme.
2. Load node/1 and verify test class IS rendered on body field.
3. Via the config form, check the 'Override the field template for all themes' checkbox and save.
4. Load node/1 and verify test class IS rendered on body field.

Test 3 (Non-core without field.html.twig)

1. Make Fences Test Theme A the default theme.
2. Load node/1 and verify test class IS rendered on body field.
3. Via the config form, check the 'Override the field template for all themes' checkbox and save.
4. Load node/1 and verify test class IS rendered on body field.

Test 4 (Non-core with field.html.twig)

1. Make Fences Test Theme B the default theme.
2. Load node/1 and verify test class IS NOT rendered on body field.
3. Via the config form, check the 'Override the field template for all themes' checkbox and save.
4. Load node/1 and verify test class IS rendered on body field.

Steps 2-4 on each test could be factored out into a protected method on the test class. Or.. we could also use a dataProvider to run scenarios through a single test method. My vote would be for the dataProvider approach.

I don't see how those test failures are related to this MR.

The two test failures are unrelated to this MR. Interestingly, they don't occur when I run tests locally:

$ ../vendor/phpunit/phpunit/phpunit -c core/ modules/contrib/fences/ --verbose
PHPUnit 9.6.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.16
Configuration: /var/www/html/web/core/phpunit.xml.dist

Testing /var/www/html/web/modules/contrib/fences
.................................                                 33 / 33 (100%)

Time: 03:30.280, Memory: 4.00 MB

OK (33 tests, 144 assertions)

HTML output was generated

Re README.md, there's no README. (Kinda surprised no one has tried to credit farm that yet.) What do you think about opening a new issue for adding a README? We could reference this issue to make sure the new template override behavior is documented in the event the README gets committed after this MR is merged.

Proposed README language re this issue:

## Configuration
By default, the Fences module only overrides the field template (field.html.twig) for core themes. When the 'Override the field template for all themes' setting is enabled, Fences will override the field template for all themes (both core and config).

Re the status warning, what do you think about this?

1. Load all active themes.
2. For each active theme, search the list of available templates for anything starting with 'field-'.
3. List any such templates in a warning returned in hook_requirements().

Just pushed fences_requirements(). I'd like to add test coverage in the next few days.

Turns out there is already a issue for the README: #3303084: Update module page and README.md to be up to date.

Went ahead and wrote test coverage.

While there weren't any coding standards issues with this MR, there were a handful in code:

$ phpcs --standard=Drupal,DrupalPractice web/modules/contrib/fences/ --extensions=php,yml,install,module,twig
 
FILE: /var/www/html/web/modules/contrib/fences/fences.install
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 2 WARNINGS AFFECTING 3 LINES
--------------------------------------------------------------------------------
 83 | ERROR   | Doc comment short description must be on a single line, further
    |         | text should be a separate paragraph
 93 | WARNING | Line exceeds 80 characters; contains 90 characters
 97 | WARNING | Line exceeds 80 characters; contains 94 characters
--------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/fences/fences.links.menu.yml
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 4 | ERROR | [x] Expected 1 newline at end of file; 2 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/fences/fences.module
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 10 | ERROR | [x] Use statements should be sorted alphabetically. The first
    |       |     wrong one is Drupal\Core\Field\FormatterInterface.
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: /var/www/html/web/modules/contrib/fences/src/TagManager.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 6 | ERROR | [x] Use statements should be sorted alphabetically. The first
   |       |     wrong one is Drupal\Core\Extension\ModuleHandlerInterface.
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------


FILE: ...r/www/html/web/modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
--------------------------------------------------------------------------------
 8 | ERROR | [x] Use statements should be sorted alphabetically. The first
   |       |     wrong one is Drupal\entity_test\Entity\EntityTest.
--------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------

Time: 592ms; Memory: 6MB

Pushed a commit to correct them.

@Anybody - It's all good. The three issues in .install were already there. They're all minor changes. Plus, now there's one less thing to worry about when switching to GitLab CI (which checks coding standards).

Actually.. yes - We're adding a new route, so the cache will need to be rebuilt. Typically, the way I've seen this handled is to add an empty post_update function. Thoughts?

This issue seems to persist for the Bulma theme: #3485788: Bulma's field.html.twig overrides and is incompatible with Fences module's

Is there anything themes should do or refrain from doing to be compatible with Fences?

OK that question still stands, as far as best practices for contrib themes to play well with Fences, but the big green box on the module page that links to this issue really needs to be updated to say that one can go to /admin/config/user-interface/fences/settings and enable Override the field template for all themes!

You are right @mlncn, this was missing from the module description. Thanks! I've added it.