Skip to content

Updated sebastian/phpcpd, phpunit/phpunit and laminas/laminas-server dependency for php8 update #31383

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

Conversation

konarshankar07
Copy link
Contributor

@konarshankar07 konarshankar07 commented Dec 22, 2020

Description (*)

This PR will update the packages related to php8 compatibility

Fixed Issues (if relevant)

  1. Fixes Update laminas/laminas-server dependency version to "^2.9.1" #31347
  2. Fixes Update laminas/laminas-servicemanager dependency version to "^3.5.1" #31346
  3. Fixes Update laminas/laminas-code dependency version to "^3.5.1" #31345
  4. Fixes Update sebastian/phpcpd dependency version to "~6.0.3" #31344
  5. Fixes Update elasticsearch/elasticsearch dependency version to "~7.11.0" #31341
  6. Fixes Update laminas/laminas-feed dependency version to "^2.13.0" #31340
  7. Fixes Update laminas/laminas-mvc dependency to PHP 8 compatible version #31783

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 Dec 22, 2020

Hi @konarshankar07. 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 php8-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

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

@sivaschenko
Copy link
Member

@magento run all tests

Copy link
Contributor

@coderimus coderimus left a comment

Choose a reason for hiding this comment

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

Hello @konarshankar07
Thank you for the work you have done in the scope of the PHP8 compatibility. 👍

Please, take a look at my comment about the minimum-stability.

composer.json Outdated
@@ -385,5 +385,6 @@
"Magento\\PhpStan\\": "dev/tests/static/framework/Magento/PhpStan/"
}
},
"prefer-stable": true
"prefer-stable": true,
"minimum-stability": "dev"
Copy link
Contributor

Choose a reason for hiding this comment

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

We must avoid using the dev as a minimum-stability. Please, change.

Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request @konarshankar07 ! Can you please take a look at my comment

composer.json Outdated
@@ -385,5 +385,6 @@
"Magento\\PhpStan\\": "dev/tests/static/framework/Magento/PhpStan/"
}
},
"prefer-stable": true
"prefer-stable": true,
"minimum-stability": "dev"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the minimum stability should be dropped to dev. Is that required for some reason?

@sivaschenko
Copy link
Member

@magento run all tests

@konarshankar07
Copy link
Contributor Author

Hello @sivaschenko @coderimus
Minimum stability is set to dev for the elasticsearch/elasticsearch update
Thanks

@sivaschenko
Copy link
Member

@konarshankar07 I see, elasticsearch 7.11 is not yet published. Let's update the elasticsearch version later, when 7.11 will be available on packagist https://packagist.org/packages/elasticsearch/elasticsearch to keep all the dependencies stable.

@konarshankar07
Copy link
Contributor Author

@sivaschenko
Should we downgrade the elasticsearch for now or we need to wait for the release?

@coderimus
Copy link
Contributor

@konarshankar07 in our case we have to not to have any dev stability packages and taken into account the next 2 aspects we need to downgrade it to the stable one:

  • 7.11 release will not be soon and, possibly, during this time we will have other packages updated and, maybe, new conflicts, so it is better to have PHP8 compatible system now :) to unblock Magento 3rd party vendors to upgrade their packages if needed.
  • I think that the upgrade of the ES should be another complex task because ES is one of the critical packages which is responsible for data. It will need not only to upgrade version but also revied from the security perspective.
    cc @sivaschenko

@konarshankar07
Copy link
Contributor Author

@coderimus
Thanks for the response. What do you suggest to move forward with this PR?

Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Hi @konarshankar07 , please see a summary of changes required for the PR to move forward

composer.json Outdated
@@ -33,18 +33,18 @@
"colinmollenhour/credis": "1.11.1",
"colinmollenhour/php-redis-session-abstract": "~1.4.0",
"composer/composer": "^1.9",
"elasticsearch/elasticsearch": "~7.7.0",
"elasticsearch/elasticsearch": "~7.11.0",
Copy link
Member

Choose a reason for hiding this comment

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

Let's revert the elasticsearch version change

composer.json Outdated
@@ -385,5 +385,6 @@
"Magento\\PhpStan\\": "dev/tests/static/framework/Magento/PhpStan/"
}
},
"prefer-stable": true
"prefer-stable": true,
"minimum-stability": "dev"
Copy link
Member

Choose a reason for hiding this comment

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

this line should be removed

@konarshankar07
Copy link
Contributor Author

@sivaschenko
Thanks for the suggestion and I have reverted the elasticsearch update and removed the minimum stability.

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@coderimus
Copy link
Contributor

@magento run Functional Tests B2B, Functional Tests CE, Functional Tests EE, Integration Tests, Sample Data Tests CE , Static Tests, Unit Tests, WebAPI Tests

@coderimus
Copy link
Contributor

@sivaschenko @konarshankar07 I have things to discuss:

  1. I think we have to have PHP 8 instead of PHP 7.4 on our test builds. And we have to modify the composer required section: "php": "~7.3.0||~7.4.0|| ~8.0.0",

image

  1. I think we should raise a discussion about having the laminas/laminas-code 4.x.x in our project instead of the 3.x.x. From my perspective, having the 4.x.x is better because:
  • 4.x.x will have support for all new PHP8 features
  • It has PHP 7.4 as minimum supported version
  • of course fixes and improvements according to the CHANGE.log
  • having major version is good for future releases

This 4.0.0 version has some BC breaks and I think that they will not affect us or if so we will fix them :)

What do you think?

@konarshankar07
Copy link
Contributor Author

konarshankar07 commented Jan 6, 2021

Hello @coderimus
yes, I agree with you but it should be covered in different PR and its part of #31081 issue and I think @jamescowie is working on this update
For Ref:- #31269

@coderimus
Copy link
Contributor

coderimus commented Jan 6, 2021

@konarshankar07 thank you for details and link PHP 8 upgrade issue 👍 Now I have a more clear vision of the whole process.

From my perspective, the PHP 8 version set to the required section of the composer.json should be done in the scope of this issue because without it we are not able to say that updated versions, Magento core code and 3rd party vendors included to the package are really compatible. Because here the PHP version is a conductor and dependencies are followers. Also, the laminas/laminas-code has a new version with fixes, updates for the PHP 8 which makes having 3.x.x version is not required, as for me.

I can suggest next:

  • set PHP 8 requirement to the composer.json
  • check result
  • update laminas/laminas-code to the 4.x.x (fix BIC things)
  • update all other libraries if not done in step 2

With this flow, I think, we will have PHP 8 compatibility upgrade consistently. Otherwise, we provide not the php8 dependency upgrade but the simple dependency upgrade :)

What do you think about this? @konarshankar07 @sivaschenko

@sivaschenko
Copy link
Member

Hi @coderimus @konarshankar07

We will not be able to run tests on PHP 8 until all dependencies are updated. Let's do this step by step.
I think it would be better to update laminas components to the minimal version compatible with PHP 8 in the scope of PHP 8 compatibility project, to avoid additional work and BIC.

@coderimus
Copy link
Contributor

Hi @sivaschenko @konarshankar07

@sivaschenko thank you for the update strategy explanation. Got your point and agreed with it. Let's proceed as you suggested.

@sivaschenko
Copy link
Member

sivaschenko commented Jan 6, 2021

@konarshankar07 can you please check if the failed tests can be fixed.

I.e. it looks like lib/internal/Magento/Framework/composer.json should be updated as well to fix static tests

Also, can you please ensure composer.lock is updated only for the changed dependencies.

(run composer update specific package instead of general composer update)

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@konarshankar07 konarshankar07 marked this pull request as draft January 7, 2021 11:24
@konarshankar07
Copy link
Contributor Author

@magento run all tests

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@coderimus
Copy link
Contributor

@magento run all tests

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@konarshankar07 konarshankar07 changed the title Updated dependency for php8 update Updated sebastian/phpcpd, phpunit/phpunit and laminas/laminas-server dependency for php8 update Jan 29, 2021
@konarshankar07
Copy link
Contributor Author

@magento run Static Tests

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@konarshankar07
Copy link
Contributor Author

@magento run all tests

@m2-assistant
Copy link

m2-assistant bot commented Feb 17, 2021

Hi @konarshankar07, 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.

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

Successfully merging this pull request may close these issues.

4 participants