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

Code validation of upgrade to PHP 7.4 #7582

Closed
3 tasks done
jkalexander7 opened this issue Jan 13, 2022 · 13 comments · Fixed by #7684
Closed
3 tasks done

Code validation of upgrade to PHP 7.4 #7582

jkalexander7 opened this issue Jan 13, 2022 · 13 comments · Fixed by #7684
Assignees
Labels
DevOps CMS team practice area Platform CMS Team

Comments

@jkalexander7
Copy link

jkalexander7 commented Jan 13, 2022

User Story or Problem Statement

PHP 7.3 will be end of life December 2021. Upgrade to PHP 7.4 is needed prior to making updates to BRD system with AL2.
Previous upgrade: (#1004)

Acceptance Criteria

  • All modules are compatible with PHP 7.4
  • There are no new PHP notices when running CICD tests
  • Upgrade tugboat, lando, and ddev to PHP 7.4
@jkalexander7 jkalexander7 added DevOps CMS team practice area Needs refining Issue status Platform CMS Team labels Jan 13, 2022
@jkalexander7
Copy link
Author

@jkalexander7 jkalexander7 changed the title Upgrade PHP 7.3 to 7.4 Code validation of upgrade to PHP 7.4 Jan 13, 2022
@cweagans
Copy link
Contributor

@jkalexander7 jkalexander7 removed the Needs refining Issue status label Jan 13, 2022
@indytechcook
Copy link
Contributor

indytechcook commented Jan 13, 2022

Code Compatibility test: docker run --rm -v $(pwd):/app vfac/php7compatibility 7.4 -d memory_limit=1G

@indytechcook
Copy link
Contributor

indytechcook commented Jan 13, 2022

Reading down the thread, #1004 was originally a 7.4 ticket and we did all the code combability checks and fixes them. It shifted to 7.3 when we had issues with drush and devshop.

@indytechcook
Copy link
Contributor

PHPStan version check: https://phpstan.org/config-reference#phpversion

@cweagans
Copy link
Contributor

In the Tugboat config, there's a reference to #6216 which may prevent us from immediately upgrading Tugboat to PHP 7.4. @ElijahLynn do you know if we've upgraded our Tugboat Docker version from 19 to 20? I see #6223 is still open - I guess that was a followup?

@ndouglas
Copy link
Contributor

ndouglas commented Jan 19, 2022

Confirmed that the host is still running Docker 19:

sh-4.2$ sudo docker version
Client:
 Version:           19.03.13-ce
 API version:       1.40
 Go version:        go1.13.15
 Git commit:        4484c46
 Built:             Mon Nov  2 19:22:46 2020
 OS/Arch:           linux/amd64
 Experimental:      false

Server:
 Engine:
  Version:          19.03.13-ce
  API version:      1.40 (minimum version 1.12)
  Go version:       go1.13.15
  Git commit:       4484c46
  Built:            Mon Nov  2 19:24:15 2020
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.4.1
  GitCommit:        c623d1b36f09f8ef6536a057bd658b3aa8632828
 runc:
  Version:          1.0.0-rc92
  GitCommit:        ff819c7e9184c13b7c2607fe6c30ae19403a7aff
 docker-init:
  Version:          0.19.0
  GitCommit:        de40ad0

@ndouglas
Copy link
Contributor

I ran phpstan analyze with the PHP version set to 7.4 (thanks, Neil!) and other options unchanged and no errors were reported.

Screen Shot 2022-01-19 at 8 49 00 AM

I commented out the excluded tests and included contrib modules:

Screen Shot 2022-01-19 at 8 56 05 AM

and reran:

Screen Shot 2022-01-19 at 8 55 45 AM

This was a little noisier:

Screen Shot 2022-01-19 at 8 57 56 AM

Most of these were for unknown methods and properties on objects, which is a consequence of idiomatic Drupal development. I ignore them!

Screen Shot 2022-01-19 at 9 17 27 AM

and that cuts the errors almost in half.

Screen Shot 2022-01-19 at 9 18 06 AM

I scanned for "deprecated" but all results were of three groups:

  • deprecated in Drupal 8.x/9.x, removed in 10.0.0
  • deprecated in Symfony 4.x
  • deprecated in PHPUnit

I didn't find any references to deprecations in PHP itself. Looking at the 7.4 deprecations, this makes sense -- none jump out at me as things I've seen in our codebase.

(and some things like nested ternary conditionals, given PHP <= 8.0.0's unconventional associativity would definitely jump out at me)

I also reviewed the baseline file and observed that none of its listed errors could confound detection of any new issues with PHP 7.4.

This is just a first step, of course, but so far I haven't turned up any issues.

@ndouglas
Copy link
Contributor

I expected PHPUnit to have the highest impact for the shortest time investment, so I ran it first:

Testing tests/phpunit
..F.....tests\phpunit\Content\CreateMediaTest::testCreateMedia
Operation took 0.416 seconds compared to the benchmark of 2 seconds.
.......................................................  63 / 657 (  9%)
............................................................... 126 / 657 ( 19%)
............................................................... 189 / 657 ( 28%)
............................................................... 252 / 657 ( 38%)
............................................................... 315 / 657 ( 47%)
............................................................... 378 / 657 ( 57%)
............................................................... 441 / 657 ( 67%)
............................................................... 504 / 657 ( 76%)
............................................................... 567 / 657 ( 86%)
...............................................tests\phpunit\Performance\CreateNodeTest::testCreateNodePerformance
Operation took 0.633 seconds compared to the benchmark of 5 seconds.
.tests\phpunit\Performance\CreateTaxonomyTermTest::testCreateTaxonomyTerm
Operation took 0.175 seconds compared to the benchmark of 2 seconds.
.tests\phpunit\Performance\EditNodeTest::testEditNodePerformance
Operation took 0.049 seconds compared to the benchmark of 5 seconds for type page.
.tests\phpunit\Performance\EditNodeTest::testEditNodePerformance
Operation took 0.063 seconds compared to the benchmark of 5 seconds for type landing_page.
.tests\phpunit\Performance\LoginTest::testLoginPerformance
Operation took 0.763 seconds compared to the benchmark of 3 seconds.
.tests\phpunit\Performance\ScalabilityCreateNodeTest::testScalabilityCreateNodeTest
Operation took 3.14 and completed 17 iterations.
........... 630 / 657 ( 95%)
...........................                                     657 / 657 (100%)

Time: 15.21 minutes, Memory: 1.31 GB

There was 1 failure:

1) tests\phpunit\API\PostApiQueueTest::testPostApiQueueProcessing
POST request is expected to fail with 404 due to incomplete endpoint URL.
Failed asserting that 0 matches expected 404.

/app/docroot/vendor/phpunit/phpunit/src/Framework/Constraint/IsEqual.php:100
/app/tests/phpunit/API/PostApiQueueTest.php:87
/app/docroot/vendor/phpunit/phpunit/src/Framework/TestResult.php:703
/app/docroot/vendor/phpunit/phpunit/src/Framework/TestSuite.php:627
/app/docroot/vendor/phpunit/phpunit/src/Framework/TestSuite.php:627
/app/docroot/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:656
/app/docroot/vendor/phpunit/phpunit/src/TextUI/Command.php:235
/app/docroot/vendor/phpunit/phpunit/src/TextUI/Command.php:194
phpvfscomposer:///app/docroot/vendor/phpunit/phpunit/phpunit:97

FAILURES!
Tests: 657, Assertions: 1398, Failures: 1.

This test fails in local dev for reasons unrelated to the 7.4 upgrade.

I'll run the entire test suite now.

@ndouglas
Copy link
Contributor

ndouglas commented Jan 19, 2022

The issue mentioned above with Alpine and PHP 7.4 might not be relevant since the Tugboat PHP container does not use Alpine, but Debian. Unless I'm missing something.

EDIT: See followup issue #7667.

EDIT 2: This is apparently still relevant because the issue seems to be with Moby and not related to Alpine Linux.

@indytechcook
Copy link
Contributor

@cweagans @ndouglas would you agree with the tests passing on tugboat and the static code scans passing we can close this issue?

@ndouglas
Copy link
Contributor

@indytechcook I think it's sufficient to close it. I have a PR open for the actual code updates for Tubgoat/Lando/DDEV, but they're trivial obv.

@indytechcook
Copy link
Contributor

I had thought we had a follow up issue to actually make the change in tugboat/lando/ddev but it's actually an AC on this issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DevOps CMS team practice area Platform CMS Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants