Skip to content

fix: resolve deprecations for implicitly nullable parameters in PHP 8.4 #231

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
Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

fix: resolve deprecations for implicitly nullable parameters in PHP 8.4 #231

wants to merge 13 commits into from

Conversation

wesolydexter
Copy link

Resolved deprecation warnings related to implicitly nullable parameters in PHP 8.4. Adjusted parameter declarations to explicitly use nullable types where default null values are set.

Changes were applied using the fixer with the nullable_type_declaration_for_default_null_value option.

This ensures compatibility with PHP 8.4 and avoids deprecation messages.

fix: resolve deprecations for implicitly nullable parameters in PHP 8.4
1245123 
Resolved deprecation warnings related to implicitly nullable parameters in PHP 8.4. Adjusted parameter declarations to explicitly use nullable types where default `null` values are set.

Changes were applied using the fixer with the `nullable_type_declaration_for_default_null_value` option.

This ensures compatibility with PHP 8.4 and avoids deprecation messages.
@wesolydexter wesolydexter mentioned this pull request Apr 8, 2025
Open
@wesolydexter
Copy link
Author

@jcchavezs would you take a look. It'll take 2 min :)

@jcchavezs
Copy link
Contributor

jcchavezs commented Apr 10, 2025 •
edited
Loading

I am not part of this project anymore but changes LGTM. I would say you should add a link to the deprecation and support 8.2, 8.3 and 8.4 on the CI and see what happens and maybe @reta or @shakuzen can approve the CI.

@reta
Copy link

reta commented Apr 10, 2025

I am not part of this project anymore but changes LGTM. I would say you should add a link to the deprecation and support 8.2, 8.3 and 8.4 on the CI and see what happens and maybe @reta or @shakuzen can approve the CI.

I would love to, have to admit that PHP is not my thing, @codefromthecrypt do you know who can help us here?

BafS
BafS approved these changes Apr 10, 2025
View reviewed changes
Copy link

@BafS BafS left a comment

Choose a reason for hiding this comment

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

Thank you! We have the same issue in our codebase

@shakuzen
Copy link
Member

It looks like the CI needs to be updated in order to run. It's getting Missing download info for actions/cache@v2 which looks to be caused by v2 of that Action being EOL: https://github.blog/changelog/2024-12-05-notice-of-upcoming-releases-and-breaking-changes-for-github-actions/#actions-cache-v1-v2-and-actions-toolkit-cache-package-closing-down

@wesolydexter
Copy link
Author

I added the missing PHP versions to the CI and updated the cache version.

Update workflows to use the latest output method
05e2473 
Reason: The ::set-output command was deprecated by GitHub. Replacing it improves security and ensures compatibility with the latest GitHub Actions standards.
https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

Implementation: Replaced usages of the ::set-output command with the $GITHUB_OUTPUT.

Side effects: None expected. The workflow behavior remains the same.
@wesolydexter
Copy link
Author

I swapped out ::set-output for $GITHUB_OUTPUT because the old one’s outdated.
https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands

Fix composer cache directory setup
1ebf49b 
Reason: The workflow was failing on Ubuntu and macOS because the output of the composer cache directory command had extra newlines, which broke the GitHub Actions output format.

Implementation: Added a sanitization step to remove newlines and carriage returns from the command output using `tr -d '\n' | tr -d '\r'`.

Side effects: None expected. The change ensures consistent behavior on all runners (Windows, macOS, and Linux).
@wesolydexter
Copy link
Author

I hope that's the end of the surprises...

dmajka added 5 commits April 11, 2025 12:38
Fix composer cache directory output in CI
fb90b62 
Reason: The CI workflow was failing because the step that retrieves the composer cache directory was not providing the correct path. This caused issues in subsequent steps that rely on the cache directory.

Implementation: Modified the `Get composer cache directory` step to correctly define the path output.

Side effects: None expected.
Fix mysqli connection test.
b2a116a 
Reason: During tests, an error is thrown: `strpos(): Argument openzipkin#1 ($haystack) must be of type string, null given`.
Implementation: Added a safeguard against null values.
Additionally: Removed the `version` field from the Docker Compose file as it is unnecessary (the `version` attribute is obsolete).
Side effects: None.
Renamed abstract test classes to avoid warnings
f67b0b4 
Reason: The abstract test classes had a "Test" suffix, which caused deprecation warnings in PHPUnit because it now tries to treat such classes as actual tests.

