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

Add support for PHP 8.1 #1385

Merged
merged 3 commits into from
Feb 9, 2023
Merged

Add support for PHP 8.1 #1385

merged 3 commits into from
Feb 9, 2023

Conversation

kouralex
Copy link
Contributor

@kouralex kouralex commented Nov 3, 2022

Reasons for creating this PR

Skosmos currently supports PHP versions 7.3 - 8.0 even though 7.3 EOL has already been reached and 7.4 is nearing its own end, see https://www.php.net/supported-versions.php. Therefore, there is a growing need of new version support.

Description of the changes in this PR

This PR straighforwardly fixes incompatibility issues (and deprecation notices) encountered whilst developing and testing Skosmos with PHP 8.1. I used the proposed solution described in #1274 (comment) to get past EasyRdf-related issues. Closes #1274

Known problems or uncertainties in this PR

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@kouralex kouralex added enhancement maintenance Dependency changes, security updates, infrastructure tweaks & general mainenance labels Nov 3, 2022
@kouralex kouralex added this to the Next Tasks milestone Nov 3, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@kouralex kouralex requested a review from osma November 3, 2022 20:12
@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 71.35% // Head: 69.55% // Decreases project coverage by -1.81% ⚠️

Coverage data is based on head (417ae45) compared to base (b13b2c4).
Patch coverage: 42.30% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1385      +/-   ##
============================================
- Coverage     71.35%   69.55%   -1.81%     
  Complexity     1650     1650              
============================================
  Files            32       32              
  Lines          3774     4257     +483     
============================================
+ Hits           2693     2961     +268     
- Misses         1081     1296     +215     
Impacted Files Coverage Δ
controller/RestController.php 22.85% <0.00%> (+0.58%) ⬆️
controller/WebController.php 15.59% <0.00%> (-1.73%) ⬇️
model/sparql/GenericSparql.php 92.52% <ø> (+1.24%) ⬆️
controller/Controller.php 57.36% <14.28%> (ø)
model/Request.php 71.91% <40.00%> (ø)
model/Concept.php 83.66% <100.00%> (-2.75%) ⬇️
model/ConceptSearchParameters.php 85.55% <100.00%> (ø)
model/Model.php 80.81% <100.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@joelit
Copy link
Contributor

joelit commented Nov 4, 2022

Hooray for sweetrdf! Core functionalities need to be checked to be sure there are no regressions - I don't 100% trust the coverage of tests in this case.

Copy link
Contributor

@joelit joelit left a comment

Choose a reason for hiding this comment

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

Excellent work! :)

I gave this a thorough run-through, and here are my findings:

But there is something wrong with library dependencies to AssetCollection for robloach/component-installer:

Deprecation Notice: Return type of Assetic\Asset\AssetCollection::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/AssetCollection.php:215
Deprecation Notice: Return type of Assetic\Filter\FilterCollection::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Filter/FilterCollection.php:73
Deprecation Notice: Return type of Assetic\Filter\FilterCollection::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Filter/FilterCollection.php:78
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionFilterIterator::getChildren() should either be compatible with RecursiveFilterIterator::getChildren(): ?RecursiveFilterIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionFilterIterator.php:80
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionFilterIterator::accept() should either be compatible with FilterIterator::accept(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionFilterIterator.php:50
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::hasChildren() should either be compatible with RecursiveIterator::hasChildren(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:104
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::getChildren() should either be compatible with RecursiveIterator::getChildren(): ?RecursiveIterator, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:112
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::current($raw = false) should either be compatible with Iterator::current(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:54
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::next() should either be compatible with Iterator::next(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:89
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::key() should either be compatible with Iterator::key(): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:84
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::valid() should either be compatible with Iterator::valid(): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:99
Deprecation Notice: Return type of Assetic\Asset\Iterator\AssetCollectionIterator::rewind() should either be compatible with Iterator::rewind(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:94
Deprecation Notice: strrpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /var/www/html/Skosmos/vendor/kriswallsmith/assetic/src/Assetic/Asset/Iterator/AssetCollectionIterator.php:40

Maybe the composer.json should change to https://github.com/oomphinc/composer-installers-extender or https://github.com/oomphinc/composer-installers-extender

@osma
Copy link
Member

osma commented Dec 15, 2022

Excellent work! A couple of remarks on unit tests:

  • I suggest merging the latest changes from master because the GitHub Actions CI tests were just fixed (PR Fix running git commands in CI tests under GitHub Actions #1397)
  • PHP version matrix for unit tests should be adjusted in .github/workflows/ci.yml so that PHP 8.1 is no longer set as experimental (i.e. tests are no longer allowed to fail on 8.1)

@osma osma self-assigned this Feb 9, 2023
@osma
Copy link
Member

osma commented Feb 9, 2023

I'll see if I can finish this off.

@osma
Copy link
Member

osma commented Feb 9, 2023

But there is something wrong with library dependencies to AssetCollection for robloach/component-installer:

This is caused by the components/handlebars.js package we are using. We have the latest released version (4.7.7), but the packaging for Composer uses the outdated robloach/component-installer package which depends on the also outdated kriswallsmith/assetic package that causes deprecation warnings on PHP 8.1 and will likely break on 8.2. There's not much we can do here except to switch away from the Composer package components/handlebars.js into something better maintained - if nothing else works, we could just include the handlebars.js code in this repo. I'll open a separate issue about this shortly.

@sonarcloud
Copy link

sonarcloud bot commented Feb 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

LGTM

@osma osma marked this pull request as ready for review February 9, 2023 11:45
@osma osma merged commit 6c48032 into master Feb 9, 2023
@osma osma deleted the php81-support branch February 9, 2023 11:46
@osma
Copy link
Member

osma commented Feb 9, 2023

Reported the Handlebars.js dependency issue as #1401 .

@osma osma modified the milestones: Next Tasks, 2.17 Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement maintenance Dependency changes, security updates, infrastructure tweaks & general mainenance
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

Tests fail on PHP 8.1
3 participants