Remove whitespace modifiers from links.html.twig

The purpose is to remove whitespace.

http://twig.sensiolabs.org/doc/templates.html#whitespace-control

If there is a desire to remove these modifiers from the twig templates then we will have to change many more templates than this one.

There are currently 139 whitespace modifiers across 41 template files:

./core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig
./core/modules/filter/templates/filter-caption.html.twig
./core/modules/forum/templates/forum-icon.html.twig
./core/modules/image/templates/image-crop-summary.html.twig
./core/modules/image/templates/image-resize-summary.html.twig
./core/modules/image/templates/image-scale-summary.html.twig
./core/modules/node/templates/field--node--created.html.twig
./core/modules/node/templates/field--node--title.html.twig
./core/modules/node/templates/field--node--uid.html.twig
./core/modules/system/templates/details.html.twig
./core/modules/system/templates/form-element-label.html.twig
./core/modules/system/templates/item-list.html.twig
./core/modules/system/templates/links.html.twig
./core/modules/system/templates/pager.html.twig
./core/modules/system/templates/system-themes-page.html.twig
./core/modules/system/templates/table.html.twig
./core/modules/system/templates/tablesort-indicator.html.twig
./core/modules/system/tests/modules/common_test/templates/common-test-foo.html.twig
./core/modules/update/templates/update-project-status.html.twig
./core/modules/user/templates/user.html.twig
./core/modules/user/templates/username.html.twig
./core/modules/views/templates/views-view-list.html.twig
./core/modules/views/templates/views-view-summary-unformatted.html.twig
./core/modules/views_ui/templates/views-ui-display-tab-bucket.html.twig
./core/modules/views_ui/templates/views-ui-display-tab-setting.html.twig
./core/themes/classy/templates/content-edit/filter-caption.html.twig
./core/themes/classy/templates/dataset/forum-icon.html.twig
./core/themes/classy/templates/dataset/forum-list.html.twig
./core/themes/classy/templates/dataset/item-list.html.twig
./core/themes/classy/templates/dataset/table.html.twig
./core/themes/classy/templates/dataset/tablesort-indicator.html.twig
./core/themes/classy/templates/field/field--node--created.html.twig
./core/themes/classy/templates/field/field--node--title.html.twig
./core/themes/classy/templates/field/field--node--uid.html.twig
./core/themes/classy/templates/form/details.html.twig
./core/themes/classy/templates/form/form-element-label.html.twig
./core/themes/classy/templates/navigation/links.html.twig
./core/themes/classy/templates/navigation/pager.html.twig
./core/themes/classy/templates/user/user.html.twig
./core/themes/classy/templates/user/username.html.twig
./core/themes/seven/templates/tablesort-indicator.html.twig

What's the reason for removing them at all? Who wants a whole bunch of empty line breaks from branches and loops in the outputted file?

These failing tests are confusing to me. Why would such a simple change cause MySQL errors? I re-created the patch after a merge from 8.0.x. Hopefully, this will pass...

Testbot doesn't seem to like my patch for some odd reason. This patch is from @joelpittet, which is shown in the issue description.

Does this interdiff make it clearer?

interdiff remove-excess-whitespace-modifiers_0.patch drupal-2472591-remove-link-template-whitespace-8.patch > interdiff-0-8.txt

The question of whether we can remove the 'if' should be a separate issue IMO.

This needs to update the Classy template as well.

Re-writing tests to be more flexible and allow for "non-hardcoded" HTML output.

I wasn't able to complete this one yet, but I will re-visit it in a couple days if no one else has completed it. The latest patch actually does work, but the test needs to be rewritten in order for it to pass. Currently the template's output is being compared against a hardcoded HTML output string, and it should be converted to something like an xpath pattern in order to make this particular test more flexible.

The test in question can be found at /core/modules/block/src/Tests/Views/DisplayBlockTest.php::testBlockContextualLinks()

The last two lines:

$this->assertIdentical($json[$id], '<ul class="contextual-links"><li class="block-configure"><a href="' . base_path() . 'admin/structure/block/manage/' . $block->id() . '">Configure block</a></li><li class="entityviewedit-form"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1">Edit view</a></li></ul>');
$this->assertIdentical($json[$cached_id], '<ul class="contextual-links"><li class="block-configure"><a href="' . base_path() . 'admin/structure/block/manage/' . $cached_block->id() . '">Configure block</a></li><li class="entityviewedit-form"><a href="' . base_path() . 'admin/structure/views/view/test_view_block/edit/block_1">Edit view</a></li></ul>');

Should be changed to something like this:

$menu = $this->xpath('//ul[contains(@class, "contextual-links")]/li[contains(@class, "block-configure")]/a[href="/admin/structure/block/manage/' . $block->id(). '"]');
$this->assertEqual((string) $li_one, "Configure block");

The above xpath asssertion does not actually work, but I think it's on the right track. I would be interested in seeing what others are able to come up with.

Could we write somewhere down that people don't write this kind of tests anymore because they are PITA to test.

At the LA extended sprints, going to work on this issue.

Adding an interdiff for the patch in comment 16 as there wasn't one.

@laurii which type of tests? Xpath assertions? Is there another form of testing that would serve us here? I am fairly green to writing tests with simplytest, so I am open to suggestions.

Thanks!

@mark.labrecque sorry for being a little unfinished on my comment. So I mean its good idea to replace this kind of tests where we test hard coded markup and we should document that its problematic so we don't get any more of this kind of tests. Xpath + assertions way to go!!

Don't you mean un-Finnish in your comment? Sorry, I couldn't resist :)

Thanks for your response

I hope someone would teach me to un-Finnish my comments so they would be easier to read ;)

I'm working with @connorwk at the DrupalCon Los Angeles extended sprints, and this is our first attempt at doing a patch using XPath to avoid whitespace problems in the test assertions. This should make the ExpandDrupal\views_ui\Tests\DisplayTest test no longer fail. (This obviously isn't a complete patch -- I'm just attempting to mentor @connorwk and see if this method might work.)

The patch @jaredsmith submitted is not working. I will continue to work on this to figure out what is needed to be done.

So I fixed the first failed test in Drupal\block\Tests\Views\DisplayBlockTest
I was posting my progress to allow others to review my work and let me know if what I am doing seems correct.
I replaced assertIdentical with xpath to fix the problem we have when we remove the whitespace modifiers.
I will continue to work on the other tests in the mean time.

Thanks @connorwk, good to see you again at DCLA. Setting to needs review.

the test looks _way better_ to be! Good work on that! Hopefully we can still make rest of the tests pass

Good work @connorwk. Thanks for participating in the extended sprint at DrupalCon Los Angeles. Now that you've got one test passing, you can use that same technique to fix up the other failing tests.

Keep up the great work!

Currently, I am blocked on this, as when the latest patch is applied to my local install, the tests expected to pass are failing due to the following message:

Valid HTML found on "http://drupal8.local/contextual/render?destination=test-page" 	Browser 	DisplayBlockTest.php 	291 	Drupal\block\Tests\Views\DisplayBlockTest->testBlockContextualLinks() 	Pass
simplexml_import_dom(): Invalid Nodetype to import

simplexml_import_dom(Object)
Drupal\simpletest\WebTestBase->parse()
Drupal\simpletest\WebTestBase->xpath('//ul/li/a[contains(@href, :block) and text()="Configure block"]', Array)
Drupal\block\Tests\Views\DisplayBlockTest->testBlockContextualLinks()
Drupal\simpletest\TestBase->run(Array)
simpletest_script_run_one_test('1', 'Drupal\block\Tests\Views\DisplayBlockTest::testBlockContextualLinks')

	Warning 	AssertContentTrait.php 	133 	Drupal\simpletest\WebTestBase->parse() 	Exception
The contextual links are present. 	Other 	DisplayBlockTest.php 	292 	Drupal\block\Tests\Views\DisplayBlockTest->testBlockContextualLinks() 	Fail
Valid HTML found on "http://drupal8.local/contextual/render?destination=test-page" 	Browser 	DisplayBlockTest.php 	294 	Drupal\block\Tests\Views\DisplayBlockTest->testBlockContextualLinks() 	Pass
simplexml_import_dom(): Invalid Nodetype to import

simplexml_import_dom(Object)
Drupal\simpletest\WebTestBase->parse()
Drupal\simpletest\WebTestBase->xpath('//ul/li/a[contains(@href, :cached_block) and text()="Configure block"]', Array)
Drupal\block\Tests\Views\DisplayBlockTest->testBlockContextualLinks()
Drupal\simpletest\TestBase->run(Array)
simpletest_script_run_one_test('1', 'Drupal\block\Tests\Views\DisplayBlockTest::testBlockContextualLinks')

	Warning 	AssertContentTrait.php 	133 	Drupal\simpletest\WebTestBase->parse() 	Exception
The contextual links are present.

These same tests are passed by the testbot, so this could simply be due to some local setting/extension needing to be enabled/installed, so I will look into this when I get some time.

Alright this patch should finally fix all the tests that were previously failing.
Most of it was switching over from using assertIdentical over to xpath.
Using assertIdentical is a problem because if we just rewrote the string to work with assertIdentical if one white-space is added into the html it again breaks the test, so using xpath should prevent such problems again.

But for Drupal\system\Tests\Theme\FunctionsTest I had to do some other things.
First off we were using assertThemeOutput for testing the output, upon further inspection of why this function was failing it also used assertIdentical which we know was broken by removing the whitespace modifiers.
Since I couldn't simply rewrite assertThemeOutput as it was used in other places I created a new test function which is a slight rewrite of it called assertThemeOutputByXPath. It takes in the same info but the $expected variable shall now be formatted for the xpath function.

Also in the later tests (also in Drupal\system\Tests\Theme\FunctionsTest) which where failing the strings fed into assertEqual needed to be trimmed with the trim function because there was some extra whitespace around it now.

On another note maybe we should change the other locations that use assertThemeOutput with my new function assertThemeOutputByXPath to prevent other tests breaking in the future by white-spaces? Xpath seems to be the more proper way to test for nodes like this instead of seeing if two strings are identical. Maybe I should open another issue for this? Let me know what you guys think, assertThemeOutput isn't used in to many other locations so doing so wouldn't be too much of a tremendous task. I'd even do it if everyone thinks it should be done.

I should also note I removed checking of class information as per a suggestion from @davidhernandez so if you ask why that is gone I can't answer you other then I was told to avoid checking them :)

+++ b/core/themes/classy/templates/navigation/links.html.twig
@@ -34,25 +34,25 @@
+{% if links %}

I'm wondering if this is actually a BC breaking change because of display: inline-block; is being messed in case list has spaces between its items.

Forgot to assign. Working on this issue.

At this point we should probably leave Classy and Stable alone and just make this change in the core links.html.twig template, then we don't need to worry about BC :)

Made the changes in the core links.html.twig template @Cottser

Changed the status to needs review.

Thanks @vpanicke that's looking better! Setting to needs work for the issue summary update.

Thanks @Cottser , Updated the Summary as well.

Corrected the Version number

Updated the tests

This needs peer review, @vpanicke generally we don't RTBC our own patches unless it's a trivial fix or something which is sometimes done as part of a patch review. Thanks!

Thanks @Cottser ,was not aware of that. Please can you help review close the same.

Didn't know about this, thanks this looks great. There are a bunch of block level elements so the whitespace shouldn't be hurt from this change, the only span in there didn't change, heading.level could be an inline element potentially, but not realistically and also no whitespace elements needed on it.

+++ b/core/modules/system/templates/links.html.twig
@@ -33,25 +33,25 @@
       <li{{ item.attributes }}>
-        {%- if item.link -%}
+        {% if item.link %}
           {{ item.link }}
-        {%- elseif item.text_attributes -%}
+        {% elseif item.text_attributes %}
           <span{{ item.text_attributes }}>{{ item.text }}</span>
-        {%- else -%}
+        {% else %}
           {{ item.text }}
-        {%- endif -%}
+        {% endif %}
       </li>

Are we sure that we should be adding all this whitespace inside the li's?

It would be great to have a comparison between what this template currently outputs and what it will with all the changes.

Here's the output before and after the patch. And the patch only affects core/system and not stable.

http://twigfiddle.com/68oi2r

Before:

<h3>Heading</h3><ul><li>Text 1</li><li><span enabled>Text 2</span></li><li><a>Link 1</a></li></ul>

After:

                <h3>Heading</h3>
        <ul>
            <li>
                        Text 1
                    </li>
            <li>
                        <span enabled>Text 2</span>
                    </li>
            <li>
                        <a>Link 1</a>
                    </li>
    </ul>

I can see a scenario where the LI's are changed to inline elements and the whitespace could matter then for the inside. We could mitigate this by just removing the whitespace inside the <li> elements. I don't think the rest would matter in any practical sense.

Second opinions?

links.html.twig File in two place.
core/modules/system/templates/links.html.twig
core/themes/classy/templates/navigation/links.html.twig

Please suggest where me wrong.

I agree with joelpittet's comment , we just need to remove white space inside the 'li' elements.

Unable to apply patch on latest 8.3.x branch output is here for git apply:

