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

[PHP 8.4] Explicitly mark nullable parameters #22693

Merged
merged 1 commit into from
Oct 18, 2024
Merged

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Oct 17, 2024

Description:

With PHP 8.4 implicitly marking parameters as nullable by only setting null as default value is deprecated.
To get rid of all the deprecation notices caused by our code, this PR aims to change all occurrences to explicitly mark the parameters as nullable.

refs #22471

Review

@sgiehl sgiehl added the Needs Review PRs that need a code review label Oct 17, 2024
@sgiehl sgiehl added this to the 5.2.0 milestone Oct 17, 2024
@sgiehl sgiehl requested a review from a team October 17, 2024 08:13
@michalkleiner
Copy link
Contributor

Did you use a tool to convert the params or did you do it manually?

@@ -174,7 +174,7 @@ public function getPrettyPercentFromQuotient($value)
* This parameter is not currently supported and subject to change.
* @api
*/
public function formatMetrics(DataTable $dataTable, Report $report = null, $metricsToFormat = null, $formatAll = false)
public function formatMetrics(DataTable $dataTable, ?Report $report = null, $metricsToFormat = null, $formatAll = false)
Copy link
Contributor

@michalkleiner michalkleiner Oct 17, 2024

Choose a reason for hiding this comment

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

It's annoying we can't use mixed or union type here yet, however it looks like this can be typed to array based on its use?

@@ -174,7 +174,7 @@ public function getPrettyPercentFromQuotient($value)
* This parameter is not currently supported and subject to change.
* @api
*/
public function formatMetrics(DataTable $dataTable, Report $report = null, $metricsToFormat = null, $formatAll = false)
public function formatMetrics(DataTable $dataTable, ?Report $report = null, $metricsToFormat = null, $formatAll = false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function formatMetrics(DataTable $dataTable, ?Report $report = null, $metricsToFormat = null, $formatAll = false)
public function formatMetrics(DataTable $dataTable, ?Report $report = null, ?array $metricsToFormat = null, $formatAll = false)

@@ -61,7 +61,7 @@ class Updater
* for the plugin name.
* @param Columns\Updater|null $columnsUpdater The dimensions updater instance.
*/
public function __construct($pathUpdateFileCore = null, $pathUpdateFilePlugins = null, Columns\Updater $columnsUpdater = null)
public function __construct($pathUpdateFileCore = null, $pathUpdateFilePlugins = null, ?Columns\Updater $columnsUpdater = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function __construct($pathUpdateFileCore = null, $pathUpdateFilePlugins = null, ?Columns\Updater $columnsUpdater = null)
public function __construct(?string $pathUpdateFileCore = null, ?string $pathUpdateFilePlugins = null, ?Columns\Updater $columnsUpdater = null)

@sgiehl
Copy link
Member Author

sgiehl commented Oct 18, 2024

@michalkleiner I actually used a regex to find those and fixed them manually. Might be possible that I missed some though.
But I didn't want to introduce new type hints on purpose and only fixed those that had an implicit nullable value.

@sgiehl sgiehl requested a review from a team October 18, 2024 12:21
@michalkleiner
Copy link
Contributor

implicit nullable value.

Implicit nullable typed value. Fair enough.

@sgiehl sgiehl merged commit 83adf01 into 5.x-dev Oct 18, 2024
26 checks passed
@sgiehl sgiehl deleted the implicitnullable branch October 18, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants