X Tutup
Skip to content

add docker wrapper to php-cs-fixer#2388

Open
zordsdavini wants to merge 7 commits intophpactor:masterfrom
zordsdavini:phpcsfixer_docker_wrapper
Open

add docker wrapper to php-cs-fixer#2388
zordsdavini wants to merge 7 commits intophpactor:masterfrom
zordsdavini:phpcsfixer_docker_wrapper

Conversation

@zordsdavini
Copy link
Contributor

@zordsdavini zordsdavini commented Oct 3, 2023

The main idea is to add wrapper to php-cs-fixer command that it could be run from docker with same php version as project. The case why can't be used sh wrapper with phpactor in project container is that phpactor itself can't be in project.

Wrapper command should be built from:

docker exec -i -u nobody php_pk /bin/sh -c "XDEBUG_MODE=off PHP_CS_FIXER_IGNORE_ENV=1 /var/www/bin/php-cs-fixer fix --dry-run --verbose --format json -"

Plan is:

  • add (example) "language_server_php_cs_fixer.bin": "\/var\/www\/bin\/php-cs-fixer" and "language_server_php_cs_fixer.wrapper": "\/usr\/bin\/docker exec -i -u nobody container_name \/bin\/sh -c"
  • in PhpCsFixerProcess.php on given config convert php-cs-fixer call into sh command and run by Amp\Process

@zordsdavini zordsdavini marked this pull request as draft October 3, 2023 14:29
@zordsdavini
Copy link
Contributor Author

zordsdavini commented Oct 4, 2023

Stuck. Help is needed :) I need to get this (example with interaction):

docker exec -i -u nobody php_pk /bin/sh -c "XDEBUG_MOD=off PHP_CS_FIXER_IGNORE_ENV=1 /var/www/bin/php-cs-fixer fix --dry-run --verbose --format json -"
PHP needs to be a minimum version of PHP 7.4.0 and maximum version of PHP 8.1.*.
Current PHP version: 8.2.10.
Ignoring environment requirements because `PHP_CS_FIXER_IGNORE_ENV` is set. Execution may be unstable.
PHP CS Fixer 3.9.5 Grand Awaiting by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.2.10
Loaded config default from "/var/www/.php-cs-fixer.php".
Using cache file ".php-cs-fixer.cache".
<?php echo ("dss");
{"files":[{"name":"php:\/\/stdin","appliedFixers":["no_unneeded_control_parentheses","blank_line_after_opening_tag","single_quote"]}],"time":{"total":29.961},"memory":16}%  

but in phpactor I get

[2023-10-04 04:36:10] phpactor.DEBUG: Executed '/usr/bin/docker' 'exec' '-i' '-u' 'nobody' 'php_pk' '/bin/sh' '-c' '"XDEBUG_MODE=off PHP_CS_FIXER_IGNORE_ENV=1 /var/www/bin/php-cs-fixer fix --dry-run --verbose --format json -"', which exited with 127 {"channel":"php-cs-fixer"} []
[2023-10-04 04:36:10] phpactor.ERROR: Diagnostic provider "php-cs-fixer" errored with "php-cs-fixer exited with code '127'; cmd: '/usr/bin/docker' 'exec' '-i' '-u' 'nobody' 'php_pk' '/bin/sh' '-c' '"XDEBUG_MODE=off PHP_CS_FIXER_IGNORE_ENV=1 /var/www/bin/php-cs-fixer fix --dry-run --verbose --format json -;"'; stderr: '/bin/sh: 1: XDEBUG_MODE=off PHP_CS_FIXER_IGNORE_ENV=1 /var/www/bin/php-cs-fixer fix --dry-run --verbose --format json -;: not found '; stdout: ''", removing from pool {"channel":"LSPDIAG"

@zordsdavini
Copy link
Contributor Author

zordsdavini commented Oct 4, 2023

found issue. proc_open sh command should not be escaped with "" 🎉

@zordsdavini zordsdavini force-pushed the phpcsfixer_docker_wrapper branch from ae09799 to 328e70a Compare October 4, 2023 12:56
@zordsdavini zordsdavini changed the title add docker wrapper to php-cs-fixer [WIP] add docker wrapper to php-cs-fixer Oct 4, 2023
@zordsdavini zordsdavini marked this pull request as ready for review October 4, 2023 13:21
@dantleech
Copy link
Collaborator

dantleech commented Oct 6, 2023

i'm not convinced this is the right way to approach this. the "wrapper" concept applies to all linters that need to call out to docker-container. it would be better to run Phpactor in the same enviornment as the project (especially as some features such as enums are only enabled when the runtime is f.e. 8.1)

additionally it is better to specify the wrapper ['as', 'an', 'array', 'of', 'arguments'] as this removes any ambiguity caused by quoting.

would it be possible to install multiple versions of PHP on your o/s or distribution? e.g. I had php7.4, php8.0, php8.1 on debian, and on NixOS I can basically choose.

@zordsdavini
Copy link
Contributor Author

As I said before and in the description: this covers the case when phpactor can't be used in same container. I had some thoughts about to have second container with same php version for phpactor only and include project source.
About wrapper command. It seems not so much differents as it doesn't have any quotes at all and in the result it is same array.
I use Arch. It can have different versions of php, but mainly I develop in containers, so it is not so important and project can be in different version as main machine.

@dantleech
Copy link
Collaborator

dantleech commented Oct 6, 2023

although what problem is this solving? i also run many dockerised projects with various requirements, and running the tooling with my globally installed version of PHP (8.1) is fine

@zordsdavini
Copy link
Contributor Author

The problem is php-cs-fixer. It can't be in different versions during checks, because then cache file will be rebuilt and this takes time (from 10 seconds to 5-6 minutes). So, the problem is not directly for phpactor but for the tools around it. I'll give a try to figure out how to use phpactor in another container and how to deal with vim/mason installation but I'm not sure how to deal per project. This patch works for me and I don't need to deal with more complexity but if it is wrong way how to solve my issue then I need to align with the main vision of phpactor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

X Tutup