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

Backporting changes from shish #457

Closed
wants to merge 181 commits into from
Closed

Conversation

moufmouf
Copy link
Member

@moufmouf moufmouf commented Nov 25, 2024

This PR backports all changes made in the shish fork.

I guess a few things need to be changes before merging this (putting back the README in place would be change number 1). @shish would you be interested in being a contributor of the original repo? (instead of working on your own fork? This could avoid fragmentation :) )

szepeviktor and others added 30 commits April 9, 2023 16:53
Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

See:
 - [RFC](https://wiki.php.net/rfc/deprecate-implicitly-nullable-types)
 - [PHP 8.4: Implicitly nullable parameter declarations deprecated](https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated)
Currently the generation of files is failing due to ?\\DateTimeZone in a nullable parameter on at least one function. This changeset fixes that a bit crudely but effectively.
shish and others added 21 commits October 8, 2024 19:08
ext-intl no longer needed?
@shish
Copy link
Collaborator

shish commented Nov 26, 2024

Yes I'm happy to help out here and avoid fragmentation :)

Another thing before merging is to decide which versions of PHP are supported - I forget the details and I'm currently AFK travelling for a couple of days so can't easily look it up, but IIRC I couldn't figure out a way to accurately implement wrappers for both PHP<8.2 and PHP>=8.2 at the same time (I think it was because 8.1 can't parse "true" as a return type and php8.2's standard library includes a function which uses "true" as a return type) - for my fork I decided to just support 8.2 onwards, but that is something that should be consciously decided by the maintainence team rather than accidentally happening in the middle of a giant merge

@staabm
Copy link
Collaborator

staabm commented Nov 26, 2024

thx for working in all the upstream changes. I am not confident whether this huge PR should be merged.
I would love doing smaller PRs per change (I guess for most of the changes simple PRs are already open).

if the other maintainers think this PR here has no problems, I am fine with merging though

//cc @shish @OskarStark @silasjoisten

@shish
Copy link
Collaborator

shish commented Nov 26, 2024

My fork is basically upstream + having spent a few days manually merging most of the PRs and dealing with all the merge conflicts (and given how stale upstream had grown, there were a lot of merge conflicts, odds are that the merges weren't perfect...) - having extra eyes sounds like a good idea in any case

Another spanner in the works is that upsteam's build is so out of date that the CI and build process doesn't work until after most of those merges are done - so if you want to use the upstream code as a basis and then merge the outstanding PRs one at a time, be aware that it'll be hard to get any signal on if the individual merges are good or not ^^;;

@OskarStark
Copy link
Collaborator

I would also prefer smaller PRs.
About the PHP version, it seems legit to me, what @shish says about the return type. Can you please elaborate when you are back from traveling?

That said, I would be fine in dropping PHP 8.1 as there is a version for someone which can be used, so we can move one. Thoughts?

@shish
Copy link
Collaborator

shish commented Nov 26, 2024

If somebody else feels like taking the upstream code and merging most of the outstanding PRs, we can see if you get the same result that I got, which would be one way of validating our work, though it seems very labour intensive 😅

@staabm
Copy link
Collaborator

staabm commented Nov 28, 2024

@shish I think I have now merged most changes, which were provided by pull requests to thecodingmachine/safe
could you double check whether there is nothing missing and also provide your manual changes as a PR from your fork?

does this sound feasible/useful?

@shish
Copy link
Collaborator

shish commented Nov 28, 2024

The PRs I've sent so far were mostly the infrastructure bits (CI/CD etc) as those were fairly simple to break out into smaller pieces - the changes to the generator itself are something I'm still figuring out how to split up ^^

@staabm staabm closed this Dec 2, 2024
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.

10 participants