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

Remove implicitly nullable parameter declarations #215

Merged

Conversation

MarkusBauer
Copy link

Implicitly nullable parameter declarations are deprecated from PHP 8.4 on and will be removed in PHP 9. This PR removes all nullable declarations to keep the code compatible with future PHP versions, with no impact on backward compatibility.

As an alternative to dropping declarations, we could use declarations of the form (?Matrix $ctm = null), but then we would lose compatibility with PHP before 7.1.

How to reproduce the issues:

> docker run --rm -v .:/app "php:8.4-rc-cli" find /app -type f -name "*.php" -exec php -d error_reporting=32765 -l {} \; | grep -v 'No syntax errors detected'
Deprecated: setasign\Fpdi\GraphicsState::__construct(): Implicitly marking parameter $ctm as nullable is deprecated, the explicit nullable type must be used instead in /app/src/GraphicsState.php on line 29
Deprecated: setasign\Fpdi\PdfParser\Type\PdfStream::parse(): Implicitly marking parameter $parser as nullable is deprecated, the explicit nullable type must be used instead in /app/src/PdfParser/Type/PdfStream.php on line 38
Deprecated: setasign\Fpdi\PdfParser\Type\PdfDictionary::get(): Implicitly marking parameter $default as nullable is deprecated, the explicit nullable type must be used instead in /app/src/PdfParser/Type/PdfDictionary.php on line 110

With the patches from this PR, the deprecation warnings are gone.

Copy link
Member

@JanSlabon JanSlabon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Markus, thank you for the PR. What do you think about my comments?

src/GraphicsState.php Show resolved Hide resolved
src/PdfParser/Type/PdfStream.php Show resolved Hide resolved
src/PdfParser/Type/PdfDictionary.php Show resolved Hide resolved
@andrewnicols
Copy link

Is support for PHP 7.1 (and 7.2, 7.3) really something that is desirable, let alone PHP 5?

@MarkusBauer
Copy link
Author

Is support for PHP 7.1 (and 7.2, 7.3) really something that is desirable, let alone PHP 5?

Not sure if desirable, but I wouldn't drop it for such a minimal change - in particular because it solves a problem we don't have yet. There are some really old PHP versions around in the LTS world. For example, RedHat's PHP 5.4 just went EOL a few weeks ago.

@MaximilianKresse MaximilianKresse merged commit d9f85ad into Setasign:master Aug 6, 2024
13 checks passed
@JanSlabon
Copy link
Member

We agree with @MarkusBauer and do not drop legacy support for such a minimal change for now.

@MarkusBauer MarkusBauer deleted the fix-php-8.4-deprecations-2 branch August 6, 2024 09:41
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.

4 participants