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

Do not try to explode() return parameter when it already is an array #29

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Do not try to explode() return parameter when it already is an array #29

merged 1 commit into from
Mar 20, 2024

Conversation

jensschuppe
Copy link
Contributor

On PHP 8, explode() throws a TypeError instead of issueing a warning, so requests will fail without a proper error message when the return parameter is not a string.

This PR adds a check for whether the return parameter already is an array. I'm not sure it will ever be a string, as in my testing even for just a single value this was an array.

@jensschuppe jensschuppe added bug Something isn't working status:needs review Code needs review and testing. labels Mar 14, 2024
@jensschuppe jensschuppe requested a review from dontub March 14, 2024 14:13
@dontub dontub removed the status:needs review Code needs review and testing. label Mar 19, 2024
@jensschuppe jensschuppe added the status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. label Mar 19, 2024
dontub
dontub requested changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@dontub dontub left a comment

Choose a reason for hiding this comment

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

Just seen that \CRM_Utils_Array::value() is deprecated. So this should be replaced.

@jensschuppe jensschuppe added status:needs work There is code, but it needs additional work before it should be reviewed. and removed status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. labels Mar 19, 2024
@jensschuppe jensschuppe requested a review from dontub March 19, 2024 12:11
@jensschuppe jensschuppe added status:needs review Code needs review and testing. and removed status:needs work There is code, but it needs additional work before it should be reviewed. labels Mar 19, 2024
dontub
dontub reviewed Mar 20, 2024
View reviewed changes
Civi/RemoteToolsRequest.php Outdated Show resolved Hide resolved
@jensschuppe jensschuppe added status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. and removed status:needs review Code needs review and testing. labels Mar 20, 2024
@jensschuppe jensschuppe merged commit 85f80ce into master Mar 20, 2024
9 checks passed
@jensschuppe jensschuppe deleted the returnNoExplode branch March 20, 2024 10:32
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code). and removed status:reviewed and tested Code has received thorough review and testing and is ready to be committed/merged. labels Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dontub dontub dontub approved these changes

Assignees
No one assigned
Labels
bug Something isn't working status:fixed The issue has been resolved (usually by committing/merging code).
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jensschuppe @dontub