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

PHP 8.1 Upgrade #5611

Merged
merged 101 commits into from
Sep 23, 2022
Merged

PHP 8.1 Upgrade #5611

merged 101 commits into from
Sep 23, 2022

Conversation

joewhitsitt
Copy link
Contributor

@joewhitsitt joewhitsitt commented Aug 24, 2022

Relates to #5315
Resolves #5708

To do

History

Please track any migration comparison operators of old migrations here:
#5708

Setup

It is recommended that your Docker and DDEV are updated:

  • For docker, click the menu bar icon to download and install an update.
  • brew upgrade ddev if you installed via homebrew

To Test

  • pull
  • ddev restart
  • ddev composer install
  • ddev auth ssh
  • ddev ssh
  • php -v and confirm 8.1 or greater

More Testing

Semi-related, homebrew users should brew install php@8.1 on their machines and switch to it at some point.
Afterward, update your PHPStorm > Preferences > PHP > CLI Interpreter

@joewhitsitt joewhitsitt added the WIP Work in progress label Aug 24, 2022
@joewhitsitt
Copy link
Contributor Author

Going to try switching to jammy for PHP 8.1

@joewhitsitt
Copy link
Contributor Author

neutron/temporary-filesystem         2.4    requires  php (^5.6 || ^7.0)    
php-ffmpeg/php-ffmpeg                v0.14  requires  php (^5.3.9 || ^7.0)  
richardbporter/drush-users-commands  3.0.2  requires  php (^7.4)

@joewhitsitt
Copy link
Contributor Author

@pyrello
Copy link
Contributor

pyrello commented Sep 13, 2022

@pyrello do you know if this is still needed?

https://github.com/uiowa/uiowa/blob/main/docroot/modules/custom/uiowa_search/composer.json

I think this is an artifact of when it was a standalone repo. I don't think it's needed anymore.

@joewhitsitt
Copy link
Contributor Author

yay, travis went farther. running into this now: #5315 (comment)

@pyrello
Copy link
Contributor

pyrello commented Sep 13, 2022

I think we need to merge in #5625 either to main or specifically to this branch to make sure we are getting all the correct composer dependency versions.

@pyrello
Copy link
Contributor

pyrello commented Sep 13, 2022

It looks like the version of phpstan/phpdoc-parser in #5625 is set to 1.8, which is not compatible with everything else. When merging in #5625 and resolving conflicts in composer.lock, accept the changes for phpstan/phpdoc-parser in this branch. Otherwise, you're going to be in composer incompatible dependency hell.

@GaryRidgway GaryRidgway removed their assignment Sep 21, 2022
@joewhitsitt joewhitsitt changed the title Prepare for PHP 8.1 upgrade PHP 8.1 Upgrade Sep 21, 2022
@joewhitsitt joewhitsitt removed the WIP Work in progress label Sep 21, 2022
@joewhitsitt joewhitsitt marked this pull request as ready for review September 21, 2022 17:06
@joewhitsitt
Copy link
Contributor Author

The #blt Drupal Slack channel helped again. This is now properly auto deploying to DEV. Moving forward with upgrading to php 8.1. This PR is ready for review.

@pyrello pyrello self-assigned this Sep 22, 2022
@pyrello
Copy link
Contributor

pyrello commented Sep 22, 2022

Pulling this to start testing it

@pyrello
Copy link
Contributor

pyrello commented Sep 22, 2022

Updated testing instructions: ddev restart has to happen prior to ddev composer install because of composer PHP constraint added to composer.json.

Copy link
Contributor

@pyrello pyrello left a comment

Choose a reason for hiding this comment

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

I have reviewed all file changes and this is good to go from that perspective.

@pyrello
Copy link
Contributor

pyrello commented Sep 22, 2022

Review that things previously restricted to Admins only are still working (they all use the same access check) so check to see that the advanced fontawesome icon options aren't visible to webmasters on event/timeline block.

@joewhitsitt What I am seeing is that the advanced options are visible but greyed out. Is that what is intended?

@joewhitsitt
Copy link
Contributor Author

Correct. Additional Font Awesome Settings fields should be disabled

@pyrello
Copy link
Contributor

pyrello commented Sep 22, 2022

I found two issues while testing:

  • List block headlines were not honoring the "visually hide title" setting
  • Replicate was broken because of the check to prevent deleting the current node.

I fixed both issues.

Copy link
Contributor

@pyrello pyrello left a comment

Choose a reason for hiding this comment

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

After reviewing and testing this update both locally and on dev, I think that it looks good.

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

Successfully merging this pull request may close these issues.

Migration comparison operators
3 participants