Problem/Motivation

Some files containing code in core have trailing whitespace which doesn't match with our coding standards: https://www.drupal.org/coding-standards#indenting

Proposed resolution

Remove trailing whitespace from core code. Don't touch anything in vendor or .txt files.

Remaining tasks

None. The results of the patch can be verified with ack 2.x:

ack "[ \t]+$" core --ignore-dir=core/vendor --ignore-dir=core/assets --ignore-file=match:.*\.txt

User interface changes

None

API changes

None

Original report by @pillarsdotnet

While submitting unrelated patches, I have noticed several core files that have trailing whitespace. This can easily be cleared up via the following one-line command:

perl -pi -e 's, *$,,' $(find core -regex '.*\.\(engine\|html\|inc\|info\|install\|js\|json\|module\|php\|script\|sh\|sql\|test\|txt\|xml\)')
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tor Arne Thune’s picture

Great. Maybe it would also be possible to remove trailing whitespace from patch files uploaded to Drupal.org. But that's for another issue queue...

pillarsdotnet’s picture

Status: Active » Needs review
FileSize
0 bytes

Patch.

pillarsdotnet’s picture

(sigh) For real, this time.

pillarsdotnet’s picture

@#1 remove trailing whitespace from patch files

Super-terrific-wonderful-great idea for a 6.x/7.x contrib module.

jhodgdon’s picture

Hm. I'm all for this patch, but I'm wondering if we should be changing
core/modules/filter/tests/filter.url-input.txt and
core/modules/filter/tests/filter.url-output.txt

I guess all of the tests passed, but I also noticed there is no newline at the end of these files, so maybe the extra spaces were intentional in some way? I'm not sure, and I would say maybe we should avoid changing those files.

pillarsdotnet’s picture

Re-rolled as requested.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I think this is ready to commit.

Tor Arne Thune’s picture

Yes, looks good. This has no chance of messing up anything, so safe fix!

pillarsdotnet’s picture

Issue tags: +Quick fix, +quickfix

Regenerated after multiple core commits, but the resultant patch is identical to the one in #6.

(bump!)

jhodgdon’s picture

jhodgdon’s picture

Title: Remove all trailing whitespace from Drupal core text files. » Remove all trailing whitespace from Drupal core files.
Component: documentation » other

Just a note that this affects much more than just .txt files and more than just documentation. It probably cannot be committed until the bug thresholds fall though.

webchick’s picture

Assigned: Unassigned » jhodgdon

We're under thresholds so this is back on the table again. Assigning to Jennifer, and sending for a re-test.

webchick’s picture

webchick’s picture

I should mention though that my /vast/ preference is to have things like this handled through http://drupal.org/project/drupalcs so they can be caught *before* the patch is uploaded to d.o.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, remove-trailing-whitespace-1419298-6.patch, failed testing.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Issue tags: +Novice

Unassigning so someone can reroll this patch.

pillarsdotnet’s picture

Status: Needs work » Needs review
FileSize
7.39 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, remove-trailing-whitespace-1419298-17.patch, failed testing.

pillarsdotnet’s picture

