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

Decide which PHP versions to support #500

Open
shish opened this issue Dec 4, 2024 · 16 comments
Open

Decide which PHP versions to support #500

shish opened this issue Dec 4, 2024 · 16 comments

Comments

@shish
Copy link
Collaborator

shish commented Dec 4, 2024

The PHP standard library is constantly changing, and our generated code changes with it, which means that we naturally get out-of-sync with older php versions; this happens in both subtle and not-subtle ways, eg

  • ftp_alloc takes a resource in php 8.1 and an FTP\Connection in 8.2+ -- because we're based on the current docs, we generate code that says "we need an FTP\Connection" -- so this function will crash with a type incompatibility if anybody tried to run it on 8.1
    • the fix for this would be to add a manual override for all these functions which accepts resource|FTP\Connection as input. Going through not just the current docs but also historic versions of docs to see which functions changed parameter types, and manually supporting all of those, seems like a nightmare which would inevitably fail due to human error.
  • the current stdlib has a function with a return type of true, which php 8.1 treats as a syntax error
    • we have a hack to replace true with bool

Given that supporting a wider range of versions becomes exponentially harder, what range do we want to support?

@staabm
Copy link
Collaborator

staabm commented Dec 4, 2024

with stdlib you mean php-src or some other php based dependency?

  • ftp_alloc takes a resource in php 8.1 and an FTP\Connection in 8.2+ -- because we're based on the current docs, we generate code that says "we need an FTP\Connection"

maybe it would make sense to publish different safe versions, which indicates the min php version?

like thecodingmachine/safe-php81 and thecodingmachine/safe-php82 etc. (only diff from our point of view would be the base signature version we consume)

or alternatively keep thecodingmachine/safe-php but publish multiple major versions (thru the github automation, not maintaining multiple code-bases on our end; major 3.x for php81, major 4.x for php82,..)

@staabm
Copy link
Collaborator

staabm commented Dec 4, 2024

//cc @OskarStark @silasjoisten opinions?

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

like thecodingmachine/safe-php81 and thecodingmachine/safe-php82 etc. (only diff from our point of view would be the base signature version we consume)

As an application developer, I want my application to support a range of both current-and-future PHP versions -- I wonder if composer has sufficient magic so that the app dev can require thecodingmachine/safe and that gets translated into safe-php81 / safe-php82 etc at install time? Although with composer.lock files I think that would mean the end-user installing the application ends up trying to install whichever package is mentioned in the lock file, not whichever version is correct for their runtime? So if the app dev publishes a composer.lock with safe-php81, and a user tries to install the app with php82, that'll fail? :S

or alternatively keep thecodingmachine/safe-php but publish multiple major versions (thru the github automation, not maintaining multiple code-bases on our end; major 3.x for php81, major 4.x for php82,..)

My gut was thinking in this direction - so an application developer who wants to support "php81 or newer" can depend on "tcm/safe 3.x or newer"... except I think composer will refuse to install 4.x when asked for "3.x or newer", as that's considered a breaking change?

I have no good answers here D:


Separate from the problem of "how do we support multiple versions", there's also a question of "which versions to support" -- for that, my gut tells me that since the current release is so old, we should do one more release which supports 8.1-8.4, then create a branch from there which will continue to support 8.1 with security-fixes-only for as long as php 8.1 itself receives security fixes -- and then going forwards we only officially support "actively maintained" versions of php ( https://endoflife.date/php )

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

An alternative approach which seems like implementation hell, and I don't want to do this, just throwing it out there as an option:

  • check out multiple copies of the documentation, one for each PHP version
  • generate multiple functions as needed, eg
if (PHP_VERSION >= 8.0 && PHP_VERSION < 8.2) {
    function ftp_alloc(resource $connection) {...}
} elseif(PHP_VERSION > 8.2) {
    function ftp_alloc(FTP\Connection $connection) {...}
}

though even this doesn't guarantee compatibility (Like I think true as a type hint would crash php 8.1 as a syntax error, even if that type hint was only specified in the 8.2 branch of the code?)

@OskarStark
Copy link
Collaborator

If a PHP version is in security only, drop it here and increase. They can still use the versions which are available, to get new features, upgrade, same for the other packages and frameworks.we don't have the man power to maintain many version or packages.
Drop it like it's hot 🔥

@silasjoisten
Copy link
Collaborator

silasjoisten commented Dec 4, 2024

I agree we should only support PHP Versions which are maintained and not security only.

@staabm
Copy link
Collaborator

staabm commented Dec 4, 2024

if we drop PHP8.1 we will loose ubuntu22 LTS, which I think is a problem.

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

We kind-of already don't support 8.1, because the API has drifted to be incompatible - we claim to support it, but the code is merely 8.1-compatible on a surface syntax level, and it works-by-coincidence in most-but-not-all cases (ie, it works for functions whose APIs haven't changed at all)

AFAIK the PHP documentation only covers "the current version" - so my suspicion is that to properly support anything other than "the current version", we'll need to use git history to rewind the documentation back to that old version, run the generator script, and release something like safe-php81 based on the 8.1 docs (and repeat for 8.2 + 8.3) - and even if we drop 8.1 now (and we drop 8.2 when it goes out of support at the end of the month), we'd still need to take this approach if we want to correctly support both 8.3 and 8.4 despite their API differences.

I think our options are either:

  • release a separate package for each PHP version
    • ideally with some kind of glue so that the application just depends on "safe" and then either safe-php83 or safe-php84 get installed as-needed
  • we only fully support the PHP master branch, and it should mostly-work with older versions but no promises
    • if you want to run an old version of PHP, you can install an old version of Safe to go with it
    • this is the status quo, and the simplest approach - but if we do this, we should probably at least put out a table in the README or wiki which tells people what version of Safe to install given their PHP version, and maybe update composer.json to stop claiming "this code works with 8.1 - 8.4" and instead only claim support for one version at a time (which IIRC would then force composer to install old versions of safe on old versions of php)

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

