The template file links.html.twig has a lot of whitespace modifiers. These dashes here, {%- if heading -%}

They were originally added when the template was converted, but don't seem to serve a purpose and can be removed. They might have been there originally for tests but joelpittet confirmed they aren't needed for that now. See the patch here, https://www.drupal.org/node/2465399#comment-9829099

If they can be removed, remove them from the template in the System module, and the template in Classy. To determine if they can be removed we need to look for any instances where links might be rendered inline. That is a place where whitespace could be an issue.

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.

If this issue is worked on after #2455211: Comment field displayed last regardless of assigned weight gets committed, investigate removing the if from the links template in Bartik, as well.

Update : Made the changes in the core links.html.twig template to fix the white-spaces issue as mentioned by Cottser
https://www.drupal.org/node/2472591#comment-10878510

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because it's not broken, just minor cleanup to the template
Issue priority Normal, this only changes the appearance of the template code and whitespace in markup.
Unfrozen changes Unfrozen because it only changes templates.
Disruption Not disruptive at all.
Files: 
CommentFileSizeAuthor
#74 remove-whitespace-73.patch2.41 KBimshivani
#73 interdiff-70-73.txt543 bytesimshivani
#70 edit_remove_whitespace-2472591-70.patch2.42 KBfelribeiro
#68 After.png76.72 KBlomasr
#65 interdiff.txt1.3 KBamit.drupal
#65 drupal-remove-link-template-whitespace-2472591-48.patch2.42 KBamit.drupal
#49 drupal-remove-link-template-whitespace-2472591-47.patch1.21 KBvpanicke
#43 interdiff-2472591-34-43.txt19.08 KBconnorwk
#43 drupal-remove-link-template-whitespace-2472591-43.patch22.66 KBconnorwk
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,525 pass(es). View
#34 interdiff-2472591-16-34.txt1.67 KBconnorwk
#34 drupal-remove-link-template-whitespace-2472591-34.patch4.33 KBconnorwk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,349 pass(es), 16 fail(s), and 0 exception(s). View
#30 interdiff-2472591-16-30.txt1.62 KBjaredsmith
#30 drupal-2472591-remove-link-template-whitespace-30.patch3.68 KBjaredsmith
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views_ui/src/Tests/DisplayTest.php. View
#25 interdiff-2472591-12-16.txt1.17 KBconnorwk
#16 drupal-2472591-remove-link-template-whitespace-16.patch2.54 KBmark.labrecque
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,016 pass(es), 20 fail(s), and 0 exception(s). View
#14 interdiff-0-8.txt1.62 KBcilefen
#12 remove-excess-whitespace-modifiers.patch1.24 KBmark.labrecque
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,770 pass(es). View
#8 drupal-2472591-remove-link-template-whitespace-8.patch1.14 KBmark.labrecque
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,759 pass(es), 13 fail(s), and 0 exception(s). View
#5 drupal-2472591-remove-link-template-whitespace-5.patch1.14 KBmark.labrecque
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,755 pass(es), 13 fail(s), and 0 exception(s). View

Comments

nlisgo’s picture

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
davidhernandez’s picture

There is no particular desire to remove them everywhere. They are needed in many places, especially in places where the resultant markup will definitely be used inline. Just to clarify, I opened the issue about the links template, in particular, because it is being used heavily (on almost every line) and it may not be necessary. I'm also a bit more interested in this 'if' business, if someone tracks that down.

disasm’s picture

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?

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

FileSize
1.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,755 pass(es), 13 fail(s), and 0 exception(s). View
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: drupal-2472591-remove-link-template-whitespace-5.patch, failed testing.

mark.labrecque’s picture

FileSize
1.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,759 pass(es), 13 fail(s), and 0 exception(s). View

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

mark.labrecque’s picture

Status: Needs work » Needs review
mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque

Status: Needs review » Needs work

The last submitted patch, 8: drupal-2472591-remove-link-template-whitespace-8.patch, failed testing.

mark.labrecque’s picture

FileSize
1.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,770 pass(es). View

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

mark.labrecque’s picture

Status: Needs work » Needs review
cilefen’s picture

FileSize
1.62 KB

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

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

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

