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

Support PHP 8.0 #1266

Merged
merged 20 commits into from
Feb 14, 2022
Merged

Support PHP 8.0 #1266

merged 20 commits into from
Feb 14, 2022

Conversation

osma
Copy link
Member

@osma osma commented Jan 20, 2022

Reasons for creating this PR

Trying to get Skosmos working on PHP 8.0

Link to relevant issue(s), if any

Description of the changes in this PR

  • upgrade willdurand/negotiation to 3.0.* which supports PHP8
  • reenable GitHub Actions CI jobs for PHP 8.0
  • upgrade php-actions/phpunit to v3 which is slightly newer than what we used before

Known problems or uncertainties in this PR

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if not, explain why below)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

No new tests are necessary since there is no new functionality.

@osma osma self-assigned this Jan 20, 2022
@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #1266 (5e0de26) into master (52e7079) will not change coverage.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1266   +/-   ##
=========================================
  Coverage     69.33%   69.33%           
  Complexity     1648     1648           
=========================================
  Files            32       32           
  Lines          4047     4047           
=========================================
  Hits           2806     2806           
  Misses         1241     1241           
Impacted Files Coverage Δ
model/Vocabulary.php 84.67% <0.00%> (ø)
model/sparql/GenericSparql.php 92.24% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52e7079...5e0de26. Read the comment docs.

@osma osma changed the title Issue1243 support php80 Support PHP 8.0 Jan 20, 2022
@osma
Copy link
Member Author

osma commented Jan 21, 2022

This is quite difficult to untangle - there are many problems here and some unfortunate interactions between them that make debugging very hard. Things I've found out so far:

  • The PHPUnit configuration under GitHub Actions CI is not quite the same as when running PHPUnit locally. The CI environment wasn't using the phpunit.xml configuration file (now fixed in this PR) and also the error_reporting settings seem to be different. Basically, under CI the deprectation errors (E_DEPRECATED) were breaking tests, but this didn't happen when running test locally. These should be aligned - either deprecations should cause test failures everywhere (preferred) or in neither environment, if we decide so. Probably error_reporting should be set in tests/bootstrap.php so that it will be applied every time PHPUnit is run, regardless of environment.
  • there are some deprecation errors in the Skosmos codebase, as well as in the Mockery library, because PHP 8.0 deprecates the use of required parameters after optional parameters. These should probably be fixed in Skosmos code and the dependencies should be upgraded if possible.
  • The CI environment was using an old version of PHP (8.0.6) because it had created the php-build images (for Composer and PHPUnit separately) in May 2021 and cached them as packages in the Skosmos repository. I just deleted the PHP 8 ones (but not php 7.x which are similarly old), to force the use of a newer PHP. Locally I'm using PHP 8.0.14 and 8.0.15 seems to be the newest available 8.0 release. Update: Now the CI environment seems to be using PHP 8.1.1, which is not what I wanted at all! The ci.yml requests 8.0 which should use the newest available 8.0.* version.
  • The Http304 tests (which use ob_start) seem to be the ones getting stuck under CI. I disabled them (by skipping in setUp) in this PR for now.

@osma
Copy link
Member Author

osma commented Jan 21, 2022

Now the CI environment seems to be using PHP 8.1.1, which is not what I wanted at all! The ci.yml requests 8.0 which should use the newest available 8.0.* version.

This can be fixed by requesting a more specific version such as 8.0.15, see here

@osma
Copy link
Member Author

osma commented Jan 21, 2022

I think I've fixed most of the above:

  • PHP version numbers specified with quotes got me the right PHP 8.0 version (YAML parsing issue)
  • tests are running with the same error_reporting and deprecation policy (strict!) both locally and in CI
  • fixed a few cases of deprecated method parameters in Skosmos code
  • upgraded Mockery and DomCrawler to versions that work on PHP 8.0 (but also still support 7.2)
  • reenabled Http304 tests that were no longer getting stuck (but I never found out why)
  • disabled debug output for phpunit

The PHPUnit step is now passing under PHP 8.0 CI 🎉 - but there are still problems with the Code Climate test reporter on PHP 8.0, which causes CI jobs to fail. I'll leave sorting that out for later!

@osma osma added this to the 2.14 milestone Jan 24, 2022
@sonarcloud
Copy link