Perhaps, if we're willing to slightly abuse semver:

  • Manually use git to rewind the docs to 8.1
  • edit composer.json to say "supports 8.1.X"
  • run generate
  • publish 3.1.0
  • use git to rewind the docs to 8.2
  • edit composer.json to say "supports 8.2.X"
  • run generate
  • publish 3.2.0
  • use git to rewind the docs to 8.3
  • edit composer.json to say "supports 8.3.X"
  • run generate
  • publish 3.3.0
  • use git to rewind the docs to 8.4
  • edit composer.json to say "supports 8.4.X"
  • run generate
  • publish 3.4.0
  • tell users to depend on "safe 3.X" and let composer figure out the subversion?

(... buuuuut I think that still doesn't work with composer.lock files? Again throwing out ideas without really knowing what's best, hoping that it inspires somebody to have a brainwave :S)

@OskarStark
Copy link
Collaborator

if we drop PHP8.1 we will loose ubuntu22 LTS, which I think is a problem.

Not a problem for me, they can stick to the current version 🤷‍♂️

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

Thinking out loud some more, I think given composer.lock files, any approach involving "multiple packages" or "multiple versions of a package" is going to cause pain for application developers and end users, and reading the composer docs I don't see a way around that - it needs to be a single package which supports multiple php versions at once, despite the fact that our job is to mirror the PHP API, and different PHP versions have different APIs...

So a proposal:

  • we build against the nightly versions of the docs, which means we fully support nightly PHP with zero effort on our part (this is where we are now, despite the fact that we claim to support 8.1 through 8.4, and things mostly work with those versions through sheer luck)
  • we commit to adding hacks and manual overrides as-needed to backport support for "actively supported" php versions, and we advertise support for those versions in composer.json
    • an example of such a hack: when the FTP library switched from using resources to Connection objects, we'd need to implement both versions in lib/special_cases.php, with an if(PHP_VERSION ...) statement to decide which version to use
  • (perhaps, not sure about this part) if a number (ideally a larger number than "1") of maintainers are willing to commit the necessary bandwidth to support security-fix-only PHP versions, they can add the hacks and manual overrides necessary to backport support for those versions, and we advertise support for those versions in composer.json
  • if somebody wants to use an out-of-date version of PHP, we tell them to use an out-of-date version of Safe

Bonus things for consideration:

  • 8.2 is "actively supported" now, but it is due to stop being supported this month, and we're going to need some time to test and publish a beta release / make sure it works / put out a real release -- so I expect 8.2 to be in security-fix-only mode by the time we hit "publish" on the big new version.
  • The upcoming version of Debian Stable, expected to be released mid-2025, currently ships with 8.2 🙃
  • I think it should be possible to release Safe 3.0 with support for "actively supported" versions of PHP, we see what the community feedback is like, and then we maybe add support for security-fix-only versions of PHP in 3.1 -- whereas releasing 3.0 claiming to support all versions, getting the feedback of "there are a ton of subtle incompatibilities", then having to either rush a bunch of fixes or drop support for older versions, would suck for everybody.

@robsontenorio
Copy link

Does it has to do with deprecation warnings in PHP 8.4?

Deprecation Notice: Safe\gmdate(): Implicitly marking parameter $timestamp as nullable is deprecated, the explicit nullable type must be used instead in /var/www/app/vendor/thecodingmachine/safe/deprecated/datetime.php:24

@shish
Copy link
Collaborator Author

shish commented Dec 4, 2024

Does it has to do with deprecation warnings in PHP 8.4?

No, that is unrelated

(We already support new versions of PHP (the code in the master branch has already fixed the issue you mention) - the question being discussed in this thread (which we need to find an answer for before we can publish a new release with the php8.4 fixes) is which old versions are we able and willing to support)

@tpetry
Copy link

tpetry commented Dec 5, 2024

  • generate multiple functions as needed, eg
if (PHP_VERSION >= 8.0 && PHP_VERSION < 8.2) {
    function ftp_alloc(resource $connection) {...}
} elseif(PHP_VERSION > 8.2) {
    function ftp_alloc(FTP\Connection $connection) {...}
}

though even this doesn't guarantee compatibility (Like I think true as a type hint would crash php 8.1 as a syntax error, even if that type hint was only specified in the 8.2 branch of the code?)

Why not different files? One for 8.0, 8.1, 8.2, 8.3, 8.4 etc.

Sure, bundle size may increase but that isn't really a problem. And that way Safe could only include files for the current PHP version and syntax errors for different php versions are not a problem as those files are never parsed/executed.

@shish
Copy link
Collaborator Author

shish commented Dec 5, 2024

Why not different files? One for 8.0, 8.1, 8.2, 8.3, 8.4 etc.

I'm sure I had a reason why that would be a major pain too - but then I forgot the reason, and I tried it out as a proof-of-concept, and I've now half-implemented that approach and I haven't run into any major problems yet 🤔

386e365

One thing which could be an issue with any kind of "rewind the git history of the documentation back to a few years ago and generate based on that" is that currently-fixed documentation bugs will be re-introduced. I think the worst case there is that we end up having a fork of the PHP docs where we have php81 / php82 / etc branches with backported bugfixes, and then we generate code based on those bugfixed-branches rather than based on specific old commit IDs.

@staabm
Copy link
Collaborator

staabm commented Dec 5, 2024

@shish could you open a PR so we can have a look at the generated files?

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

6 participants