Skip to content

[8.x] Fixing "Line exceeds 120 characters" in TrustProxies to comply with PSR-2 #5677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 23, 2021
Merged

[8.x] Fixing "Line exceeds 120 characters" in TrustProxies to comply with PSR-2 #5677

merged 1 commit into from
Aug 23, 2021

Conversation

tentacode
Copy link
Contributor

@tentacode tentacode commented Aug 23, 2021 •
edited
Loading

When installing a new project with laravel new and installing squizlabs/php_codesniffer, the projects has one error that cannot be fixed automatically by phpcbf because the app/Http/Middleware/TrustProxies.php class has a line of more than 120 characters.

Laravel is supposed to follow the PSR-2 conventions : https://laravel.com/docs/8.x/contributions#coding-style

To reproduce :

laravel new test-coding-standards
cd test-coding-standards
composer require squizlabs/php_codesniffer
vi phpcs.xml

Small phpcs.xml file :

<?xml version="1.0"?>
<ruleset name="PHP_CodeSniffer">
    <description>The coding standard for our project.</description>
    <rule ref="PSR2"/>

    <file>app</file>
    <file>bootstrap</file>
    <file>config</file>
    <file>database</file>
    <file>resources</file>
    <file>routes</file>
    <file>tests</file>

    <exclude-pattern>bootstrap/cache/*</exclude-pattern>
    <exclude-pattern>bootstrap/autoload.php</exclude-pattern>
    <exclude-pattern>*/migrations/*</exclude-pattern>
    <exclude-pattern>*/seeds/*</exclude-pattern>
    <exclude-pattern>*.blade.php</exclude-pattern>
    <exclude-pattern>*.js</exclude-pattern>
    <exclude-pattern>tests/Feature/ExampleTest.php</exclude-pattern>
    <exclude-pattern>tests/Unit/ExampleTest.php</exclude-pattern>

    <!-- Show progression -->
    <arg value="p"/>
</ruleset>

And run vendor/bin/phpcs, here is the output :

...........W................................... 47 / 47 (100%)

FILE: /Users/gabriel/Workspace/test-cs/app/Http/Middleware/TrustProxies.php
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 22 | WARNING | Line exceeds 120 characters; contains 201 characters
---------------------------------------------------------------------------

Time: 176ms; Memory: 8MB

@tentacode tentacode force-pushed the phpcs-trust-proxies branch from 2ffadae to 1bf0d05 Compare August 23, 2021 11:24
@GrahamCampbell
Copy link
Member

👎 PSR-2 only recommends doing that where it makes sense to. In this instance, it doesn't make sense.

@tentacode
Copy link
Contributor Author

@GrahamCampbell why doesn't it make sense ? this is the only one error that could be avoided in the entire project using phpcsfixer, and you have to admit the line is quite long ?

@driesvints
Copy link
Member

I do agree that seeing the values below each other makes it a bit more readable.

@GrahamCampbell
Copy link
Member

this is the only one error that could be avoided in the entire project using phpcsfixer

No, it couldn't. Automated fixing tools cannot properly fix overlength lines. StyleCI is already enabled on this repo to keep the CS in check, automatically.

@tentacode
Copy link
Contributor Author

@GrahamCampbell sorry let me rephrase that : this is the only error that needs a manual fix on the entire project to be clean on phpcsfixer with PSR2 settings in a new project. We are creating new projects every week and everytime we need to fix this line manually, I might not be the only one so instead of juste writing my own phpcs.xml file I tried to fix the issue at the source :)

@GrahamCampbell
Copy link
Member

I'd of course recommend using using StyleCI on new projects. Laravel apps come with config out of the box, and fixing line length artificially across projects adds little value compared to spending developer time writing new features and fixing bugs. Just let automation fix what it can fix automatically, and forget the rest, especially when it is not a MUST in the spec.

@mfn
Copy link
Contributor

mfn commented Aug 23, 2021

Personally I also couldn't care about the specific rule, but I like the PR for what it is: making it easily readable without having to scroll around -> 👍

I mean this:
image

The line feels way beyond any sane limit (it's 200 or so 😱 )

@alteredchimera
Copy link

I like this PR. More for the fact the line is significantly more readable over the current version.

I agree with Graham's explanation that 120 characters is a recommendation and not a requirement, but in the future should any other headers need to be added here by default, this PR would make it easier.

Just my $0.02.

@GrahamCampbell
Copy link
Member

Sure, 👍 on this PR. My comments were general, addressing the use case of the PR author.

@taylorotwell taylorotwell merged commit 8677c94 into laravel:8.x Aug 23, 2021
@GrahamCampbell GrahamCampbell changed the title Fixing "Line exceeds 120 characters" in TrustProxies to comply with PSR-2 [8.x] Fixing "Line exceeds 120 characters" in TrustProxies to comply with PSR-2 Aug 23, 2021
@tentacode tentacode deleted the phpcs-trust-proxies branch August 23, 2021 15:42
@lakuapik lakuapik mentioned this pull request Oct 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@driesvints driesvints driesvints approved these changes

@CurvesTech CurvesTech CurvesTech approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tentacode @GrahamCampbell @driesvints @mfn @alteredchimera @CurvesTech @taylorotwell