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

Any Plan to make compatible with PHP 8.4 ? #446

Open
vijaygarg-ahead opened this issue Oct 7, 2024 · 17 comments
Open

Any Plan to make compatible with PHP 8.4 ? #446

vijaygarg-ahead opened this issue Oct 7, 2024 · 17 comments

Comments

@vijaygarg-ahead
Copy link

Any Plan to make compatible with PHP 8.4 ?

@develart-projects
Copy link
Collaborator

I don't think so. Simply: too many "strict type" changes to function interfaces. Imo we will need new fork to keep 8.2+ compatibility.

@vijaygarg-ahead
Copy link
Author

I don't think so. Simply: too many "strict type" changes to function interfaces. Imo we will need new fork to keep 8.2+ compatibility.

--> Please do. So, we will make our application compatible with php 8.4

@develart-projects
Copy link
Collaborator

Feel free to fork :)

@sreichel
Copy link

sreichel commented Oct 7, 2024

I don't think so. Simply: too many "strict type" changes to function interfaces. Imo we will need new fork to keep 8.2+ compatibility.

Have you an example for it? I am testing OpenMage with php8.4 and dont see any problems (only #442)

@IMSoP
Copy link

IMSoP commented Oct 8, 2024

Maybe it depends on your definition of "compatible".

Until 9.0 comes along, actual breaking changes to the language are going to be minor, so code that runs fine on 8.0 should need little if any changes to run on 8.4 (and 8.5, if that's what comes next year). For those of us using the repo to keep old projects on life support until they're retired or replaced, that's what matters.

I'm not sure what "strict type changes" you're talking about exactly, but I'm guessing it's that you're seeing new deprecations and warnings, and only considering something "compatible" if it runs without any of those? Ultimately, that's about making the library ready for 9.0 - and I agree, that will be a bigger task.

I don't really see what creating a new fork has to do with anything, though; if someone's willing to put in the work, why would this not be the right repo to publish it to, just as it was for 7.0 and 8.0?

@develart-projects
Copy link
Collaborator

@IMSoP actually the response is in your question. "should need little if any changes" - that's the point, you have to adjust your code to run project. Just because someone on the PHP side has passion to introduce more and more strict type-ing. I have plenty of work, not really happy about fixing the working code just because underlying framework has changed.

This discussion is running here for a while. For now, we decided to keep compatibility between 7.1 to 8.1.

And then, there is this topic about 8.2+ support. Not resolved yet. One idea is to create either new fork, or new version. But both will generate more work, because you have to handle both version and so.

@IMSoP
Copy link

IMSoP commented Oct 8, 2024

@develart-projects I think we're still talking about different meanings of "support". I'm talking about "the minimum changes needed to run an application in production, without fatal errors or unexpected behaviour".

Here is the list of breaking changes for upgrading from PHP 8.1 to PHP 8.2, and here is the list for PHP 8.3. Is there anything on either of those lists which has not been resolved in this repository? If not, I consider the repository to be "compatible with" PHP 8.2 and PHP 8.3.

The list for 8.4 will be similar, because the official policy is that breaking changes should only happen in major versions - i.e. the next significant breaking changes will be in PHP 9.0.

If there is a specific change which you think is blocking this repository from supporting newer versions, I would be interested to understand more clearly.

And then, there is this topic about 8.2+ support. Not resolved yet. One idea is to create either new fork, or new version. But both will generate more work, because you have to handle both version and so.

Presumably you mean clearing the deprecations and warnings introduced in 8.2 and later? I would consider that "9.0 support" - those changes will make no difference to how your application behaves under PHP 8.2, only how it will behave once PHP 9.0 comes out (or possibly even later).

Most of those are just tidying up things that were always arguably bugs, PHP is just making them more obvious. It might well require a lot of effort and testing to fix them correctly, but if someone is willing to do that work, there's unlikely to be a need for a separate fork or branch - the fixed code will work just fine on a wide range of PHP versions.

For instance, passing null to functions where strings are expected can often be resolved by using the null coalesce operator ($foo ?? ''); that was introduced in PHP 7.0, so the code you end up with will work right through from 7.0 to 9.x

@rruchte
Copy link

rruchte commented Oct 8, 2024

FWIW, I'm running a couple of projects in production on 8.3 with the latest ZF1 version and have not encountered any problems. Of course I don't use every class in the framework, but I don't think it will take much effort to continue support until 9.0. I'm willing to contribute to that effort wherever possible.

@develart-projects
Copy link
Collaborator

develart-projects commented Oct 8, 2024

@IMSoP I'm OK with any non-breaking change, therefore I always review the PR itself. If it's not trying to enforce new function signature or any strict output, I'm considering that as "safe" change to merge (without affecting existing code). So if you send any PR, I'm happy to review.

@develart-projects
Copy link
Collaborator

develart-projects commented Oct 8, 2024

@rruchte 8.2 examle:

The following methods now enforce their signature:

SplFileInfo::_bad_state_ex()
[SplFileObject::getCsvControl()](https://www.php.net/manual/en/splfileobject.getcsvcontrol.php)
[SplFileObject::fflush()](https://www.php.net/manual/en/splfileobject.fflush.php)
[SplFileObject::ftell()](https://www.php.net/manual/en/splfileobject.ftell.php)
[SplFileObject::fgetc()](https://www.php.net/manual/en/splfileobject.fgetc.php)
[SplFileObject::fpassthru()](https://www.php.net/manual/en/splfileobject.fpassthru.php)

Not sure if any of those are used by ZF library, just an example of the breaking change in the 8.2.

@IMSoP
Copy link

IMSoP commented Oct 8, 2024

Not sure if any of those are used by ZF library, just an example of the breaking change in the 8.2.

Well, that's easily discovered.

grep -ir SplFileObject
# No results

grep -ir SplFileInfo
library/Zend/File/PhpClassFile.php:class Zend_File_PhpClassFile extends SplFileInfo
library/Zend/File/ClassFileLocator.php:        // If we somehow have something other than an SplFileInfo object, just
library/Zend/File/ClassFileLocator.php:        if (!$file instanceof SplFileInfo) {

A 30-second glance at those two files shows that there is no mention of any of the changed functions.


Let's look at the rest of the 8.2 breaking changes:

  • DateTime::createFromImmutable() now has a tentative return type of static, previously it was DateTime.
    • "Tentative return types" are a concept only introduced in PHP 8.1 (as a step towards a PHP 9.0 or later change); so this library won't care about them at all
  • DateTimeImmutable::createFromMutable() now has a tentative return type of static, previously it was DateTimeImmutable.
    • As above
  • number symbols in relative date formats no longer accept multiple signs, e.g. +-2.
    • Searching for "-+" and "+-" shows no relevant code. Seems very unlikely to be a problem.
  • ODBC and PDO_ODBC
    • as far as I can see, the library doesn't directly support either of these extensions
  • glob() now returns an empty array if all paths are restricted by open_basedir. Previously it returned false.
    • 9 uses of glob(); most already assume the result is an array
    • \Zend_Cache_Backend_File::_clean does explicitly check for false, but only to shortcut the function and avoid a foreach loop
  • FilesystemIterator::__construct(): prior to PHP 8.2.0, FilesystemIterator::SKIP_DOTS constant was always set and couldn't be disabled.
    • No uses.
  • strtolower(), strtoupper(), stristr(), stripos(), strripos(), lcfirst(), ucfirst(), ucwords(), and str_ireplace() are no longer locale-sensitive.
    • The library never sets the system locale; the only use of setlocale() is to fetch the current setting (setlocale(LC_ALL, 0))
  • str_split() returns an empty array for an empty string now.
    • There are a bunch of uses of this. There is a slim chance that one of them was accidentally relying on this behaviour.
  • ksort() and krsort() now do numeric string comparison under SORT_REGULAR using the standard PHP 8 rules now.
    • This is about comparing mixed lists of strings and numbers. There are a bunch of uses, but it's unlikely anything in the framework itself is going to care about this edge case.
  • var_export() no longer omits the leading backslash for exported classes, i.e. these are now fully qualified.
    • Non-debug uses are in bin/classmap_generator.php, library/Zend/CodeGenerator/Php/Parameter.php, library/Zend/Serializer/Adapter/PhpCode.php, and library/Zend/Config/Writer/Array.php
    • I can't see any code in these which would obviously break because of the extra backslash
  • GlobIterator now returns an empty array if all paths are restricted by open_basedir.
    • No uses.

That's it.


So, we can conclude that the only possible problems someone might see are:

  • str_split being given an empty string, and the code somehow relying on the old behaviour
  • ksort being given a mixture of strings and numbers, and the code somehow relying on the old sort order

We could spend a little bit longer looking at those uses; or we could conclude that both are pretty unlikely, and the library already supports PHP 8.2.

@develart-projects
Copy link
Collaborator

develart-projects commented Oct 8, 2024

@IMSoP thanks for the analysis. Anyway, your reply is not contradicting anything I wrote in my last reply. All these are potential breaking changes and if someone drop an PR and we will exclude any backward compatibility issues, I'm happy to merge.

Here is an example of the PR, causing breaking changes, because of deprecation messages: https://github.com/Shardj/zf1-future/pull/397/files

I refused to merge, because of what I already explained. Considering all potential consequences case-by-case.

@IMSoP
Copy link

IMSoP commented Oct 8, 2024

@IMSoP thanks for the analysis. Anyway, your reply is not contradicting anything I wrote in my last reply. All these are potential breaking changes and if someone drop an PR and we will exclude any backward compatibility issues, I'm happy to merge.

All of what are potential breaking changes? What PR are you waiting for? I went through every single change in the changelog and explained why no change is needed to the library for any of them.

The point I was trying to illustrate was that "supporting PHP 8.x" mostly means "reading through a small checklist, making sure none of the rare edge cases happen to affect this library". It took me 30 minutes; maybe we could spend another hour or two to be really sure. There's a vast ocean of difference between that and "we will need new fork to keep 8.2+ compatibility".

Here is an example of the PR, causing breaking changes, because of deprecation messages: https://github.com/Shardj/zf1-future/pull/397/files

This is an example of a change which will be needed for PHP 9.0, at the earliest. Until then, it is a nice to have, and I totally understand your choice to favour extending support for 7.1 over early support for 9.0.

@woranl
Copy link

woranl commented Oct 9, 2024

Is there a possibility to leverage LLM to help rewrite some of the code? The prompt may be to “Make library compatible to PHP 8.4” I never tried it myself, but it seems like a good use case for AI.

@IMSoP
Copy link

IMSoP commented Oct 9, 2024

@woranl If significant changes are needed, there's some better tools out there than random word generators, notably Rector. The bigger challenge is how far the library can be updated without dropping support for older versions of PHP, and without requiring users to adjust their applications as well.

But just to reiterate there will probably be no changes needed to this library to run under PHP 8.4. There may be an increase in deprecation notices and warnings, but these do not indicate required changes.

When PHP 9.0 comes along, there will be tougher choices to make, but that's likely to be at least two years away.

@IMSoP
Copy link

IMSoP commented Oct 9, 2024

To move things forward, I have opened issues with checklists for PHP 8.2 and 8.3 compatibility: #449 and #450 The 8.2 list is mostly already checked, per my comment above; the 8.3 list I've glanced at some very easy checks, the rest is mostly just searching the code for relevant function calls.

Once PHP 8.4, comes out, we can raise a similar list based on the final list of changes in that.

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

No branches or pull requests

7 participants
@IMSoP @rruchte @sreichel @woranl @develart-projects @vijaygarg-ahead and others