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

martins.bruvelis → made their first commit to this issue’s fork.

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?

@martins.bruvelis thank you very very much, this looks impressive! :)
I'll need more time to review this in detail, but great work!

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?

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?

@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

Thanks all, I still think, this is a very handy addition!
I'd like to ask @Grevil and @thomas.frobieter to review and test this. It should not change existing behavior, but provide a convenient way for non-developers to use fences for overrides.

@Grevil and @thomas.frobieter should check the logics and the general functionality plus ensure BC, so nothing breaks.
@Grevil please think about #12 and check if there are sufficient tests for both cases. I agree we shouldn't add a test-dependency on any third-party theme. It would have to be tested with core themes.

I agree, this is brilliant work and looks really great! Very likely to be merged.

@martins.bruvelis did you check if Olivero perhaps overwrites the field.html.twig? Also the issue summary says, Bartik doesn't work with fences? But that's removed from core in Drupal 10: https://www.drupal.org/node/3304352 →

@Grevil: After review and feedback please hand over to @thomas.frobieter!

#Needs manual testing for @thomas.frobieter and @Grevil

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 are of enabling this setting 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 📌 Refactor Kernel Tests to explicitly use a Theme Fixed , 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

#25 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.)

@Chris Burge and all the others here, could you kindly try / test the MR and tell if it fixes the situation as expected for you?
Then we should try to get this in, instead of adding custom fixes.

Leaving this NW as the final points from #23 have to be added and the patches by @Foxy-vikvik were not helpful outside of the MR. Thanks!

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();.

Thanks for the detailed feedback and testing! That sounds good.

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();.

I agree on that!

Any ideas for tests (see discussion above). Otherwise I'd merge this whenever community says it's reviewed enough.

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

Whao @Chris Burge that was a smart idea! Thank you for that and the implementation. As written above, we're not running into this issue, as we have our own base themes, so the priority for us isn't that high on this task. But I'm very thankful the community helps to get that fixed!

I guess the failing test might be caused by the issue fork, we had such cases in the past in other forks.

I left some final comments. Then I think this is ready for RTBC by another reviewer.

Thank you @Chris Burge! Left some comments. We're getting closer :)

Thank you so much @Chris Burge! Looks great. Hope I (or someone else) will have the time for a final code-check. Very busy currently.

In the meantime, what do you think about these last points from the issue summary?

- [] and in the README.md
- [] Show a warning in the status report if this issue is detected

I guess some more information might make sense to better explain the new option and the need for it?

Finally, this seems to need a rebase. But we're getting super close, thanks to your awesome work here!

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().

@Chris Burge thank you very much once again. Perfect, I agree with all these points. And sorry, I'm very busy currently and trying to maintain as many modules as possible for the community, so I can't help coding here currently.

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: Add proper documentation for all versions on the module page & README.md → .

Went ahead and wrote test coverage.

Thanks @Chris Burge that looks great! :)

The failing tests seem unreated, but there are 3 code style fixes to do. After that I think it's perfect and ready to go!

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.

Thanks @Chris Burge sorry I thought I had seen a report about the lines in the .install file in the MR above. Maybe I was wrong...

@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).

Thanks @Chris Burge. So RTBC from my side. Anything else to consider for existing sites?
Otherwise, I think we can merge this into 3.x, what do you think?

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?

@Chris Burge confirm #51! Just looked that up in another module.

BTW really sad to have no or hard to find documentation at Drupal.org for these things ... :/

Thank you all! Merged into dev now. Let's see if the tests work in main.

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