Skip to content

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Jun 25, 2025

Summary

The code style update in pull request #53625 appears to trigger a Psalm warning about Response returning null, which makes it non-compliant with the interface.

Checklist

@kesselb kesselb force-pushed the debt/noid/wrong-return-type-iresponse branch from 2781784 to 636af10 Compare June 25, 2025 13:34
@kesselb kesselb self-assigned this Jun 25, 2025
@kesselb kesselb added 3. to review Waiting for reviews technical debt labels Jun 25, 2025
@kesselb kesselb added this to the Nextcloud 32 milestone Jun 25, 2025
@kesselb kesselb marked this pull request as ready for review June 25, 2025 13:35
@kesselb kesselb requested a review from a team as a code owner June 25, 2025 13:35
@kesselb kesselb requested review from leftybournes and removed request for a team June 25, 2025 13:35
@kesselb kesselb marked this pull request as draft June 25, 2025 13:43
@kesselb
Copy link
Contributor Author

kesselb commented Jun 25, 2025

Changing the status back to draft, as there are some other changes needed.

image

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 25, 2025
@kesselb kesselb force-pushed the debt/noid/wrong-return-type-iresponse branch 2 times, most recently from a0aef55 to 33feb34 Compare June 27, 2025 08:57
@codecov
Copy link

codecov bot commented Jun 27, 2025

❌ Unsupported file format

Upload processing failed due to unsupported file format. Please review the parser error message:
Error deserializing json

Caused by:
expected value at line 1 column 1

For more help, visit our troubleshooting guide.

@kesselb kesselb marked this pull request as ready for review June 27, 2025 09:07
@kesselb kesselb added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Jun 27, 2025
return $response->getBody();
$content = $response->getBody();

if (is_resource($content)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m always scared by is_resource calls because of the long ongoing migration from resources to objects in PHP.
See https://php.watch/articles/resource-object
File handles are not planned to move to object yet but that may happen someday.

What about:

				if (is_string($content) || $content === null) {
					return false;
				}
				return $content;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

@kesselb kesselb force-pushed the debt/noid/wrong-return-type-iresponse branch from 33feb34 to de54bdb Compare June 30, 2025 09:50
@susnux susnux merged commit 59ae739 into master Jun 30, 2025
222 of 226 checks passed
@susnux susnux deleted the debt/noid/wrong-return-type-iresponse branch June 30, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants