fix: handle invalid JSON in loadJsonAsArray to prevent TypeError#33
Conversation
src/FeedIo/Reader/Document.php
Outdated
| return json_decode($this->content, true); | ||
| $result = json_decode($this->content, true); | ||
|
|
||
| if ($result === null && json_last_error() !== JSON_ERROR_NONE) { |
There was a problem hiding this comment.
What if it failed and the result is ""? I'd assume we want to know there too?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Grotax I saw you pushed some changes, is this good to merge? |
|
My comments are still there, waiting to be resolved. |
Can you review again? |
src/FeedIo/Reader/Document.php
Outdated
| ? json_last_error_msg() | ||
| : 'expected array or object, got ' . gettype($result); | ||
| throw new \InvalidArgumentException( | ||
| 'malformed JSON string. parsing error: ' . $errorMsg |
There was a problem hiding this comment.
Just because it's not an array doesn't make it malformed. You'll need to throw on a failing json decode, or you need to do complete format validation and just ignoring the json error status.
The current implementation is incomplete and incorrect.
There was a problem hiding this comment.
You'll need to throw on a failing json decode, or you need to do complete format validation and just ignoring the json error status. - is this direction?
There was a problem hiding this comment.
@SMillerDev can you review my last commit?
Thank you
eureka928
force-pushed
the
fix/json-decode-null-handling
branch
from
db7ba2e to
88937ab
Compare
|
Hi @Grotax would you run the test? and review once more? |
Grotax
force-pushed
the
fix/json-decode-null-handling
branch
from
43affe9 to
ca32a8b
Compare
When content starts with '{' but isn't valid JSON, json_decode()
returns null which violates the array return type declaration,
causing a TypeError.
Fixes nextcloud/news#3129
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: eureka928
Grotax
force-pushed
the
fix/json-decode-null-handling
branch
from
ca32a8b to
0eefb45
Compare
3 participants
Summary
loadJsonAsArray()to handle invalid JSON contentInvalidArgumentExceptionwith descriptive message when JSON parsing failsProblem
When content starts with
{(detected as JSON byisJson()), but the content isn't actually valid JSON,json_decode()returnsnull. This violates thearrayreturn type declaration onloadJsonAsArray(), causing aTypeError:This can happen when a feed URL returns an error response that happens to start with
{, for example:{"data":null,"status":"error","error":{"code":1000,"message":"not found"}(Note the missing closing
})Solution
Add proper error checking similar to the existing
loadDomDocument()method:json_decode()returnednulljson_last_error()to distinguish parse errors from valid null JSONInvalidArgumentExceptionwith a descriptive error messageRelated Issues
Fixes nextcloud/news#3129