Twig debugging enables the output of HTML comments summarizing the used Twig template and its suggestions.
This is implemented inside \Drupal\Core\Template\TwigThemeEngine::renderTemplate (and \twig_render_template of core/themes/engines/twig/twig.engine).
The added HTML does not only include comments but also line breaks between the comments.
Whitespace between HTML tags inserts empty text nodes into the DOM which can affect the appearance of the rendered document.
Do not output whitespace between HTML comment tags when Twig debugging is active.
No user interface changes.
No introduced terminology.
No API changes. (I hope no one depends on Twig debug adding empty text nodes to the DOM.)
No data model changes.
Twig debug mode does not add text nodes only containing whitespace to the DOM anymore.
| Comment | File | Size | Author |
|---|
Show commandsStart 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:
changes
Comments
Comment #3
cilefen commentedThis looks like it should be a child issue of #2914733: [META] twig debug can break parts of site, and it may actually duplicate one of the issues linked there.
I'm not certain about that, and it's cool to get a test for this.
Comment #4
tgaugesComment #5
tgaugesI implemented a solution in core/lib/Drupal/Core/Template/TwigThemeEngine.php (and core/themes/engines/twig/twig.engine) and a new test core/modules/system/tests/src/FunctionalJavascript/Theme/TwigDebugMarkupTest.php.
I also needed to update existing tests and
\_filter_autopincore/modules/filter/filter.module.Please review :)
Comment #6
tgaugesComment #7
tgaugesComment #8
tgaugesThis issue might be interpreted as a duplicate of #3363487: newline on glossify-link.html.twig causes extra space. But that issue is in the project Glossify and the conclusion is that
(#12 by jacobupal)
So this is now that core issue. I found no other (core) issue specifically regarding whitespace added by the twig debug output.
Comment #9
tgaugesComment #10
tgaugesI updated the tests a bit so that both
TwigDebugMarkupTestclasses use the same code to enable Twig debug: https://git.drupalcode.org/project/drupal/-/merge_requests/14122/diffs?diff_id=1883246&start_sha=023eeadb7d4bdf524bf0f0e2c836724deb7acea6Comment #11
tgaugesComment #12
tgaugesComment #13
jwilson3Comment #14
ghost of drupal pastI must admit I was largely ignorant of the problems here but https://developer.mozilla.org/en-US/docs/Web/CSS/Guides/Text/Whitespace lays it out.
However, I am a bit unhappy about adding an entire functional test for this. Is it really necessary?
Comment #15
tgaugesIs the problem with the new test that it is a browser test?
I think with PHP 8.4 and Dom\Element::textContent one could rewrite the test to not use JavaScript in a browser.
@ghost of drupal past Would that be a solution you are happier with?
One could also simply check for the exact string with the line breaks at the expected positions. But that makes the test more brittle in case of unrelated changes to the HTML comments.
Comment #17
smustgrave commentedAppears to need a rebase.
Comment #18
tgaugesThanks, I merged main into the issue branch. There were no line conflicts. The tests are still good.
Comment #19
ghost of drupal pastThanks for the work.
Yes, I was thinking a kernel test should work and would be significantly faster.
You could look at FilterCaptionTwigDebugTest and its register method to see how to enable twig debug in a kernel test. You could look at the TwigAttributesTest (also a kernel test) to see how you could test with inline templates using renderInline.
Comment #20
tgaugesThanks for the input. I moved the issue back to "Needs work" with a new task to re-implement the new test.
I'm currently not actively working on this. So if someone else wants to do it, feel free.
Comment #21
tgaugesI lied. I found time today to work on this. The webdriver test is replaced with a functional test, please take a look.
Comment #22
tgaugesComment #23
smustgrave commentedSorry appears to need another rebase after the theme engine removals and filter.module moved to hooks (assuming those are the conflicts). Will try and keep an eye out next round.
Comment #24
tgaugesThanks for the feedback. I re-merged main into the issue branch.
As you wrote: The main conflict was the removal of core/modules/filter/filter.module which is now replaced with core/modules/filter/src/Plugin/Filter/FilterAutoP.php.
Comment #25
smustgrave commentedOnly question is do we need a test trait if that function is only called from 1 file?
Comment #26
tgaugesNo, we don't need that trait. I removed it: ef189d77
Comment #27
smustgrave commentedFeedback appears to be addressed on this one, no additional suggestions.
Comment #28
quietone commentedI've made suggestions in the MR to correct coding standards. I also think a comment can be removed, so setting to needs work for others to consider.
I updated credit.
Comment #29
tgauges@quietone Thanks for the coding standards review. I read up on https://project.pages.drupalcode.org/coding_standards/php/documentation/ and agree with your comments. The suggestions are applied and I'm setting this issue back to reviewed and tested.
(I also merged current main into the issue branch to fix a failing test.)
Comment #30
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #31
tgaugesI merged main back into the issue branch. It was only a simple conflict.
Comment #33
catchCommitted/pushed to main, thanks!
Needs a backport MR for 11.x
Comment #35
tgaugesI created !16853 as backport for 11.x.
It is a cherry-pick and a removal of the test which required PHP 8.4 (
Dom\HTMLDocument).Comment #36
smustgrave commentedNot sure if we need a new test but we will see!
Comment #37
longwaveIn 11.x we also have a copy of the template render function in twig.engine, which we should update here to match.
In two minds whether we should backport that test or not.