correction possible bug maybe if fast-path detects an UTF-8 violation…

[LLyaudet]

… but my code doesn't.

[LLyaudet]

@Seldaek I found a possibility with low probability that my code may hide an UTF-8 violation that would be detected by mb_check_encoding or preg_match. It shouldn't be the case with current UTF-8. But if some version of Unicode or another RFC for UTF-8 add further restrictions, it may be the case that mb_check_encoding or preg_match will be patched before my code. So this PR will allow to detect it.

[LLyaudet]

@Seldaek Sorry for the 3 force-with-lease. It should be almost pixel perfect now. I don't see anything else to improve before merging.

[Seldaek]

I found a possibility with low probability that my code may hide an UTF-8 violation that would be detected by mb_check_encoding or preg_match.

Good point, but I think the code could be simplified a lot, not sure why you added that new argument but see last commit I pushed does that make sense to you?

[LLyaudet]

Hello @Seldaek :),
I think my code was better with the argument for two reasons:

• This way you can test with or without fast-path.
• You added in the README that Utf8Validator class could be used independently, and I think it makes sense to have this possibility for the user. With my unmodified code, the default behavior is similar to what it is now.

In fact, from a feature complete point of view, I'm wondering if there would be real use cases, where it is convenient to use only the fast path inside some code, but be able to replay some "data integration" with my enhanced UTF-8 error code detection to analyze what happened (post-mortem) without having to modify a lot of code.
I know it may be seem complicated, but I do know by thinking outside of the box frequently, that if you give possibilities to users, some are clever enough to find a valid use case.

[LLyaudet]

Imagine something like a DDOS protection, where it is fast-path only, and only random sampling of the attack data is done for full analysis.
Etc.
I see no problem if you want to keep your code simpler.
I will add "creeping featurism" in mine.
I love "creeping featurism" :).

[Seldaek]

• This way you can test with or without fast-path.

You could skip the fast path yes. But the only tests worth it are testing that the code below detects an error, and if it detects an error the fast path will not early-return, so it will run the code below. So I don't think this is a valid point.

• You added in the README that Utf8Validator class could be used independently, and I think it makes sense to have this possibility for the user. With my unmodified code, the default behavior is similar to what it is now.

I don't see how disabling the fast path is useful to any one, it just removes an optimization, runs slower, but will not detect anything more or less. If you want to skip detection for some and do sub-sampling then you could just call the Utf8Validator for a subset of content?

[LLyaudet]

You could skip the fast path yes. But the only tests worth it are testing that the code below detects an error, and if it detects an error the fast path will not early-return, so it will run the code below. So I don't think this is a valid point.

I would not assume that the only tests worth it are on my code.
From a paranoid point of view, comparing stdlib behavior and my code behavior makes sense, hence atomic activations of fast-path and slow-path makes sense.
Testing stdlib should be the responsibility of stdlib maintainers, but "argument of authority supply chain attacks" may exist.
Moreover, if you add performance tests/metrics (it doesn't require paranoia), you may want shiny graphics to see how performance evolved between versions of stdlib/PHP, etc., and how the fast-path and the slow-path compare.

I don't see how disabling the fast path is useful to any one, it just removes an optimization, runs slower, but will not detect anything more or less. If you want to skip detection for some and do sub-sampling then you could just call the Utf8Validator for a subset of content?

Imagine you disabled the slow-path during a DDOS, but sampled files for post-mortem analysis.
After that all sampled files are faulty.
You want precise error messages.
You disable the fast-path, since it brings nothing.
You may say, but then the performance gain by avoiding redoing fast-path is weak.
But imagine that your sampling took less than 1/10000 of the data, but you still have more than GB of data, and that the post-mortem analysis is done on huge amount of data by an AI, etc.
You can always "imagine", but "reality surpasses fiction" (La réalité dépasse la fiction.).

[Seldaek]

I see but yeah I think I'm gonna have to say I disagree and maybe it's best to keep the feature creep to your other project then ;)

I think this is good enough for the purposes of this project.

[LLyaudet]

I think this is good enough for the purposes of this project.

Probably.
May I ask you to warn me (ping me in GitHub, create an issue on DevOrSysAdminScripts, or send me an email) if some case of this exception is reported some day to jsonlint, please ?

[Seldaek]

Yup :) You can also watch this repo for new issues, because it sees such low traffic as it is quite stable, it shouldn't spam you too much with irrelevant infos.