Skip to content

[PHP 8.1 compatibility] Improve type checking for Transitions API method getTransitionsForAction #20599

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

Merged
merged 5 commits into from
Apr 21, 2023

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Apr 18, 2023

Description:

The transitions plugin getTransitionsForAction() API method doesn't handle the actionName parameter being passed as an array and throws a fatal error.

This PR allows a single element array or simple string to be correctly parsed, multi-element or empty arrays will return a NoDataForAction error.

Includes tests.

Fixes #20590

Review

@bx80 bx80 added Bug For errors / faults / flaws / inconsistencies etc. 5.0.0 labels Apr 18, 2023
@bx80 bx80 added this to the 5.0.0 milestone Apr 18, 2023
@bx80 bx80 self-assigned this Apr 18, 2023
@bx80 bx80 requested a review from michalkleiner April 18, 2023 06:49
@sgiehl
Copy link
Member

sgiehl commented Apr 18, 2023

@bx80 We maybe should stop trying to support incorrectly provided parameters. The action name is meant to be passed as a string. We do not expect it to be an array. With Matomo 5 we introduced the possibility to typehint API parameters. So we could simply add a typehint to all of the API methods that expect action name or type to be a string. If a user provides it as an array the API should then give a proper error message.

@bx80
Copy link
Contributor Author

bx80 commented Apr 18, 2023

Since we do have parameters elsewhere that do accept multiple types it's hard to know exactly what should be allowed where. Moving to type hints wherever possible will be a good way to solve this and add some certainty, though it might expose a few bugs on the way!

I'll adjust this PR to simply add type hints and use that approach for any similar issues. 👍

@sgiehl
Copy link
Member

sgiehl commented Apr 18, 2023

@bx80 we added a lot code in the past to handle incorrectly provided parameters as there was no easier way to do it and we wanted to avoid php errors to be thrown. But using the new typehints possibility makes it a lot simpler, as the API will directly return a proper error and we might actually be able to get rid of a lot error handling code in the APIs over time.

@bx80 bx80 added the Needs Review PRs that need a code review label Apr 18, 2023
@sgiehl sgiehl merged commit a3b037d into 5.x-dev Apr 21, 2023
@sgiehl sgiehl deleted the m20590-transitions-check-actionname-type branch April 21, 2023 14:23
@sgiehl sgiehl changed the title Improve type checking for the transitions getTransitionsForAction method [PHP 8.1 compatibility] Improve type checking for Transitions API method getTransitionsForAction May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For errors / faults / flaws / inconsistencies etc. Needs Review PRs that need a code review
Development

Successfully merging this pull request may close these issues.

Fatal error: htmlspecialchars_decode(): Argument #1 ($string) must be of type string, array given in Common.php
3 participants