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

make sure that parameters are a string when string is expected #40944

Merged
merged 1 commit into from
Aug 25, 2023

Conversation

phil-davis
Copy link
Contributor

Description

Functions like explode hash strpos strtolower expect a string as input. But sometimes the code is passing something that can be null. That is normally converted to an empty string on-the-fly by PHP. For the future, it is better to explictly cast to a string where we know this can happen "normally".

How Has This Been Tested?

CI

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@phil-davis phil-davis self-assigned this Aug 25, 2023
@owncloud owncloud deleted a comment from update-docs bot Aug 25, 2023
@phil-davis phil-davis marked this pull request as ready for review August 25, 2023 10:00
@phil-davis phil-davis changed the title [WIP] Explicitly cast to string when string is expected Explicitly cast to string when string is expected Aug 25, 2023
@phil-davis phil-davis requested a review from jvillafanez August 25, 2023 10:01
@phil-davis
Copy link
Contributor Author

This sort of thing was noticed by https://github.com/bohwaz in https://github.com/bohwaz/owncloud-core-php8
So I am suggesting that we adjust these that he and I have found.

@jvillafanez
Copy link
Member

I partially disagree with the solution.
If the string could be null, then you should be able to handle that null value somehow. In a lot of scenarios, if the value is null, the behavior will be different than if the value is a string.

In general, I think it's better something like $stringValue = $value ?? '' or if ($value === null) {....} since the null value is explicitly handled the way we want.

I'd follow the same ruling I have with == vs ===: if you want to cast to a string, include a comment to explicitly say that the casting is intended and all the possible scenarios are handled the way we want.
For example, for the (string)$request->getHeader('depth') something like "the getHeader could return null so we'll consider it as an empty string", could be good enough to know that it could be null and that case is handled the way we want.

@phil-davis phil-davis changed the title Explicitly cast to string when string is expected make sure that parameters are a string when string is expected Aug 25, 2023
@phil-davis
Copy link
Contributor Author

I partially disagree with the solution.

I agree with you - I have adjusted the code and the changelog...

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

71.4% 71.4% Coverage
0.0% 0.0% Duplication

@phil-davis phil-davis merged commit 983e28b into master Aug 25, 2023
@delete-merged-branch delete-merged-branch bot deleted the cast-to-string branch August 25, 2023 16:28
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.

2 participants