sonarcloud bot commented Jan 24, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot E 1 Security Hotspot
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma
Copy link
Member Author

osma commented Jan 24, 2022

I disabled Code Climate and Codecov reporters for PHP 8, since PHPUnit 8.5 won't collect coverage information for PHP 8 anyway. Now the PHP 8.0 CI jobs are succesful 🎉

I also added PHP 8.1 CI tests in experimental mode (permitted to fail). There are a lot of deprecation warnings causing nearly all tests to fail on PHP 8.1. The main culprit seems to be EasyRdf, which isn't yet compatible with PHP 8.1, but there's an open PR which should hopefully fix this before long.

@osma
Copy link
Member Author

osma commented Jan 24, 2022

All seems well now looking at the test results, but I still need to test actually running Skosmos on PHP 8.0, because test coverage is far from 100%. That might uncover some more problems.

@kinow
Copy link
Collaborator

kinow commented Jan 24, 2022

Tried running with Docker but something went wrong when starting the server.

diff --git a/dockerfiles/Dockerfile.ubuntu b/dockerfiles/Dockerfile.ubuntu
index d834ce42..cdf6600c 100644
--- a/dockerfiles/Dockerfile.ubuntu
+++ b/dockerfiles/Dockerfile.ubuntu
@@ -8,6 +8,8 @@ ARG SKOSMOS_VERSION=v.2.9
 
 ARG DEBIAN_FRONTEND=noninteractive
 
+RUN apt-get update && apt-get install -y software-properties-common && add-apt-repository -y ppa:ondrej/php
+
 # git is necessary for some composer packages e.g. davidstutz/bootstrap-multiselect
 # gettext is necessary as php-gettext was available in 18.04, but not in 20.04
 RUN apt-get update && apt-get install -y \
@@ -15,13 +17,13 @@ RUN apt-get update && apt-get install -y \
     curl \
     gettext \
     git \
-    libapache2-mod-php7.4 \
+    libapache2-mod-php8.0 \
     locales \
-    php7.4 \
-    php7.4-curl \
-    php7.4-xsl \
-    php7.4-intl \
-    php7.4-mbstring \
+    php8.0 \
+    php8.0-curl \
+    php8.0-xsl \
+    php8.0-intl \
+    php8.0-mbstring \
     php-apcu \
     php-zip \
     unzip \
@@ -55,7 +57,7 @@ ENV LC_ALL=en_US.UTF-8
 ENV LANG=en_US.UTF-8  
 
 # timezone
-RUN sed -i 's/;date.timezone =/date.timezone = "UTC"/g' /etc/php/7.4/apache2/php.ini
+RUN sed -i 's/;date.timezone =/date.timezone = "UTC"/g' /etc/php/8.0/apache2/php.ini
 
 COPY dockerfiles/config/000-default.conf /etc/apache2/sites-available/000-default.conf
error
Composer is operating significantly slower than normal because you do not have the PHP curl extension enabled.
No composer.lock file present. Updating dependencies to latest instead of installing from lock file. See https://getcomposer.org/install for more information.
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires PHP extension ext-intl * but it is missing from your system. Install or enable PHP's intl extension.
  Problem 2
    - Root composer.json requires PHP extension ext-xsl * but it is missing from your system. Install or enable PHP's xsl extension.
  Problem 3
    - Root composer.json requires easyrdf/easyrdf 1.1.* -> satisfiable by easyrdf/easyrdf[1.1.0, 1.1.1].
    - easyrdf/easyrdf[1.1.0, ..., 1.1.1] require ext-dom * -> it is missing from your system. Install or enable PHP's dom extension.
  Problem 4
    - phpunit/phpunit[8.5.0, ..., 8.5.11] require php ^7.2 -> your php version (8.1.1) does not satisfy that requirement.
    - phpunit/phpunit[8.5.12, ..., 8.5.23] require ext-dom * -> it is missing from your system. Install or enable PHP's dom extension.
    - Root composer.json requires phpunit/phpunit 8.5.* -> satisfiable by phpunit/phpunit[8.5.0, ..., 8.5.23].

