Added some dev tools & code sniffers

[sreichel]

Description (*)

Install some dev-tools:

• https://github.com/FriendsOfPHP/PHP-CS-Fixer
• https://github.com/phpmd/phpmd
• https://github.com/squizlabs/PHP_CodeSniffer

Install code sniffs:

• https://github.com/magento-ecg/coding-standard
• https://github.com/PHPCompatibility/PHPCompatibility

Questions or comments

ToDos:

• add rulesets (fix some errors before)
• add it to worklow
• add update readme and provide some examples?

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)
• Add yourself to contributors list

[fballiano]

can I ask you what do these lines do?

  "scripts": {
    "set-phpcs-paths": [
      "[ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility,vendor/magento-ecg/coding-standard",
      "[ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/phpcs -i"
    ],
    "post-install-cmd": [
      "@set-phpcs-paths"
    ],
    "post-update-cmd":  [
      "@set-phpcs-paths"
    ]

[sreichel]

This commands are executed after every composer install and composer update to add additional sniffs to PHPCS. With --no-dev flag nothing happens.

• $COMPOSER_DEV_MODE is to detect if --no-dev flag was set.
• we have to tell PHPCS where it can find additional sniffs (ECG, PHPCompa...)
• vendor/bin/phpcs -i just shows currently installed sniffs

[fballiano]

oki but is phpcs then doing something? will this impact also store owner that installed openmage with composer?

[sreichel]

No, it does nothing. Just make the tools available. Also no impact for composer installed shops.

[fballiano]

is there a way not to hardcode vendor?

[sreichel]

Tried with COMPOSER_VENDOR_DIR ....

... if you set COMPOSER_VENDOR_DIR as ENV variable it works ...

ddev config.yaml:

web_environment: [
  COMPOSER_VENDOR_DIR=test
]

composer.json

    "set-phpcs-paths": [
      "[ $COMPOSER_DEV_MODE -eq 0 ] || $COMPOSER_VENDOR_DIR/bin/phpcs --config-set installed_paths $COMPOSER_VENDOR_DIR/phpcompatibility/php-compatibility,$COMPOSER_VENDOR_DIR/magento-ecg/coding-standard",
      "[ $COMPOSER_DEV_MODE -eq 0 ] || $COMPOSER_VENDOR_DIR/bin/phpcs -i"
    ],

Having custom vendor dir is an edge case ... i'd not change it.

Edit:

• set vendor-dir in composer.json is not required in this case
• set vendor-dir in composer.json DOES NOT set COMPOSER_VENDOR_DIR

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit affbdeb. ± Comparison against base commit ff13954.

[addison74]

This was the output for the composer install command before the changes in this PR:

This is the output for composer install command after the changes were merged:

From the description, I should have installed those dev-tools and code sniffs, but I didn't, choosing to adopt the behavior of one OM user who is not aware of the changes adopted in this repository and doesn't even read the information, but just takes them as they are.

Shouldn't this situation have been foreseen and the error avoided? Maybe we should have installed them, asked him if he installs them because maybe he doesn't want to, for all these situations the process had to end without errors.

[fballiano]

hmmmm it's weird, because the workflow run smoothly :-\

[justinbeaty]

It looks like it’s the post-install-cmd’s bash syntax and Addison is on windows.

[addison74]

My Linux distribution that I currently use in testing is Debian 10, but here I was asked to do a remote installation of OM + XAMPP + Phpstorm + Composer + ..., so that this person can start working and who knows help us in the future.

I did not check this PR in Debian, but if one who uses Composer and is only familiar with the basic commands, he will be very disappointed when he has to deal with such errors. Until this change, things went as you can see from the screenshot without any issues. If those changes are specific to Linux, maybe they should be taken into account only there. Let's not forget an important aspect, Composer runs on MacOS, Linux and ... Windows. Also, OM can be tested in Windows with WAMP or XAMPP when the developers don't want to take everything to a distribution like Ubuntu. I met many people who used Phpstorm in Windows with remote Linux distributions.

[justinbeaty]

On Ubuntu 20.04, it all looks fine to me:

[...]
  - Installing symfony/dependency-injection (v6.1.3): Extracting archive
  - Installing symfony/config (v6.1.3): Extracting archive
  - Installing pdepend/pdepend (2.10.3): Extracting archive
  - Installing phpmd/phpmd (2.12.0): Extracting archive
Generating autoload files
29 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
Autoloader patch to ./app/Mage.php was applied successfully
> [ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility,vendor/magento-ecg/coding-standard
Using config file: /home/www-data/magento-public/htdocs/vendor/squizlabs/php_codesniffer/CodeSniffer.conf

Config value "installed_paths" added successfully
> [ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/phpcs -i
The installed coding standards are MySource, PEAR, PSR1, PSR2, PSR12, Squiz, Zend, PHPCompatibility, Ecg and EcgM2

I think that we need to have a platform independent composer script to run the phpcs config, like I did here for the release builder: https://github.com/fballiano/openmage/pull/3/files

Or can we use a phpcs.xml file? I haven't used phpcs but I'll investigate it.

[fballiano]

mmmm it is true that on my mac it gives me this error:

fab@MBP-Fab ~/P/openmage (1.9.4.x)> /usr/local/opt/[email protected]/bin/php /usr/local/bin/composer install
you may want to add the packages.firegento.com repository to composer.
add it with: composer.phar config -g repositories.firegento composer https://packages.firegento.com
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Nothing to install, update or remove
Generating autoload files
29 packages you are using are looking for funding.
Use the `composer fund` command to find out more!
./app/Mage.php was already patched
> [ $COMPOSER_DEV_MODE -eq 0 ] || vendor/bin/phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility,vendor/magento-ecg/coding-standard
PHP Fatal error:  Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.1.0". You are running 7.4.30. in /Users/fab/Projects/openmage/vendor/composer/platform_check.php on line 24

Fatal error: Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.1.0". You are running 7.4.30. in /Users/fab/Projects/openmage/vendor/composer/platform_check.php on line 24

when on the first line I lunched it with php8.1

[addison74]

@fballiano - good observation. In my case PHP version installed by XAMPP is 8.1.6. Another condition to be evaluated and which creates confusion. We changed the minimum PHP requirement to version 7.3, but it is required >8.1, and this complicates things in the case of the current PR.

[justinbeaty]

@fballiano I'm curious what happens if you put this in your composer.json file

"scripts": {
    "test-php": "php --version"
}

and run:

/usr/local/opt/[email protected]/bin/php /usr/local/bin/composer run-script test-php

[fballiano]

@fballiano - good observation. In my case PHP version installed by XAMPP is 8.1.6. Another condition to be evaluated and which creates confusion. We changed the minimum PHP requirement to version 7.3, but it is required >8.1, and this complicates things in the case of this PR.

that's because of the composer.lock... it's not an easy thing to fix unless we remove the composer.lock at all

[fballiano]

@fballiano I'm curious what happens if you put this in your composer.json file

"scripts": {
    "test-php": "php --version"
}

and run:

/usr/local/opt/[email protected]/bin/php /usr/local/bin/composer run-script test-php

well, my system wide "php" command is 7.4, but there's absolutely no reason why anything should test for it, it should only test the currently running version.

[justinbeaty]

well, my system wide "php" command is 7.4, but there's absolutely no reason why anything should test for it, it should only test the currently running version.

Right, but if composer invokes vendor/bin/phpcs from the scripts block, then it sounds like it's using the system wide php, which makes sense to me.

Edit: I mean makes sense to me in that composer would use the system wide PHP install, not that it should do that.

[fballiano]

composer install --no-dev works fine obviously but... I don't know...

[justinbeaty]

This would probably work on linux/macOS:

    "set-phpcs-paths": [
      "[ $COMPOSER_DEV_MODE -eq 0 ] || $(php -r 'echo PHP_BINARY;') vendor/bin/phpcs --config-set installed_paths vendor/phpcompatibility/php-compatibility,vendor/magento-ecg/coding-standard",
      "[ $COMPOSER_DEV_MODE -eq 0 ] || $(php -r 'echo PHP_BINARY;') vendor/bin/phpcs -i"
    ],

But I think we should use PHP scripts for composer's scripts block instead of bash. There you can write platform-independent code. Ideally for this PR it could be done via phpcs.xml but I'm still looking into it.

[addison74]

I first ran with composer.lock from the repo, but when I saw that an error appeared, I brought the whole directory to the previous stage then I deleted composer.lock. Unfortunately I didn't solve anything by running composer install. I will leave it to you to find a solution because you have experience with Composer and from other PRs.

[fballiano]

@justinbeaty you really are a machine, in the good way of course.
@addison74 generally speaking, if we don't need the "development" stuff, we should always run composer install --no-dev, less downloads and less requirements and it will work also for you :-)

[justinbeaty]

@justinbeaty you really are a machine, in the good way of course.

Wait, now I'm wondering if what I wrote actually does work. I think it would call the same 7.4 binary 😅.

However putting the script in dev/scripts and calling it via OpenMage\\Scripts\\Something::Run should definitely work.

[addison74]

On my Windows installation, look what happens when I run composer install --no-dev (without the file composer.lock existing in the directory).

[justinbeaty]

@addison74 this is still because the script uses bash syntax. It definitely needs to change.

[fballiano]

I thought it wouldn't be called at all, sorry for that. I shut up 🤐

[addison74]

@justinbeaty - I just wanted to show you that the --no-dev option can work, but not on Windows machines. If you fail to find a solution and if we keep this PR, we will have to mention somewhere that Composer does not work in Windows. That was my initial intention.

[justinbeaty]

Let's just wait for @sreichel and see if he knows about using phpcs.xml files. If not, then we can either remove the script from post-update-cmd and post-install-cmd and require it to be called via composer run-script set-phpcs-paths or that we need to use a platform independent PHP script that can configure phpcs without breaking windows.

[sreichel]

1. @justinbeaty phpcs.xml file will not help. It's for setting custom rulesets for sniffs.
2. @addison74 until its fixed, a workaround for windows could be adding --no-scripts and run the command manually.

I think that we need to have a platform independent composer script to run the phpcs config

Yep, but i'm not familar with bash scripts ... maybe @colinmollenhour can help?

[justinbeaty]

• @justinbeaty phpcs.xml file will not help. It's for setting custom rulesets for sniffs.

Okay, then I'll try and whip up a composer PHP script that should work.

Edit: #2467

[colinmollenhour]

Yep, but i'm not familar with bash scripts ... maybe @colinmollenhour can help?

I agree that running phpcs should be very easy so adding the dev dependencies and scripts makes sense. But, I would be in favor of removing the "post-*" hooks so users can run these manually if they actually want to use these tools, but otherwise there is no additional complication for using composer just to manage dependencies.

Regarding supporting multiple platforms, I support it when it's easy but otherwise I'd like to see support focused around Docker since that is really the only practical platform-agnostic approach. So brew, XAMPP, yum, etc. should all be considered marginally supported and Docker should be the favored method. This way things like running code quality tools are exactly the same for all users because they are run inside a container. E.g. after installing Docker for Windows|Mac, which is super easy, one could simply run a command which invokes Docker which then invokes phpcs and the result is the same regardless of the dev's platform specifics.

That is, all commonly used commands could be run through composer scripts with aliases:

$ composer run-script set-phpcs-paths
$ composer run-script phpcs
$ composer run-script phpcs-fix

This would work with system-installed php if it was similar to the Docker environment, but the "composer" command itself does not need to use a system-installed php, it can be a wrapper for a docker command:

Here is an example from my .bashrc, although I would propose putting this in a script like shell/composer.sh and shell/composer.bat to make it available to all:

if ! command -v composer >/dev/null; then
  alias composer='docker run --rm -it -u $(id -u):$(id -g) -v ${COMPOSER_HOME:-$HOME/.composer}:/tmp -v $(pwd):/app composer:latest'
fi

It may be necessary to build a custom composer container for openmage that extends the official composer one by adding some dependencies but that would be easy.

[sreichel]

Regarding supporting multiple platforms, I support it when it's easy but otherwise I'd like to see support focused around Docker since that is really the only practical platform-agnostic approach. So brew, XAMPP, yum, etc. should all be considered marginally supported and Docker should be the favored method.

I thought this one-line-command could be converted easily. :)

I'd also prefer a more "docker focused" development, but #2466 shows that it is not so common. It would not came to my mind that we need something like a release-builder (#2165). My expectation was that most would use e.g. composer.

Working with "plain" docker also requires some knowledge. Maybe we make some docs to a ddev based setup, thats easy to use.