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

Allow higher versions of phpstan to be installed (Magento 2.4 edition) #31972

Closed

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Feb 4, 2021

(cherry picked from commit 075a1b2)

Description (*)

This is the exact same change as #30581, but this one targets the 2.4-develop branch and not the php8-develop branch.

My original intention was to introduce this change in the 2.4 codebase. However it was changed to the php8 branch because it fitted in that context as well. But I'm afraid that the php8 branch will not get merged soon enough in 2.4-develop (or even not at all, they aren't sure yet), so I decided to create a second PR now targeting the branch I originally wanted to see this change in.

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Mismatch between requirements bitExpert/phpstan-magento#42: Mismatch between requirements

Manual testing scenarios (*)

  1. Somehow, tell phpstan to run on the Magento 2 source code and it should run without problems (it might detect more/different issues, and that is expected)

Questions or comments

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)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Feb 4, 2021

Hi @hostep. Thank you for your contribution
Here is 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

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, 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, please 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 hostep changed the title Allow higher versions of phpstan to be installed. Allow higher versions of phpstan to be installed (Magento 2.4 edition) Feb 4, 2021
@ihor-sviziev ihor-sviziev self-assigned this Feb 5, 2021
@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev
Copy link
Contributor

@sivaschenko AFAIK php8-develop branch will be merged to 2.4-develop after some time. Right? Should we accept this PR?

@hostep
Copy link
Contributor Author

hostep commented Feb 5, 2021

I'd prefer it if this makes it in Magento 2.4.3, if we need to wait on php8 getting merged, it will happen a lot later?

@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests CE, Functional Tests EE

@ihor-sviziev ihor-sviziev added Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Feb 8, 2021
@ihor-sviziev ihor-sviziev added Progress: review and removed Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. labels Feb 9, 2021
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8748 has been created to process this Pull Request

@hostep
Copy link
Contributor Author

hostep commented Feb 16, 2021

This should probably be put on hold until a solution is found for #30581 (comment)

@ihor-sviziev
Copy link
Contributor

@hostep looks like the listed issue was fixed in #32215. I believe it's better to port it together. Don't you think so?

@ihor-sviziev ihor-sviziev mentioned this pull request Mar 30, 2021
16 tasks
@hostep
Copy link
Contributor Author

hostep commented Mar 30, 2021

Indeed! I'll try to have a look later this week to include those commits as well here.

@hostep hostep force-pushed the relax-phpstan-constraint-24 branch from 137989f to c906698 Compare March 30, 2021 19:44
@hostep
Copy link
Contributor Author

hostep commented Mar 30, 2021

Rebased, cherry-picked the 2 commits from #32215, and force pushed

@magento run all tests

Warning: when you'll run composer install with this commit, it will output Warning: The lock file is not up to date with the latest changes in composer.json. You may be getting outdated dependencies. It is recommended that you run 'composer update' or 'composer update <package name>'., this is a huge problem currently on the 2.4-develop branch where the composer.lock file is missing a bunch of dependencies that should be there based on the contents of the composer.json file, it has something to do with that web-token/jwt-framework package.
I'll rebase again, once somebody (hopefully soon) will fix that problem.

@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests EE, WebAPI Tests

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-8748 has been created to process this Pull Request

@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests EE

@gabrieldagama
Copy link
Contributor

Hi @hostep, we are currently in the process of merging the php8-develop branch into the 2.4-develop, so I believe this PR will be redundant.

Closing this PR for now since that should be done soon.

Thanks!

@m2-assistant
Copy link

m2-assistant bot commented Apr 6, 2021

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@hostep
Copy link
Contributor Author

hostep commented Apr 6, 2021

Ah okay, good to know, thanks for letting me know @gabrieldagama! This means Magento 2.4.3 will ship with this change and will be compatible with PHP 8.0, right? Nice!

@ihor-sviziev
Copy link
Contributor

@hostep, I don't believe it will be fully compatible with PHP 8, but at least we'll have much fewer dependencies that do not allow using PHP 8.

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 Award: bug fix Release Line: 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mismatch between requirements
5 participants