[make:*] use php-cs-fixer to style/lint all generated php templates #1251

jrushlow · 2022-12-06T16:23:51Z

We use php-cs-fixer to lint generated php files created by all makers. We use the following precedents for determining which php-cs-fixer config and binary to use:

PHP CS Fixer Config

MAKER_PHP_CS_FIXER_CONFIG_PATH env is set, we use it
If root-project-path/.php-cs-fixer.dist.php exists, we use it
We use the bundled config from Resources/config/php-cs-fixer.config.php

PHP CS Fixer Binary

MAKER_PHP_CS_FIXER_BINARY_PATH env is set, we use it
We use the bundled php-cs-fixer phar located in src/Bin/php-cs-fixer-v3.13.0.phar

An exception is thrown if either of the env var's above are set, and the file does not exist.

Passing -v to the command will show which php-cs-fixer binary and configuration file maker is using.

Internally, we no longer run php-cs-fixer in every make:* test. Doing so now would basically test php-cs-fixer itself is working - not our business... Lets save time in each test...

weaverryan

Cool 😎

weaverryan · 2022-12-08T17:51:40Z

README.md

+
+MakerBundle uses php-cs-fixer to enforce coding standards when generating `.php`
+files. If you have `friendsofphp/php-cs-fixer` added to your project, we'll
+use the `bin/php-cs-fixer` binary and `.php-cs-fixer.dist.php` configuration file


Suggested change

use the `bin/php-cs-fixer` binary and `.php-cs-fixer.dist.php` configuration file

look for and use the `bin/php-cs-fixer` binary and `.php-cs-fixer.dist.php` configuration file

Wouldn't/shouldn't it be vendor/bin/php-cs-fixer?

Also, since installing php-cs-fixer into your composer.json isn't a best practice, I'm not sure we should support this. We could look to see if there is a global php-cs-fixer binary available and use that.

In all cases, if we're using their binary or their config file (regardless of if it's automatic or because they set env vars), it might be a good idea to print a small note at the top of the command's output about this.

When you install via composer e.g. (composer require --dev ......), php-cs-fixer installs the binary in the path-to-project/bin folder https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/a2bdba33ddb7df1cedce536c3e9c3db8d73ddd3e/composer.json#L65 (Note: our test "supports" this argument but then again I wrote the test so its probably flawed.. haha)

Looking at their readme, https://github.com/PHP-CS-Fixer/PHP-CS-Fixer#installation we could check for the binary in tools/php-cs-fixer instead/in addition to the above.

I didn't even think about php-cs-fixer installed globally - I'm wondering if we should add this bit in after the fact due to the different ways that the binary can be installed (manually, via composer, phive, and brew). I'd have to do some playing around to see if there are any differences in the PATH vars for us to use. e.g. installing manually (globally) the user has to:

Download composer -> move it to /usr/local/bin on linux -> ensure thats in their system PATH

But you can also install globally via composer by composer global install....

I'm thinking we could have a more robust method of checking for the binary using the autoloader -> checking the global path(s).

Thoughts?

In all cases, if we're using their binary or their config file (regardless of if it's automatic or because they set env vars), it might be a good idea to print a small note at the top of the command's output about this.

Agree'd

We also need to figure out how to write tests for the global installs as I don't think that is something we can do right out of the box now - but I could be mistaken.

I think we can simply use this class: https://github.com/symfony/process/blob/6.2/ExecutableFinder.php

If we find php-cs-fixer, cool. Use it. Else, don't. It's so simple I don't think we'd need any additional tests.

php-cs-fixer installs the binary in the path-to-project/bin folder https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/a2bdba33ddb7df1cedce536c3e9c3db8d73ddd3e/composer.json#L65 (

I think that means "install php-cs-fixer into whatever Composer's bin directory is", which is vendor/bin by default.

Executable finder works for me...

README.md

src/Generator.php

weaverryan · 2022-12-08T17:59:09Z

src/Util/TemplateLinter.php

+            return;
+        }
+
+        $this->phpCsFixerBinaryPath = \dirname(__DIR__).'/Bin/php-cs-fixer-v3.13.0.phar';


Any thoughts on how we could not forget to routinely update this?

Aside from post it notes, thinking we have a new ci job the runs daily to check for the latest version and compares it to our php-cs-fixer.VERSION.php bin file name... I don't think we want this running on every PR/push as that we result in a bunch of rebases over time?

LOL

Maybe a monthly CI job would be awesome if it's not too much work - one that could create a PR with the change would be ideal.

jrushlow · 2022-12-12T17:44:18Z

src/Generator.php

-                $this->getFileContentsForPendingOperation($targetPath, $templateData)
+                $this->getFileContentsForPendingOperation($targetPath)


Unused var unrelated to this pr

jrushlow · 2022-12-12T17:52:43Z

tests/Maker/MakeMigrationTest.php