Implementation: Renamed abstract test classes by appending the "Case" suffix to their names. Updated all references to the renamed classes accordingly.

Side effects: No functional changes. Only class names were updated.
Fix namespace issue in MysqliTest
c309923 
Reason: Namespace name doesn't match the PSR-0/PSR-4 project structure.
Side effects: None expected.
Update CI to upload coverage report for PHP 8.1
bf7a85e 
Reason: PHP 8.0 is no longer a supported version.
Implementation: Upgraded testing coverage to the latest supported PHP version (security fixes only).
Side effects: None expected.
@wesolydexter
Copy link
Author

From what I can see, there’s probably an issue with access to Coveralls (maybe the token is invalid - I don’t know, it’s in the secrets). However, I also noticed a few other errors and warnings, so I’m fixing those. It’s possible that this test error was interfering with the coverage check.

@wesolydexter
Copy link
Author

Can someone give an approval?

@shakuzen
Copy link
Member

Running the tests was failing:

Run composer test -- --coverage-clover=build/logs/clover.xml
> phpunit tests '--coverage-clover=build/logs/clover.xml'
PHPUnit [9](https://github.com/openzipkin/zipkin-php/actions/runs/14403682799/job/40485287264?pr=231#step:9:10).6.22 by Sebastian Bergmann and contributors.
Error response from daemon: No such container: zipkin_php_mysql_test
sh: 1: docker-compose: not found

Revert version number in Docker Compose file.
539be3c 
Reason: This argument should be redundant, but CI tests fail to start, so it might be related to this issue (`Error response from daemon: No such container: zipkin_php_mysql_test`).
Implementation: Added the `version` field to the Docker Compose file.
Side effects: None expected.
@wesolydexter
Copy link
Author

One more approval, please.

Use docker compose instead of docker-compose.
f845d65 
Reason: `docker-compose: not found`.
Implementation: Replaced deprecated `docker-compose` commands with `docker compose`.
Side effects: None.
@wesolydexter
Copy link
Author

That should help.

@shakuzen
Copy link
Member

Seems to have helped on ubuntu but macos is still having issues

@wesolydexter
Copy link
Author

It seems that the php-coveralls configuration is causing the issue here. The error Client error occurred. status: 422 Unprocessable Entity and the message Couldn't find a repository matching this job usually indicate a problem with the COVERALLS_REPO_TOKEN configuration. Could you please double-check that the correct token is set in the repository secrets, and verify that the project is properly linked to Coveralls?
If fixing the token isn’t feasible or Coveralls isn’t critical for this project, we might consider removing this step from the workflow, as the coverage report submission isn’t working as expected anyway.

As for macOS, it can be disabled for tests using Docker Compose (for Windows, it has been disabled for a long time).

dmajka added 2 commits April 15, 2025 12:42
Skip testing mysql connection on non-Linux systems
ee9748f 
Reason: The mysql connection test by Docker compose does not work on macOS (`sh: docker: command not found`).
Implementation: Previously, the test was skipped only on Windows. This update broadens the condition to skip the test on all non-Linux systems.
Side effects: None.
Disable Codecov coverage upload in the CI workflow
9914070 
Reason: We cannot push a simple fix due to an outdated CI configuration that has not been maintained for 2 years.
Implementation: Commented out the step responsible for uploading coverage reports to the Codecov service. This change may be temporary or used to troubleshoot coverage reporting issues. Documentation references and environment variable configurations remain intact for future reactivation if needed.
Side effects: Risk of neglecting code testing.
@wesolydexter
Copy link
Author

Since I doubt anyone will manage to configure php-coveralls, I suggest disabling it temporarily.

shakuzen
shakuzen reviewed Apr 16, 2025
View reviewed changes
Copy link
Member

@shakuzen shakuzen left a comment

Choose a reason for hiding this comment

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

The changes look reasonable to me, but like @reta, PHP is not in my area of expertise.

@wesolydexter
Copy link
Author

Shall we merge?

@wesolydexter
Copy link
Author

@reta?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shakuzen shakuzen shakuzen left review comments

@BafS BafS BafS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wesolydexter @jcchavezs @reta @shakuzen @BafS