Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgraded phpstan dependency to 1.1 #34555

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Nov 6, 2021

Description (*)

PHPstan version 1.0 got released earlier this week: https://phpstan.org/blog/phpstan-1-0-released
This PR changes the dependency constraints for phpstan.
This also changes the deprecated excludes_analyse config to excludePaths.

Related Pull Requests

https://github.com/magento/partners-magento2ee/pull/650

Fixed Issues (if relevant)

  1. Update phpstan/phpstan composer dependency to v1.x #34604

Manual testing scenarios (*)

  1. Run phpstan on various php files to see if it still works without issues, for example:
./vendor/bin/phpstan analyse --level 1 --no-progress --memory-limit=4G --configuration ./dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon setup/src/Magento/Setup/Test/Unit/Console/Style/MagentoStyleTest.php

(this command comes from #30581 (comment), and also see: https://github.com/magento/magento2/blob/2.4-develop/dev/tests/static/framework/Magento/TestFramework/CodingStandard/Tool/PhpStan.php#L81-L92)

Questions or comments

Should we still allow version 0.12.x of phpstan to get installed as well, or is that not necessary?

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Nov 6, 2021

Hi @hostep. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE,
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@hostep
Copy link
Contributor Author

hostep commented Nov 6, 2021

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@ihor-sviziev
Copy link
Contributor

@xmav can we add it to the platform health?

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

3 similar comments
@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-9331 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@xmav
Copy link
Contributor

xmav commented Nov 8, 2021

Hi @ihor-sviziev
Yes this could be part of Platform Health. I saw version 1.1.1 - https://github.com/phpstan/phpstan/releases
Could we try it ?

@xmav
Copy link
Contributor

xmav commented Nov 8, 2021

(please keep target branch to 2.4-develop)

Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hostep,
Could you update phpstan to latest available version (1.1.1 now)?

@hostep
Copy link
Contributor Author

hostep commented Nov 8, 2021

Done!

On my local machine, I do get this error after running that command mentioned above twice, the first time it works fine though, which is a bit weird?

➜ ./vendor/bin/phpstan analyse --level 1 --no-progress --memory-limit=4G --configuration ./dev/tests/static/testsuite/Magento/Test/Php/_files/phpstan/phpstan.neon setup/src/Magento/Setup/Test/Unit/Console/Style/MagentoStyleTest.php

In File.php line 208:

  Failed to open stream hoa://Library/Regex/Grammar.pp.

Entire stacktrace:

Exception trace:
  at vendor/hoa/file/File.php:208
 Hoa\File\File->_open() at vendor/hoa/file/Read.php:105
 Hoa\File\Read->_open() at vendor/hoa/stream/Stream.php:219
 Hoa\Stream\Stream::_getStream() at vendor/hoa/stream/Stream.php:297
 Hoa\Stream\Stream->open() at vendor/hoa/stream/Stream.php:178
 Hoa\Stream\Stream->__construct() at vendor/hoa/file/File.php:181
 Hoa\File\File->__construct() at vendor/hoa/file/Read.php:66
 Hoa\File\Read->__construct() at /private/var/folders/wr/w5n5bdgn1pb3wxws02x83qch0000gn/T/phpstan/cache/nette.configurator/Container_e87f1fc81a.php:5285
 Container_e87f1fc81a->createServiceRegexGrammarStream() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:157
 _PHPStan_9b5387833\Nette\DI\Container->createService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:90
 _PHPStan_9b5387833\Nette\DI\Container->getService() at /private/var/folders/wr/w5n5bdgn1pb3wxws02x83qch0000gn/T/phpstan/cache/nette.configurator/Container_e87f1fc81a.php:5291
 Container_e87f1fc81a->createServiceRegexParser() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:157
 _PHPStan_9b5387833\Nette\DI\Container->createService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:90
 _PHPStan_9b5387833\Nette\DI\Container->getService() at /private/var/folders/wr/w5n5bdgn1pb3wxws02x83qch0000gn/T/phpstan/cache/nette.configurator/Container_e87f1fc81a.php:3209
 Container_e87f1fc81a->createService033() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:157
 _PHPStan_9b5387833\Nette\DI\Container->createService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:90
 _PHPStan_9b5387833\Nette\DI\Container->getService() at /private/var/folders/wr/w5n5bdgn1pb3wxws02x83qch0000gn/T/phpstan/cache/nette.configurator/Container_e87f1fc81a.php:3028
 Container_e87f1fc81a->createService024() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:157
 _PHPStan_9b5387833\Nette\DI\Container->createService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:90
 _PHPStan_9b5387833\Nette\DI\Container->getService() at /private/var/folders/wr/w5n5bdgn1pb3wxws02x83qch0000gn/T/phpstan/cache/nette.configurator/Container_e87f1fc81a.php:3172
 Container_e87f1fc81a->createService030() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:157
 _PHPStan_9b5387833\Nette\DI\Container->createService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:90
 _PHPStan_9b5387833\Nette\DI\Container->getService() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/nette/di/src/DI/Container.php:177
 _PHPStan_9b5387833\Nette\DI\Container->getByType() at phar://vendor/phpstan/phpstan/phpstan.phar/src/DependencyInjection/Nette/NetteContainer.php:36
 PHPStan\DependencyInjection\Nette\NetteContainer->getByType() at phar://vendor/phpstan/phpstan/phpstan.phar/src/DependencyInjection/MemoizingContainer.php:37
 PHPStan\DependencyInjection\MemoizingContainer->getByType() at phar://vendor/phpstan/phpstan/phpstan.phar/src/Command/AnalyseCommand.php:138
 PHPStan\Command\AnalyseCommand->execute() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Command/Command.php:228
 _PHPStan_9b5387833\Symfony\Component\Console\Command\Command->run() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php:856
 _PHPStan_9b5387833\Symfony\Component\Console\Application->doRunCommand() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php:237
 _PHPStan_9b5387833\Symfony\Component\Console\Application->doRun() at phar://vendor/phpstan/phpstan/phpstan.phar/vendor/symfony/console/Application.php:138
 _PHPStan_9b5387833\Symfony\Component\Console\Application->run() at phar://vendor/phpstan/phpstan/phpstan.phar/bin/phpstan:82
 _PHPStan_9b5387833\{closure}() at phar://vendor/phpstan/phpstan/phpstan.phar/bin/phpstan:83
 require() at vendor/phpstan/phpstan/phpstan:8

@ihor-sviziev
Copy link
Contributor

@magento run all tests

@andrewbess
Copy link

I have fixed the merge-conflicts here

@andrewbess
Copy link

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@andrewbess
Copy link

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@andrewbess
Copy link

@magento run Unit Tests with env PHP 8.1 with edition CE without extensions magento-commerce/inventory, magento-commerce/adobe-stock-integration, magento-commerce/security-package, magento-commerce/magento2-page-builder, magento-commerce/adobe-ims

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@xmav
Copy link
Contributor

xmav commented Nov 10, 2021

works fine with a big change set. Tested on 2.4(2.4.3)<=>2.4-develop (2.4...2.4-develop)

@xmav
Copy link
Contributor

xmav commented Nov 10, 2021

@magento import code to Magento-trigger/magento2ce

@m2-github-services
Copy link
Collaborator

@xmav an error occurred during the Pull Request import.

@xmav
Copy link
Contributor

xmav commented Nov 10, 2021

@magento import code to Magento-trigger/magento2ce

@m2-github-services
Copy link
Collaborator

@xmav the branch with the code is successfully imported into the Magento-trigger/magento2ce repository. Branch name: imported-hostep-magento2-34555.

@xmav
Copy link
Contributor

xmav commented Nov 11, 2021

Hi @hostep @andrewbess
I would ask for one more change: change version constrain to ~1.1.0.
Other than that it looks good

Thanks!

@andrewbess
Copy link

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@andrewbess
Copy link

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@xmav
Copy link
Contributor

xmav commented Nov 11, 2021

Functional Tests issue unrelated to changes in this PR.

@xmav
Copy link
Contributor

xmav commented Nov 11, 2021

@magento import code to Magento-trigger/magento2ce

@m2-github-services
Copy link
Collaborator

@xmav the branch with the code is successfully imported into the Magento-trigger/magento2ce repository. Branch name: imported-hostep-magento2-34555.

@hostep
Copy link
Contributor Author

hostep commented Nov 12, 2021

@xmav:

I would ask for one more change: change version constrain to ~1.1.0.

Why? Have you read the backwards compatibility promise for developers and for users?

This article talks about classes, interfaces, and methods that can be safely used by extension developers without the risk of breaking in minor versions.

@magento-engcom-team magento-engcom-team merged commit a67c012 into magento:2.4-develop Nov 12, 2021
@xmav
Copy link
Contributor

xmav commented Nov 12, 2021

Hi @hostep
Thank you for pointing to that article. Main background under that is to have builds stable. We may change constraints to be more loose over time.

@hostep
Copy link
Contributor Author

hostep commented Nov 12, 2021

Ok thanks, but the problem is that I'll have to send in yet another PR within a week or 2 when version 1.2.0 gets released, so this will be a maintenance nightmare if we want to stay close to the latest version of phpstan I'm afraid.
I can see why you want those constraints set like this for packages where it's not clear how they deal with semantic versioning or backwards compatibilities, but in this case it's pretty clear.

@xmav
Copy link
Contributor

xmav commented Nov 12, 2021

even if we will use loose constraint someone should do composer update and push updated composer.lock file to use new version.

@hostep
Copy link
Contributor Author

hostep commented Nov 13, 2021

@xmav: Exactly, and that's a good thing!

So this would allow core Magento to only update phpstan when you want to, but will allow the users of the Magento software to still update phpstan to newer versions, because the users might want to stay closer to the latest version to benefit from new features and bugfixes.

@hostep
Copy link
Contributor Author

hostep commented Nov 18, 2021

@xmav, As predicted, phpstan 1.2.0 was released today: https://github.com/phpstan/phpstan/releases/tag/1.2.0

Since it contains support for new PHP 8.1 features, I believe we should probably upgrade it again over here ... (and maybe this time use ^ instead of ~ notation in the constraints ... 😉 )
If somebody is willing to do so, please do, I'm leaving in holiday for the next 3 weeks, so I won't have time to look into this myself.

Thanks!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: ready for testing Project: Platform Health Release Line: 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants