Json last error msg/error message with location error #20629
Conversation
juan-morales
changed the title
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
It looks mostly good (nicely taken from jso :) ). Just some minor things.
Other than that I like the new format not exposing the content and just showing the location which I think is acceptable without RFC. Previously I was the only one suggesting a new function but I no longer think it's necessary.
| ZEND_PARSE_PARAMETERS_NONE(); | ||
|
|
||
| RETURN_STRING(php_json_get_error_msg(JSON_G(error_code))); | ||
| if (JSON_G(error_line) > 0 && JSON_G(error_column) > 0) { |
There was a problem hiding this comment.
This check is not necessary as it is checked in php_json_get_error_msg_with_location. If it was to save creation of zend_string, then it is not necessary because it is created in RETURN_STRING anyway.
| zend_throw_exception(php_json_exception_ce, ZSTR_VAL(error_msg), error_code); | ||
| zend_string_release(error_msg); |
There was a problem hiding this comment.
| zend_throw_exception(php_json_exception_ce, ZSTR_VAL(error_msg), error_code); | |
| zend_string_release(error_msg); | |
| zend_throw_exception_zstr(php_json_exception_ce, error_msg, error_code); |
There was a problem hiding this comment.
ah good catch (I was just searching for something like that but didn't realise it's not in the header...). Let's wait for that first commit in the PR then...
|
|
||
| parser->scanner.errloc.first_column = location->first_column; | ||
| parser->scanner.errloc.first_line = location->first_line; | ||
| parser->scanner.errloc.last_column = location->last_column; | ||
| parser->scanner.errloc.last_line = location->last_line; |
There was a problem hiding this comment.
Is this necessary? Just checking because I don't have this in jso and it seems to still work so wondering why it's added here?
|
|
||
| #define PHP_JSON_INT_MAX_LENGTH (MAX_LENGTH_OF_LONG - 1) | ||
|
|
||
| #define PHP_JSON_TOKEN_LENGTH(s) ((size_t) ((s)->cursor - (s)->token)) |
There was a problem hiding this comment.
I guess s is kind of given in other macros so I would omit it here as well for consistency.
4 participants
This PR pretends to add the location where the error occurs when processing a json string (
json_validate()orjson_decode()). As the message suggests, is the "nearby location", like MySQL does it when there is an error with a SQL string.I tried to achieve this many times in the past, but failed, my previous approaches were .... terrible and error-prone.
Now I used a different approach suggested by @bukka . I hope I get closer to a descent implementation. Did my best on this one.
I tested with JSON string with 1GB long, and values were calculated correctly for rows and columns.
Thanks for reviewing and I hope this helps