-                $this->assertCount(14, explode("\n", $output), 'Asserting that very specific output is shown - some should be hidden');
+                $this->assertCount(17, explode("\n", $output), 'Asserting that very specific output is shown - some should be hidden');


We've added additional lines to the output. This test should be changed in a separate PR to assert exactly what we're looking for rather than total number of lines. - Todo added on a post-it

weaverryan

Minor comments - looks awesome!

weaverryan · 2022-12-13T16:09:46Z

src/Resources/doc/index.rst

+file by their respective environment variables:
+
+- ``MAKER_PHP_CS_FIXER_BINARY_PATH`` e.g. /path/to/project/php-cs-fixer.php
+- ``MAKER_PHP_CS_FIXER_CONFIG_PATH`` e.g. /path/to/project/php-cs-fixer.config.php


Can I set this to just a path in my app? That'd be much more useful - e.g.

MAKER_PHP_CS_FIXER_CONFIG_PATH=.php-cs-fixer.dist.php

Same for the binary - can I just say MAKER_PHP_CS_FIXER_BINARY_PATH=php-cs-fixer to use a globally-installed one?

Also, I think using .php-cs-fixer.dist.php in the example would be more realistic.

...to use a globally-installed one?

If by globally, you mean one that is installed in the project directory? Then Yes... I change the "path" example to tools/vendor/bin/php-cs-fixer to sort of match php-cs-fixer's install docs. That with the "tip" below - should cover our bases i think...

Also, I think using .php-cs-fixer.dist.php in the example would be more realistic.

Good Catch!

src/Util/TemplateLinter.php

weaverryan · 2022-12-13T16:13:07Z

src/Command/MakerCommand.php

@@ -90,13 +95,17 @@ protected function interact(InputInterface $input, OutputInterface $output): voi

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
+        $output->write($this->linter->getLinterUserMessage());


My gut reaction at this moment is: let's only show this if $output is in verbose mode or higher. Will be a lot of noise... for something where I think 90+% of people will be happy enough with the output.

agree'd - unfortunately I can test for this... $output->isVerbose() is always true during the tests. I think this may having something to do with bridge and how we get deprecations? But im not sure.. Didn't spend a whole lot of time digging into it.

Locally, this works as expected.. pass -v and we get the message...

k0d3r1s · 2023-03-08T13:22:56Z

Hey. Any reason this is not in any tagged version yet?

weaverryan · 2023-03-08T15:14:50Z

We're just behind on MakerBundle right now due to some "in real life" stuff. Hopefully we can catch up soon :)

This PR was merged into the 1.0-dev branch. Discussion ---------- [git-released] v1.49.0 # Changelog ## [v1.49.0](https://github.com/symfony/maker-bundle/releases/tag/v1.49.0) *Jun 7th, 2023* ### Feature - [#1321](#1321) - Changing make:stimulus-controller to require StimulusBundle - *`@weaverryan`* - [#1309](#1309) - Apply `get_class_to_class_keyword` PHP-CS-Fixer rule - *`@seb`-jean* - [#1276](#1276) - [make:migration] Change message when required package for migration doesn't exist. - *`@bdaler`* - [#1261](#1261) - [make:registration-form] use UniqueEntity attribute instead of annotation - *`@jrushlow`* - [#1253](#1253) - [make:migration] Add link to new migration files - *`@nicolas`-grekas* - [#1251](#1251) - [make:*] use php-cs-fixer to style/lint all generated php templates - *`@jrushlow`* - [#1244](#1244) - [make:security:form-login] new maker to use built in FormLogin - *`@jrushlow`* - [#1242](#1242) - [make:*] use static return type instead of self for setters - *`@jrushlow`* - [#1239](#1239) - Improve error messages to show PHP & XML configurations are not supported - *`@ThomasLandauer`* - [#1238](#1238) - [make:*] improve output messages for Symfony CLI users - *`@jrushlow`* - [#1237](#1237) - [make:registration-form] Print registration form errors - *`@comxd`* ### Bug - [#1307](#1307) - [make:twig-component] handle upstream changes to how live components are rendered - *`@jrushlow`* - [#1270](#1270) - [make:authenticator] Core\Security or SecurityBundle\Security - Avoid deprecations in 6.2 - *`@nacorp`* - [#1265](#1265) - [make:crud] Make sensio/framework-extra-bundle an optional dependency - *`@acrobat`* - [#1264](#1264) - [make:controller] doctrine/annotations is not needed - *`@jrushlow`* - [#1262](#1262) - [make:reset-password] doctrine/annotations are not needed - *`@jrushlow`* _Generated by Git Released_ Commits ------- a7bb5c6 Updating date, adding 1 more PR cb06c33 [release] v1.49.0

jrushlow added Feature New Feature Status: Needs Work Additional work is needed labels Dec 6, 2022

jrushlow changed the title ~~WIP - [make:*] use php-cs-fixer to style/lint all generated php templates~~ [make:*] use php-cs-fixer to style/lint all generated php templates Dec 8, 2022

