aidantwoods / SecureHeaders

A PHP library aiming to make the use of browser security features more accessible.
MIT License
423 stars 23 forks source link

Enable PHP-CS-Fixer #34

Closed lucasmichot closed 7 years ago

lucasmichot commented 7 years ago

Fix code with PHP-CS-Fixer: /vendor/bin/php-cs-fixer

aidantwoods commented 7 years ago

In theory we'd only need to run the style test on a single build, wonder if that's achievable with travis – could use the latest php-cs-fixer if that was possible?

lucasmichot commented 7 years ago

In theory we'd only need to run the style test on a single build, wonder if that's achievable with travis

It is if you select on which branch you should run it

could use the latest php-cs-fixer if that was possible?

Nope, dependencies fail with HHVM build

aidantwoods commented 7 years ago

Nope, dependencies fail with HHVM build

Might be able to get around that with an install script that performed differently on the build we're running the style checking on

lucasmichot commented 7 years ago

It's a bit hard to code at the same times, so feel free to take control of this PR

aidantwoods commented 7 years ago

Extra hard in that we can only test upon push – I think this should do it

aidantwoods commented 7 years ago

No idea what the - global was doing and why it didn't work – got that from the documentation 🤷‍♂️

aidantwoods commented 7 years ago

Okay, so looks like that works as a PoC – can probably clean this up now so that either the style test runs, or the phpunit ones do (no reason to run the unit tests twice on the extra php7.1 build)

aidantwoods commented 7 years ago

Okay, so I've set up a little custom behaviour to trigger if the style check fails.

The output produced by php-cs-fixer [...] --diff is huge, so I've set it up to do a dry run, grab the exit code. If any errors are found then it'll go and apply the fixes, and then show a git diff – so the area causing the error is pretty obvious in the output.

screen shot 2017-04-10 at 23 53 00
aidantwoods commented 7 years ago

@lucasmichot anything else you can think of that we should add here?

aidantwoods commented 7 years ago

Apologies on the wait here!

I actually did a bunch of work over in https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/2684 to add my own styling preferences as configurable options 😉

I was kinda just waiting out the merge, but maybe I'll just require my dev branch until my changes reach stable 🤔 .

lucasmichot commented 7 years ago

Ah ah ah ! Expects very long reviews on https://github.com/FriendsOfPHP/PHP-CS-Fixer (I talk by experience).

Up to you for the merge 😃

aidantwoods commented 7 years ago

Ah ah ah ! Expects very long reviews on https://github.com/FriendsOfPHP/PHP-CS-Fixer (I talk by experience).

😉 seems so, though PR is approved and on the milestone for next point release! (Just taking a little longer than I might've thought).


I think I'll merge this more or less as-is, and take care of switching the styling over separately (since this PR is really about enabling the fixer!)

I might just tweak the .travis.yml one more time though, figured a nicer way of applying conditional tests without using bash if statements – which seems a bit hacky (see https://github.com/Parsemd/Parsemd/blob/master/.travis.yml)

aidantwoods commented 7 years ago

Right, so those last couple commits reformat .travis.yml and add in dist: trusty (referenced issue is the most referenced thing I've ever seen on GitHub: BC break in Travis' HHVM container 😢).

Let me know if that looks good for you, then should be good to merge!

aidantwoods commented 7 years ago

Just to show a bit of what that reformatting did: end result is the same, but now without need for environment variables or ifs in bash.

This is the before:

screen shot 2017-06-15 at 20 41 07

And after:

screen shot 2017-06-15 at 20 40 08

The environment variable STYLE is acting as a label, and isn't really needed (whereas before all builds had the variable, made for quite a lot of visual clutter trying to see what each test was for).