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.

Sign up for GitHub

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

Jump to bottom

[WIP] Add support for PHP 8.4 #32

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

[WIP] Add support for PHP 8.4 #32

wants to merge 2 commits into from

Conversation

bnf
Copy link
Member

@bnf bnf commented Oct 9, 2024 •
edited
Loading

TODO:

  • Check possible upgrade issues due to signature changes (plugins are executed with old/new code mixed during composer upgrade

@bnf
[TASK] Fix PHP 8.4 compatibility
d3cfe8c 
Defaulting to null without being nullable (?Foo) is deprecated for typed
parameters in PHP 8.4

Therefore following changes have been made:

 * `IncludeFile`: We actually do not need a Filesystem parameter for
   `IncludeFile. It has never been passed and is therefore removed.
 * `ClassAliasMapGenerator` `Config` parameter is unused (was never set)
   since the introduction in a63a582.
   It is removed.
 * `ClassAliasMapGenerator` `IO` parameter has always been set
   and is therefore now a required parameter.

Additional the call to str_getcsv is adapted to set all optional
arguments as the defaults will change in PHP >= 9 and PHP 8.4
deprecated the omission of the optional arguments.

Fixes TYPO3#31
@bnf
[TASK] Test PHP 8.3 and 8.4
faf06e3 
Also drop prophecy workarounds since PHP 8.2 compatibility
has been enabled upstream.
@bnf
Copy link
Member Author

bnf commented Oct 9, 2024

I think this refactoring would be good for the codebase, but applying ?null and raising required PHP version (as suggested by Helmut in #31 (comment) ) would be less invasive..

I'll provide a second PR for that so we can decide which one we take.

@bnf bnf mentioned this pull request Oct 9, 2024
Merged
@helhum
Copy link
Contributor

helhum commented Oct 11, 2024

@bnf I think this tiny refactoring still makes sense. I'd keep PHP 7.1 minimum though. So if you would give this a shot, I'd merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bnf @helhum