Status: Needs work » Closed (won't fix)

Unsubscribing. Since there are only twelve lines changed, and I can't imagine, after a thorough review of the patch, why it would break core, I find that I just don't care anymore.

jhodgdon’s picture

Status: Closed (won't fix) » Active

Just because you're frustrated is not necessarily a good reason to close the issue. Sigh.

pmitchell’s picture

Assigned: Unassigned » pmitchell
Status: Active » Needs work

Rerolling (working on as part of DC2012 core sprints)

BrockBoland’s picture

Status: Needs work » Needs review
pmitchell’s picture

Generated by the command in opening post (against the current HEAD), excludes filter/tests/filter-input-* things (which intentionally include trailing whitespace).

BrockBoland’s picture

Assigned: pmitchell » Unassigned

I applied the patch in #17 to an up-to-date checkout of 8.x and ran all tests, and it came back clean. Is there any chance there was another bug in 8.x that could have caused the tests to fail on this patch?

Re-queued for testing in hopes that it comes back OK this time. It sounds like pmitchell is re-creating the patch against the current HEAD, so I'll leave that to him/her.

BrockBoland’s picture

Status: Needs review » Reviewed & tested by the community

Not sure why pmitchell's worked and pillarsdotnet's did not, but this latest patch looks good to me and passes tests.

NROTC_Webmaster’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
597.57 KB

I know this one is marked to remove trailing whitespace but what about tabs? In particular the misc folder has quite a few files that have tabs instead of spaces.

I didn't know if I should open a new issue or not so please let me know.

Lars Toomre’s picture

The current patch is a monster. To avoid making it even bigger, I would recommend handling replacing tabs with spaces in a new issue so that this one can be put to bed.

NROTC_Webmaster’s picture

Status: Needs review » Reviewed & tested by the community

Setting this back to RTBC. The new issue is #1513540: Replace tabs with spaces from Drupal core files..

catch’s picture

Issue tags: -Quick fix, -Novice, -quickfix

Status: Reviewed & tested by the community » Needs work
Issue tags: +Quick fix, +Novice, +quickfix

The last submitted patch, remove-trailing-whitespace-1419298-26.patch, failed testing.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
5 KB

Reroll of #22

Niklas Fiekas’s picture

Status: Needs work » Needs review
Issue tags: -Quick fix, -Novice, -quickfix

Looks like this doesn't apply anymore. #31: remove-trailing-whitespace-1419298-31.patch queued for re-testing.

Btw. git diff --check $(git hash-object -t tree /dev/null) is another nice way to check for whitespace issues.

Status: Needs review » Needs work

The last submitted patch, remove-trailing-whitespace-1419298-31.patch, failed testing.

Albert Volkman’s picture

Issue tags: +Quick fix, +Novice, +quickfix
FileSize
6.41 KB

It looks like a lot of these have already been cleaned up. Found a few more as well. I see there's some trailing whitespace in Doctrine, Symfony and Twig vendor libraries... do we address those or leave them as-is?

droplet’s picture

so far only few places, it doesn't hack 3rd party scripts.

ramlev’s picture

Have removed all trailing spaces and tabs from complete core

webchick’s picture

Status: Needs review » Postponed

Thanks, but let's hold off on this until after feature freeze so we don't invalidate peoples' patches in the middle of panic-session. And I would much prefer to just have a script that the core committers can run themselves (or ideally, could be run automatically pre-commit) rather than committing 1.85M patches. :D

mgifford’s picture

Issue summary: View changes
Status: Postponed » Needs work
Issue tags: -

after feature freeze.

jsobiecki’s picture

Issue tags: +dcwroc2014
ohthehugemanatee’s picture

wooo! 543 line patch that just removes the ends of lines!

FWIW that perl command didn't run on OSX, because Apple. Had to run it in my vagrant environment. :)

zaporylie’s picture

I fixed missed line endings in two txt files and trailing whitespaces in other one. I've attached interdiff to #40.

zaporylie’s picture

Assigned: zaporylie » Unassigned
mgifford’s picture

Status: Needs review » Reviewed & tested by the community

I applied the patch, reviewed the code and am happy with the results. May this get in before a bunch of other things change.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. index 92e0e1f..095f269 100644
    --- a/core/assets/vendor/jquery-form/jquery.form.js
    
    --- a/core/assets/vendor/jquery-form/jquery.form.js
    +++ b/core/assets/vendor/jquery-form/jquery.form.js
    

    This is not our code. We should not be changing it.

  2. +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    index d06fe66..213dad5 100644
    --- a/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php
    
    --- a/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php
    +++ b/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php
    
    +++ b/core/vendor/doctrine/annotations/lib/Doctrine/Common/Annotations/AnnotationException.php
    index 40d5a69..d54a022 100644
    --- a/core/vendor/doctrine/cache/tests/Doctrine/Tests/Common/Cache/CouchbaseCacheTest.php
    
    --- a/core/vendor/doctrine/cache/tests/Doctrine/Tests/Common/Cache/CouchbaseCacheTest.php
    +++ b/core/vendor/doctrine/cache/tests/Doctrine/Tests/Common/Cache/CouchbaseCacheTest.php
    
    +++ b/core/vendor/doctrine/cache/tests/Doctrine/Tests/Common/Cache/CouchbaseCacheTest.php
    index a01faa5..15f3bcb 100644
    --- a/core/vendor/doctrine/cache/tests/travis/phpunit.travis.xml
    
    --- a/core/vendor/doctrine/cache/tests/travis/phpunit.travis.xml
    +++ b/core/vendor/doctrine/cache/tests/travis/phpunit.travis.xml
    
    +++ b/core/vendor/doctrine/cache/tests/travis/phpunit.travis.xml
    index a0808b3..4d870f6 100644
    --- a/core/vendor/doctrine/collections/lib/Doctrine/Common/Collections/Collection.php
    
    --- a/core/vendor/doctrine/collections/lib/Doctrine/Common/Collections/Collection.php
    +++ b/core/vendor/doctrine/collections/lib/Doctrine/Common/Collections/Collection.php
    
    +++ b/core/vendor/doctrine/collections/lib/Doctrine/Common/Collections/Collection.php
    index 0959ce5..263d860 100644
    --- a/core/vendor/doctrine/common/lib/Doctrine/Common/Util/Debug.php
    
    --- a/core/vendor/doctrine/common/lib/Doctrine/Common/Util/Debug.php
    +++ b/core/vendor/doctrine/common/lib/Doctrine/Common/Util/Debug.php
    
    +++ b/core/vendor/doctrine/common/lib/Doctrine/Common/Util/Debug.php
    index 45aee1c..908a725 100644
    --- a/core/vendor/doctrine/common/tests/Doctrine/Tests/Common/Reflection/ExampleAnnotationClass.php
    
    --- a/core/vendor/doctrine/common/tests/Doctrine/Tests/Common/Reflection/ExampleAnnotationClass.php
    +++ b/core/vendor/doctrine/common/tests/Doctrine/Tests/Common/Reflection/ExampleAnnotationClass.php
    
    +++ b/core/vendor/doctrine/common/tests/Doctrine/Tests/Common/Reflection/ExampleAnnotationClass.php
    index 487c2cd..259d747 100644
    --- a/core/vendor/doctrine/inflector/tests/Doctrine/Tests/Common/Inflector/InflectorTest.php
    
    --- a/core/vendor/doctrine/inflector/tests/Doctrine/Tests/Common/Inflector/InflectorTest.php
    +++ b/core/vendor/doctrine/inflector/tests/Doctrine/Tests/Common/Inflector/InflectorTest.php
    
    +++ b/core/vendor/doctrine/inflector/tests/Doctrine/Tests/Common/Inflector/InflectorTest.php
    index 353d3ba..89c44f7 100644
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Namespace.php
    
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Namespace.php
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Namespace.php
    
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Namespace.php
    index d03b5bd..c53c7c7 100644
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Exception.php
    
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Exception.php
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Exception.php
    
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Exception.php
    index 3915e32..61a4e86 100644
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Turtle.php
    
    --- a/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Turtle.php
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Turtle.php
    
    +++ b/core/vendor/easyrdf/easyrdf/lib/EasyRdf/Parser/Turtle.php
    index 557356b..16b68c6 100644
    --- a/core/vendor/easyrdf/easyrdf/scripts/copyright_updater.php
    
    --- a/core/vendor/easyrdf/easyrdf/scripts/copyright_updater.php
    +++ b/core/vendor/easyrdf/easyrdf/scripts/copyright_updater.php
    
    +++ b/core/vendor/easyrdf/easyrdf/scripts/copyright_updater.php
    index d7f7848..becc6c8 100644
    --- a/core/vendor/phpunit/phpunit/tests/_files/CoverageFunctionParenthesesWhitespaceTest.php
    
    --- a/core/vendor/phpunit/phpunit/tests/_files/CoverageFunctionParenthesesWhitespaceTest.php
    +++ b/core/vendor/phpunit/phpunit/tests/_files/CoverageFunctionParenthesesWhitespaceTest.php
    
    +++ b/core/vendor/phpunit/phpunit/tests/_files/CoverageFunctionParenthesesWhitespaceTest.php
    index 7f67f4b..d1be1c6 100644
    --- a/core/vendor/phpunit/phpunit/tests/_files/CoverageMethodParenthesesWhitespaceTest.php
    
    --- a/core/vendor/phpunit/phpunit/tests/_files/CoverageMethodParenthesesWhitespaceTest.php
    +++ b/core/vendor/phpunit/phpunit/tests/_files/CoverageMethodParenthesesWhitespaceTest.php
    
    +++ b/core/vendor/phpunit/phpunit/tests/_files/CoverageMethodParenthesesWhitespaceTest.php
    index 0b4002d..3e41b4b 100644
    --- a/core/vendor/phpunit/phpunit/tests/_files/RequirementsTest.php
    
    --- a/core/vendor/phpunit/phpunit/tests/_files/RequirementsTest.php
    +++ b/core/vendor/phpunit/phpunit/tests/_files/RequirementsTest.php
    
    +++ b/core/vendor/phpunit/phpunit/tests/_files/RequirementsTest.php
    index f087a3d..31de587 100644
    --- a/core/vendor/psr/log/Psr/Log/LoggerAwareTrait.php
    
    --- a/core/vendor/psr/log/Psr/Log/LoggerAwareTrait.php
    +++ b/core/vendor/psr/log/Psr/Log/LoggerAwareTrait.php
    
    +++ b/core/vendor/psr/log/Psr/Log/LoggerAwareTrait.php
    index 5912496..85597f5 100644
    --- a/core/vendor/psr/log/Psr/Log/LoggerTrait.php
    
    --- a/core/vendor/psr/log/Psr/Log/LoggerTrait.php
    +++ b/core/vendor/psr/log/Psr/Log/LoggerTrait.php
    
    +++ b/core/vendor/psr/log/Psr/Log/LoggerTrait.php
    index 9cd0676..fe77229 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/dotdot.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/dotdot.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/dotdot.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/dotdot.test
    index 7ae3bae..f3c2d6e 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/literals.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/literals.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/literals.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/expressions/literals.test
    index 7821a9a..921b2b3 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/autoescape/with_filters.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/autoescape/with_filters.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/autoescape/with_filters.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/autoescape/with_filters.test
    index 81563dc..2527cdd 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/nested.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/nested.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/nested.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/nested.test
    index cf7953d..5a80758 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/with_extends.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/with_extends.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/with_extends.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/embed/with_extends.test
    index 5034437..af40dff 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/for/objects.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/for/objects.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/for/objects.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/for/objects.test
    index 71e7c20..f1b2d0a 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/inheritance/parent_nested.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/inheritance/parent_nested.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/inheritance/parent_nested.test
    
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/inheritance/parent_nested.test
    index 1d2273f..fa34f37 100644
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/trim_block.test
    
    --- a/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/trim_block.test
    +++ b/core/vendor/twig/twig/test/Twig/Tests/Fixtures/tags/trim_block.test
    

    And neither is any of this. We should only be touching code that is not in a vendor directory.

  3. +++ b/core/assets/vendor/jquery-form/jquery.form.js
    index 7b33af5..22a3a44 100644
    --- a/core/modules/filter/tests/filter.url-input.txt
    
    --- a/core/modules/filter/tests/filter.url-input.txt
    +++ b/core/modules/filter/tests/filter.url-input.txt
    
    +++ b/core/modules/filter/tests/filter.url-input.txt
    index 9cc5073..94a42c0 100644
    --- a/core/modules/filter/tests/filter.url-output.txt
    
    --- a/core/modules/filter/tests/filter.url-output.txt
    +++ b/core/modules/filter/tests/filter.url-output.txt
    

    This is not code and should not have coding standards applied to it.

  4. +++ b/core/modules/filter/tests/filter.url-output.txt
    index 9df2b33..99e2ee7 100644
    --- a/core/modules/simpletest/src/Tests/FolderTest.php
    
    --- a/core/modules/simpletest/src/Tests/FolderTest.php
    +++ b/core/modules/simpletest/src/Tests/FolderTest.php
    
    +++ b/core/modules/simpletest/src/Tests/FolderTest.php
    index e08f1b3..df90beb 100644
    --- a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    
    --- a/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    +++ b/core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
    

    So we only have 2 instances in the whole of core where we have whitepsace incorrectly at the end of a line. Amazing.

ryan.gibson’s picture

Assigned: Unassigned » ryan.gibson

Snatching this before someone else does. :)

ryan.gibson’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
14.13 KB

Alright, here's the patch minus the non-code files and vendor directories. :)

star-szr’s picture

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

Before patch:

$ ack "[ \t]+$" core --ignore-dir=core/vendor --ignore-dir=core/assets --ignore-file=match:.*\.txt

core/modules/simpletest/src/Tests/FolderTest.php
13: * This test will check SimpleTest's treatment of hook_install during setUp. 

core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
203:          'type' => 'inline', 

After patching, no results. I think we're good to get this in and move on :)

star-szr’s picture

Priority: Normal » Minor

Also if this isn't minor I don't know what is :D

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, let's get 'er done. :)

Committed and pushed to 8.0.x. Thanks!

  • webchick committed a7190eb on 8.0.x
    Issue #1419298 by ryanissamson, zaporylie, ohthehugemanatee, ramlev,...

Status: Fixed » Closed (fixed)

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