To enable extensions, verify that they are enabled in your .ini files:
    - /etc/php/8.1/cli/php.ini
    - /etc/php/8.1/cli/conf.d/10-opcache.ini
    - /etc/php/8.1/cli/conf.d/10-pdo.ini
    - /etc/php/8.1/cli/conf.d/20-apcu.ini
    - /etc/php/8.1/cli/conf.d/20-calendar.ini
    - /etc/php/8.1/cli/conf.d/20-ctype.ini
    - /etc/php/8.1/cli/conf.d/20-exif.ini
    - /etc/php/8.1/cli/conf.d/20-ffi.ini
    - /etc/php/8.1/cli/conf.d/20-fileinfo.ini
    - /etc/php/8.1/cli/conf.d/20-ftp.ini
    - /etc/php/8.1/cli/conf.d/20-gettext.ini
    - /etc/php/8.1/cli/conf.d/20-iconv.ini
    - /etc/php/8.1/cli/conf.d/20-phar.ini
    - /etc/php/8.1/cli/conf.d/20-posix.ini
    - /etc/php/8.1/cli/conf.d/20-readline.ini
    - /etc/php/8.1/cli/conf.d/20-shmop.ini
    - /etc/php/8.1/cli/conf.d/20-sockets.ini
    - /etc/php/8.1/cli/conf.d/20-sysvmsg.ini
    - /etc/php/8.1/cli/conf.d/20-sysvsem.ini
    - /etc/php/8.1/cli/conf.d/20-sysvshm.ini
    - /etc/php/8.1/cli/conf.d/20-tokenizer.ini
    - /etc/php/8.1/cli/conf.d/20-zip.ini
You can also run `php --ini` in a terminal to see which files are used by PHP in CLI mode.
Alternatively, you can run Composer with `--ignore-platform-req=ext-intl --ignore-platform-req=ext-xsl --ignore-platform-req=ext-dom --ignore-platform-req=ext-dom` to temporarily ignore these required extensions.
Running update with --no-dev does not mean require-dev is ignored, it just means the packages will not be installed. If dev requirements are blocking the update you have to resolve those problems.

@osma
Copy link
Member Author

osma commented Jan 24, 2022

@kinow Thanks for testing! It seems to me based on the error messages that your Composer was running under PHP 8.1.1, not 8.0.x. Also it complained a lot about missing PHP extensions even though you clearly had those installed.

My guess is that installing php-apcu and php-zip will install PHP 8.1.1 and make that the default PHP version. Switching to php8.0-apcu and php8.0-zip should help in that case.

@kinow
Copy link
Collaborator

kinow commented Jan 26, 2022

My guess is that installing php-apcu and php-zip will install PHP 8.1.1 and make that the default PHP version. Switching to php8.0-apcu and php8.0-zip should help in that case.

You are right @osma. Changed that, and now I have Skosmos and PHP 8.0 :-) 🎉

image

@osma
Copy link
Member Author

osma commented Jan 26, 2022

You beat me to it @kinow ! Do tell if you find problems. I will also try running Skosmos itself under PHP 8.0 (not just the tests), but I haven't yet found the time to do it.

@osma
Copy link
Member Author

osma commented Feb 14, 2022

I tested this briefly on a VM running PHP 8.0.15. Everything seems to be working correctly and there are no suspicious warnings in the Apache error log. So I think it's pretty safe to merge this - if new problems turn up later we can deal with them separately.

@osma osma marked this pull request as ready for review February 14, 2022 15:13
@osma osma merged commit ddf35d7 into master Feb 14, 2022
@osma osma deleted the issue1243-support-php80 branch February 14, 2022 15:13
@osma
Copy link
Member Author

osma commented Feb 14, 2022

@kinow Now that we have PHP 8.0 support for Skosmos, should the Dockerfile be updated to use PHP 8.0 as well, the way you already tested it? Care to open a PR?

@kinow
Copy link
Collaborator

kinow commented Feb 14, 2022

@kinow Now that we have PHP 8.0 support for Skosmos, should the Dockerfile be updated to use PHP 8.0 as well, the way you already tested it? Care to open a PR?

Good idea, will try to send a quick PR this week. Excited about learning php8 with skosmos! Thanks Osma!

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

Successfully merging this pull request may close these issues.

Tests fail and CI hangs on PHP 8
2 participants