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.
ext/json/json.c
Outdated
| 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.
There was a problem hiding this comment.
your are right. I will use :
RETVAL_STR(php_json_get_error_msg_with_location(
JSON_G(error_code),
JSON_G(error_line),
JSON_G(error_column)
));RETURN_STRING is expecting other data types and when I am changing the code ... I mess it up.
ext/json/json_parser.y
Outdated
|
|
||
| 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?
There was a problem hiding this comment.
hmmm, it works, you are right. I added it since the beginning of my change assuming they were needed but ... well, they are not.
ext/json/json_scanner.re
Outdated
|
|
||
| #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.
|
Hello everyone. Thanks for the review. I am addressing each one of the comments today or tomorrow. I have being traveling . |
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.
Credits:
Besides some reading about bison, etc. ... @bukka pointed to https://github.com/libjso/jso which basically exposed me how to port its code to the PHP parser.