NA+lnadella@MINGW64 /c/www/drupal (2472591-remove-whitespace-links-html-twig)
$ git apply drupal-remove-link-template-whitespace-2472591-48.patch
error: patch failed: core/modules/system/templates/links.html.twig:33
error: core/modules/system/templates/links.html.twig: patch does not apply
error: patch failed: core/themes/classy/templates/navigation/links.html.twig:32
error: core/themes/classy/templates/navigation/links.html.twig: patch does not apply

Reroll to 8.3.

I have attached patch for the issue. Please review it.

@vaplas I am adding this as related issue.

We will work on this issue in DrupalMumbaiCodeSprint

Removed whitespace in links.html.twig also added interdiff

Thanks!!

@all i have removed the white space please check the patch.

Added patch for remove whitespace we can use {% spaceless %} {% endspaceless %}for remove whitespace in twig

Last provided patch "node-2472591-90.patch" is empty. What should be reviewed here?
Also, for the patch before that ("2472591-86.patch") the interdiff is missing.

Removed dashes and added spaceless in core/modules/system/templates/links.html.twig, core/themes/classy/templates/navigation/links.html.twig, core/themes/stable/templates/navigation/links.html.twig, test is passing

...sorry, i don't get it - please someone take over...

David notes on #2 (https://www.drupal.org/project/drupal/issues/2472591#comment-9834761), we do not need to remove the whitespace modifiers everywhere. Only in places where it is simply unnecesary.

Patch #83 and #86 both failed due to a large addition of unexpected white space created by this.

Cottser notes in #48 that we should only be updating core links.html.twig to preserve backwards compatibility.

Using the twigfiddle from #64 ( http://twigfiddle.com/68oi2r ), the solution for #95 actually creates unnecessary whitespace around each list item, due to the reduced whitespace modifiers. This is what is failing the tests.

The cleanest code appears to be a mix of {% spaceless %} and - as on the attached patch.

However, does having whitespace between the <li> </li> go against Drupal Coding Stanards as noted at https://www.drupal.org/docs/develop/coding-standards/twig-coding-standar... ? Not sure.

+1 we've reviewed this at the 2019 Austin Contribution Weekend. The use of the whitespace modified within the <li> tags makes sense to me.

Removed dashes in core/modules/system/templates/links.html.twig

@Mithun S thanks for the contribution. Could you upload an interdiff also please. Makes it easier to review.

https://www.drupal.org/documentation/git/interdiff

Don't consider the patch in #99. Patch in #97 only addresses links.html.twig in stable theme. What about the other links.html.twig in classy theme and system module as shown? Aren't they also part of core folder? Could somebody guide me on this?

Working on this issue at Friday contribution sprints at DrupalCon Seattle 2019.

Patch includes changes for Classy links.html.twig file, and core links.html.twig file. The spaceless tag now wraps the output of all markup, but the shorthand spaceless dashes remain in the unordered list markup.

I'm reviewing this at Drupalcon Seattle 2019!

Does anyone have any updates on this issue?

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I have tested the patch provided in comment #103 and it is successfully applied in drupal 10.1.

Patch #103, no longer applies to 10.1.x. I have re-rolled the patch. Please review

<h3>Heading</h3><ul><li>Text 1</li><li><span enabled>Text 2</span></li><li>&lt;a&gt;Link 1&lt;/a&gt;</li></ul>

This is the outcome using patch #118 is that desired?

Think this will need a different solution.

https://twig.symfony.com/doc/2.x/tags/spaceless.html

As of Twig 2.7, use the spaceless filter instead.

Not sure that's the approach we want to take

Applied the patch on #18 cleanly.

Addressed #119 and #120, used https://twig.symfony.com/doc/2.x/tags/spaceless.html

120 was not addressed.

And what was addressed for 119? Didn’t say anything was off? Just stating what I saw.

Relooking at this and 118 May actually be correct. Moving review for 118

@smustgrave yeah now i noticed spaceless filter
{{
"

"|spaceless }}

is #118 good or should I make a patch to spaceless filter

Think 118 is fine.

This needs an issue summary update:

1. Do we actually need spaceless, or could we just remove the modifiers with no replacement? It adds an extra level of indentation and we don't tend to strip whitespace everywhere else.
2. What is/was the outcome of this from the issue summary? We should either try to figure that out, or open a follow-up.

Also, this template has a wrapping if statement:

{% if links -%}
...
{%- endif %}

The if was a direct copy of functionality from when this was a theme function. We cannot verify why it is still needed. Investigate whether it can be removed, and, if so, remove it.

Based on #127 I am removing the 'Novice tag'