WIP - RFC - json error message enhanced #14672
Conversation
ext/json/json_parser.y
Outdated
|
|
||
| parser->scanner.error_msg = (php_json_ctype *) malloc(strlen(error_message) + 1); | ||
|
|
||
| memcpy(parser->scanner.error_msg, error_message, strlen(error_message) + 1); |
There was a problem hiding this comment.
I'm not a regular C coder and this may be a stupid question: but is this code "safe"?
strlen requires a \0 but with the char error_message[256]; and sizeof(error_message) and later the code does malloc respectively memcpy on strlen.
Is there a guarantee that the char error_message[256]; contains a \0, even if it's fully filled?
Apologies if this is all handled and not appropriate, I glanced at this and just wasn't sure.
There was a problem hiding this comment.
Honestly, I dont know exactly, BUT I will make this Perfect and safe during the se days.
I just wanted to have a proof of concept code.
thanks a lot. if its ok with you, i can ping/tag you When I update this code. Let me know.
once again, thanks
|
I have been already looking into this in past and implemented some sort of solution in parser and scanner in jso ages ago. It has got simillar parser and scanner so the idea was to first to implement it there and then to PHP. I recently revisited it but the future plan is to actually replace the current scanner and parser with simd based parser so I postponed integration of that to prevent any potential break with in the lines and basically implementing the same thing twice. So the state is that I don't want to spend too much time on the current parser improvements as it might go in the future. If you really wanted to progress on this I would recommend not to change the current messages which is not that helpful and might break some code potentially but maybe introduce something like json_last_error_info that would return array containing message, code and token start and end position instead (see JSO code how it's done in the parser and scanner for that). |
|
@bukka thanks for the feedback. I will move on with the "json_last_error_info" option. Its ok if in the future this and something else gets replaced with a new parser technology, but because we dont know exactly when that is going to happen, lets make this real now and available for our users, including Dereck, which requested this publicly 😄 (thanks for everything Dereck) So, I will re-do all this towards this new strategy, and when done , I will ping you @bukka . Also please, if you remember, let me know when working with the new parser in the future, I am a newby, but once in a while I get some edge-corner ideas that might help you. |
|
here appears that I requested a code review form you @kocsismate but I did not do it. FYI 😄 |
I don't think that error messages fall under backwards compatibility guarantees anywhere. They are meant for human consumption, not for the computer. I'm strongly in favor of improving existing error messages instead of adding new functionality to obtain the necessary information, because then everyone would automatically benefit from it. |
I will propose this RFC today, but I did a PoC change just to have an idea about how to do it.
In short, the idea is to have better error messages, instead of having an error message saying "Syntax Error", to have something like "Syntax Error - error at character X near content " foo: bar }" .... 😄
This is still Work in progress.
Eventhough is not finished, and I have to write the email to the email list, I want to say ...... Thanks in advance @derickr because you gave me the idea, ... exactly on this moment of history:
What's New in PHP 8.3 - Derick Rethans
I also tag you @bukka so you know since now, that I am doing this, as all this part of php was mainly ... you 😄
still Work In Progress, and will be done soon. Hope this change makes sense, and also makes PHP greater.