This needs to update the Classy template as well.

mark.labrecque’s picture

FileSize
2.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,016 pass(es), 20 fail(s), and 0 exception(s). View
mark.labrecque’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 16: drupal-2472591-remove-link-template-whitespace-16.patch, failed testing.

mark.labrecque’s picture

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

mark.labrecque’s picture

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.

mark.labrecque’s picture

Issue tags: +xpath, +Simpletest
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned
lauriii’s picture

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

connorwk’s picture

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

connorwk’s picture

FileSize
1.17 KB

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

mark.labrecque’s picture

@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!

lauriii’s picture

@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!!

mark.labrecque’s picture

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

Thanks for your response

lauriii’s picture

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

jaredsmith’s picture

FileSize
3.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/views_ui/src/Tests/DisplayTest.php. View
1.62 KB

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

connorwk’s picture

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

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: drupal-2472591-remove-link-template-whitespace-30.patch, failed testing.

connorwk’s picture

FileSize
4.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,349 pass(es), 16 fail(s), and 0 exception(s). View
1.67 KB

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.

Cottser’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 34: drupal-remove-link-template-whitespace-2472591-34.patch, failed testing.

lauriii’s picture

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

jaredsmith’s picture

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!

mark.labrecque’s picture

Issue tags: +Needs reroll
mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

Issue tags: -Needs reroll
mark.labrecque’s picture

Assigned: mark.labrecque » Unassigned

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.

connorwk’s picture

Status: Needs work » Needs review
FileSize
22.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,525 pass(es). View
19.08 KB

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 :)

snetcher’s picture

Version: 8.0.x-dev » 8.1.x-dev
lauriii’s picture

Status: Needs review » Needs work
+++ 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.

vpanicke’s picture

vpanicke’s picture

Assigned: Unassigned » vpanicke

Forgot to assign. Working on this issue.

Cottser’s picture

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 :)

vpanicke’s picture

Status: Needs work » Needs review

Changed the status to needs review.

vpanicke’s picture

Cottser’s picture

Status: Needs review » Needs work

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

vpanicke’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

Thanks @Cottser , Updated the Summary as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: drupal-remove-link-template-whitespace-2472591-47.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

vpanicke’s picture

Version: 8.2.x-dev » 8.1.x-dev
Status: Needs work » Needs review

Corrected the Version number

vpanicke’s picture

Status: Needs review » Reviewed & tested by the community

Updated the tests

Cottser’s picture

Status: Reviewed & tested by the community » Needs review

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!

vpanicke’s picture

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

emma.maria’s picture

Version: 8.1.x-dev » 8.2.x-dev
Issue tags: -Needs issue summary update

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Component: system.module » theme system
Assigned: vpanicke » Unassigned
Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
+++ 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.

joelpittet’s picture

Issue tags: -xpath, -Simpletest

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?

amit.drupal’s picture

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

Status: Needs review » Needs work

The last submitted patch, 65: drupal-remove-link-template-whitespace-2472591-48.patch, failed testing.

amit.drupal’s picture

Please suggest where me wrong.

lomasr’s picture

FileSize
76.72 KB

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

lnadella’s picture

Issue tags: +Needs reroll, +#dcdelhi

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
felribeiro’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

Reroll to 8.3.

Status: Needs review » Needs work

The last submitted patch, 70: edit_remove_whitespace-2472591-70.patch, failed testing.

Manuel Garcia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
imshivani’s picture

FileSize
543 bytes

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

imshivani’s picture

vaplas’s picture

I can not understand why you are so eager to get these terrible holes between a tags? I have a real case when empty tags must be empty #2789909: Remove spaces around row.content (in shot, css :empty - not working with holes). Twig is still not able to beautifully arrange spaces (see meta-tags output for example :)). A browser editor (see command code element) are displayed well no matter spaces. Therefore, improvement in this issue do not make sense to me.

Status: Needs review » Needs work

The last submitted patch, 74: remove-whitespace-73.patch, failed testing.

Manjit.Singh’s picture

@vaplas I am adding this as related issue.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

aditya.ghan’s picture

We will work on this issue in DrupalMumbaiCodeSprint