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.2 support #26

Merged
merged 3 commits into from
May 11, 2023
Merged

PHP 8.2 support #26

merged 3 commits into from
May 11, 2023

Conversation

patrickomeara
Copy link
Contributor

@patrickomeara patrickomeara commented Dec 12, 2022

This PR updates voku/portable-utf8 where a string interpolation deprecation has been fixed.

It also updates the test files to match the iterator parent return types. This upgrade means that the tests will only run on PHP 8.0 and above, due to mixed not being available on earlier versions. However, this matches the official supported versions https://www.php.net/supported-versions.php

image

I've preemptively updated the README assuming there will be a new major version released.

I've also added a github actions workflow to test PHP 8.0, 8.1 and 8.2 but Actions will need to be turned on for this repo. There may need to be changes to the file after it has been ran. I'm just basing it off other files across public and my private repos.

@SharkyKZ
Copy link
Contributor

What's the practical reason for increasing PHP requirements instead of using attributes?

@patrickomeara
Copy link
Contributor Author

Hi @SharkyKZ thanks for your question, the practical reason is so that this package works with PHP 8.2 out of the box, which it currently does not.

As the README states there are three major versions of this package, all targeting specific PHP versions. This follows semantic versioning, where breaking changes get a new major.

php-stemmer/README.md

Lines 30 to 46 in 1601ca7

Installation
------------
For PHP5, use 1.3
```
composer require wamania/php-stemmer "^1.3"
```
For PHP7 use 2.x (branch 2.x is backward compatible with 1.x)
```
composer require wamania/php-stemmer "^2.0"
```
For PHP^7.3 and PHP^8.0 use 3.x (backward compatible, but phpunit^9 don't work with php < 7.3)
```
composer require wamania/php-stemmer "^3.0"
```

Attributes are a possibility, I'll let the maintainers decide which approach they would like to take. Looking forward to moving this package forward.

@SharkyKZ
Copy link
Contributor

That is not a practical reason because narrowing PHP version support is not required to support PHP 8.2. It's a change for the sake of change. Of course, it's up to maintainers to decide. But with current code base there is no gain. Even the hard v6 requirement for voku/portable-utf8 is technically not necessary. Just my 2c.

@patrickomeara
Copy link
Contributor Author

It's not change for the sake of change, out of the box this package is causing errors when running Psalm on PHP 8.2.

./vendor/bin/psalm
Uncaught RuntimeException: PHP Error: Using ${var} in strings is deprecated, use {$var} instead in /Users/pat/Code/web/vendor/voku/portable-utf8/src/voku/helper/UTF8.php:3880 in /Users/pat/Code/web/vendor/vimeo/psalm/src/Psalm/Internal/ErrorHandler.php:66

voku/portable-utf8 has recognised this and released a fix in 6.0.5 voku/portable-utf8#181

Not pulling that updated package in because it's "technically not necessary" is not moving this package forward.

@SharkyKZ
Copy link
Contributor

I think you misunderstood. Supporting v6 is fine but dropping v5.4 was technically unnecessary. Likewise, there was no practical reason to drop PHP 7.3 support when you can simply use ReturnTypeWIllChange attribute to suppress the warnings. The narrowing of supported version is, in fact, a change for the sake of change in this case.

@wamania
Copy link
Owner

wamania commented Feb 7, 2023

Hi
I use your github actions file directly, it's something that should have already been there. Now we have ci tests.

I don't really want to have too many versions. It's possible to keep the compatibility with 7.3 and upgrade portable-utf8 (to be compatible php8.2)
As it's your PR, can you modify it to keep the compatibility with 7.3 and only add support for php8.2
there is no need here to have breaking changes.

Thanks !

@patrickomeara
Copy link
Contributor Author

I've added PHP 8.2 to the test matrix, and added back in the stability of lowest and stable

All test are now passing https://github.com/patrickomeara/php-stemmer/actions/runs/4129148443

@Namoshek
Copy link

As a workaround, since this library requires no changes when using voku/portable-utf8:^6.0, use an inline alias for the version in your composer.json:

"require": {
    "voku/portable-utf8": "6.0.12 as 5.4.0"
}

Copy link
Collaborator

@HLeithner HLeithner left a comment

Choose a reason for hiding this comment

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

Looks good to me

@HLeithner
Copy link
Collaborator

@wamania can we merge this and release a new version please?

Copy link

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

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.

6 participants