jrushlow added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Dec 8, 2022

jrushlow marked this pull request as ready for review December 8, 2022 17:25

jrushlow mentioned this pull request Dec 8, 2022

[make:security:json-login] WIP #1246

Open

jrushlow added a commit to jrushlow/maker-bundle that referenced this pull request Dec 8, 2022

php-cs-fixer moved to symfony#1251

a692b79

weaverryan reviewed Dec 8, 2022

View reviewed changes

jrushlow commented Dec 12, 2022

View reviewed changes

weaverryan reviewed Dec 13, 2022

View reviewed changes

jrushlow added 18 commits December 14, 2022 10:47

[make:*] use php-cs-fixer to lint generated php files

b087e09

php-cs-fixer it up

29fc215

allow user to use their own config and bin

40a3805

add template linter to command test

383ef3e

assert the linter is working

ee288c9

test linter works in project

b6aa002

use default php-cs-config file and binary if it exists

be16d93

better readme

7598e22

cleanup

2015522

add docs for symfony/doc

b5695b4

move the binary to resources

71dae37

refactor to track all generated files

aaa0d31

use finder to get the php-cs-fixer executable

21ab33c

composer can tell us the bin paths

e1db268

no looking for a global php-cs-fixer

6e8e562

we dont look for the binary anymore

fd02a55

consistency

1d523e6

better docs

f622ef2

jrushlow force-pushed the feature/use-php-cs-fixer-in-makers branch from eadbd57 to f622ef2 Compare December 14, 2022 15:58

jrushlow added 4 commits December 14, 2022 11:04

better path eg

6dc6e28

no php

68aec4c

only show linter output if verbose

e0c4f29

refactor linter message

0a86bf4

weaverryan approved these changes Dec 14, 2022

View reviewed changes

jrushlow added 2 commits December 14, 2022 15:20

let add the ability to use a php-cs-fixer thats in the system path

ad2d4f6

we no longer need to run php-cs-fixer in all of our tests

926a3b3

jrushlow added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Dec 14, 2022

ensure we use php prefix on windows

d38cf50

jrushlow merged commit 97f9fa5 into symfony:main Dec 15, 2022

jrushlow deleted the feature/use-php-cs-fixer-in-makers branch December 15, 2022 04:32

jrushlow added a commit to jrushlow/maker-bundle that referenced this pull request Dec 15, 2022

php-cs-fixer moved to symfony#1251

8432cf1

jrushlow mentioned this pull request May 23, 2023

[git-released] v1.49.0 #1310

Merged

jrushlow added a commit to jrushlow/maker-bundle that referenced this pull request May 23, 2023

php-cs-fixer moved to symfony#1251

053bcb5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[make:*] use php-cs-fixer to style/lint all generated php templates #1251

[make:*] use php-cs-fixer to style/lint all generated php templates #1251

jrushlow commented Dec 6, 2022 •

edited

Loading

weaverryan left a comment

weaverryan Dec 8, 2022

weaverryan Dec 8, 2022

jrushlow Dec 8, 2022 •

edited

Loading

jrushlow Dec 8, 2022

weaverryan Dec 8, 2022

weaverryan Dec 8, 2022

jrushlow Dec 8, 2022

weaverryan Dec 8, 2022

jrushlow Dec 8, 2022

jrushlow Dec 8, 2022

weaverryan Dec 8, 2022

jrushlow Dec 12, 2022

jrushlow Dec 12, 2022

jrushlow Dec 12, 2022

weaverryan left a comment

weaverryan Dec 13, 2022

jrushlow Dec 14, 2022

weaverryan Dec 13, 2022

jrushlow Dec 14, 2022

k0d3r1s commented Mar 8, 2023

weaverryan commented Mar 8, 2023

	use the `bin/php-cs-fixer` binary and `.php-cs-fixer.dist.php` configuration file
	look for and use the `bin/php-cs-fixer` binary and `.php-cs-fixer.dist.php` configuration file

		$this->getFileContentsForPendingOperation($targetPath, $templateData)
		$this->getFileContentsForPendingOperation($targetPath)

		$this->assertCount(14, explode("\n", $output), 'Asserting that very specific output is shown - some should be hidden');
		$this->assertCount(17, explode("\n", $output), 'Asserting that very specific output is shown - some should be hidden');

[make:*] use php-cs-fixer to style/lint all generated php templates #1251

[make:*] use php-cs-fixer to style/lint all generated php templates #1251

Conversation

jrushlow commented Dec 6, 2022 • edited Loading

PHP CS Fixer Config

PHP CS Fixer Binary

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrushlow Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k0d3r1s commented Mar 8, 2023

weaverryan commented Mar 8, 2023

jrushlow commented Dec 6, 2022 •

edited

Loading

jrushlow Dec 8, 2022 •

edited

Loading