Add support for php 8.1 and drop 7.2 and 7.3

marko-ilic commented 2 years ago

In the scope of this PR, the support for PHP 8.1 is added (deprecated Serializable interface is removed). Since php 7.2 and 7.3 are no supported anymore, the support for them is removed in this PR.

marko-ilic commented 2 years ago

Hello @JeroenDeDauw 👋 What do you think about changes in this PR?

JeroenDeDauw commented 2 years ago

I had a super quick glance and what I saw looked good.

Thing is that I am no longer maintaining this library. I'd like to, but WMDE insists on doing so instead. So let's see if they actually do.

marko-ilic commented 2 years ago

Hello @thiemowmde @manicki Can some of you, please, review this pr when you have time?

thiemowmde commented 2 years ago

I'm currently not active in the Wikidata project (which is where this library belongs). I have a few suggestions, though.

The WMDE teams usually work with https://phabricator.wikimedia.org to track their resources. It might be a good idea to fill a ticket over there. That should make it easier for them to see this.
From this request's description it's hard to tell how urgent it is. Note that Wikimedia servers are still using PHP 7 (for reasons nobody likes, but that's just how it is at the moment). Is there a ticket, sprint, team or something you can link this pull request to?
The main commit in this patch is huge and does a lot of different things the same time. It would be better to split this into separate commits, possibly even separate pull requests. For example, the old array syntax still works in PHP 8.1, as far as I'm aware of. The patch also adds trailing commas. Why? I don't really see how such code style changes are related to "support for PHP 8.1". Not doing them (or doing them separately) would make this patch much smaller and much easier to review.

lucaswerkmeister commented 2 years ago

The WMDE teams usually work with https://phabricator.wikimedia.org to track their resources. It might be a good idea to fill a ticket over there. That should make it easier for them to see this.

I think this could fall under the existing task T301249. I’ll leave a comment there pointing to this PR.

I agree with Thiemo that this patch does too many things at once, and also that it’s too early to drop PHP 7.2 compatibility. Production is still on 7.2; in theory we could release Diff 4.0 requiring PHP 8.1+ now, and just keep using Diff 3.0 in Wikibase, but that means it’ll be a pain if we need any other Diff changes while still on PHP 7.2.

reedy commented 2 years ago

I would concur too.

Adding PHP 8.1 support (especially if it's just Serializable related) can be done in a backwards compatible way.

manicki commented 2 years ago

To keep things somehow focsued how about I take the topic to relevant WMDE teams (i.e. teams that would actually have to "deal with" the change), and we'll come back here, in particular with regards to the question of creating 8.1 supporting version, and the need/lack of thereof of adding it in a backwards compatible way? This very library had a similar conversation when PHP 7 enabled version was added before it could have been used in Wikibase on WMF servers. I think we can refer to what was learned then with regards to how did complicate things etc.

Production is still on 7.2; in theory we could release Diff 4.0 requiring PHP 8.1+ now, and just keep using Diff 3.0 in Wikibase, but that means it’ll be a pain if we need any other Diff changes while still on PHP 7.2.

reedy commented 2 years ago

The patches to MW core have been relatively trivial - https://github.com/wikimedia/mediawiki/commit/4cc1a3eecaaecc4368753a9a08fa0732bddae6fb

There hasn't been a release for 3.5 years - https://github.com/wmde/Diff/releases/tag/3.2.0

The point isn't just about Wikimedia servers though. You're going to want to have PHP 8.1 support (as people are using it), and if you can do that with minimal work, without maintaining support for multiple branches of the library (and then also allowing those in composer.json)...

The Serializeable related parts of the diff are pretty simple - https://github.com/wmde/Diff/pull/128/files#diff-ba4af8243477bbb63e951b5666ef96e257af4efd6e76c05c29f47563a9d5a0f8R65 for example

marko-ilic commented 2 years ago

I've prepared a new PR with adding support for PHP 8.1 in a backward compatible way.

manicki commented 2 years ago

Thanks @marko-ilic for taking the effort to make the backwards-compatible approach

reedy commented 2 years ago

I think the other PR is ready for merge, other than maybe one minor issue with travis config (which may or may not have been fixed upstream). Waiting to see what https://github.com/wmde/Diff/pull/130 comes out with.

But can be squashed and merged as is; we can update the PHP 8.1 config in .travis.yml later easily enough

lucaswerkmeister commented 2 years ago

I think this has been superseded by #129 and other pull requests; please reopen if I missed something.

wmde / Diff

Add support for php 8.1 and drop 7.2 and 7.3 #128