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 (*)

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

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 3de76e23. ± Comparison against base commit 9b656d88.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 925260ca. ± Comparison against base commit 9b656d88.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 925260ca. ± Comparison against base commit 9b656d88.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit 515afb50. ± Comparison against base commit 9b656d88.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit bfb29952. ± Comparison against base commit 9b656d88.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit f61d678b. ± Comparison against base commit ff13954e.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit f61d678b. ± Comparison against base commit ff13954e.

[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.

[github-actions[bot]]

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  5 :heavy_check_mark: ±0  2 :zzz: ±0  0 :x: ±0 

Results for commit affbdeb2. ± Comparison against base commit ff13954e.

[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.