Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #917.
The issue asks for a decision on the commented-out replacements. Rather than pick one, here is what each line does, measured against PCRE on PHP 8.5.
\d→[0-9]/u, PCRE's\dmatches U+0663٣; ECMA-262 defines\das ASCII only\D→[^0-9]\w→[A-Za-z0-9_]/u, PCRE's\wmatchesé; ECMA's does not\W→[^A-Za-z0-9_]\p{digit}→ …\p{digit}outright,preg_match()returns false\p{Letter}→\p{L}So none of them is dead weight: each one exists because PCRE and ECMA disagree, and leaving it out changes what a schema means.
The bug it causes today
\dis commented out inAdditionalPropertiesConstraintbut active inPatternPropertiesConstraint, and the two are consulted for the same property name. Onmain:{"patternProperties": {"^\\d+$": {"type": "string"}}, "additionalProperties": false}{"٣": "ok"}is accepted. Per ECMA\dis[0-9], so٣matches no pattern, is an additional property, and should be rejected.{"5": "ok"}is accepted either way. This is now covered by a test per draft, each of which fails without the change.\p{digit}The two mappings in the tree are not equivalent, so this was not just a cosmetic inconsistency.
[0-9]drops non-ASCII digits,\p{Nd}keeps them, and\p{Nd}is what the ECMA property actually means. I converged on\p{Nd}, which is whatAdditionalPropertiesConstraintalready used.One line I am not comfortable deciding for you
\s→[\s\x{200B}]widens\sbeyond ECMA rather than narrowing it to match: U+200B isCf, notZs, so ECMA's\sdoes not include it. I kept the mapping exactly asPatternConstrainthas it today, sopatternbehaviour is unchanged, but propagating it to the other files does widen them. If the zero width space handling was added for a concrete reason, a comment saying so would settle it; if not, dropping it everywhere would be the more faithful reading. Happy to do either in a follow-up.Not done
The nine copies of
createPregMatchPatternare now identical, which is how they drifted apart in the first place. A shared helper would make "consistent across all files" structural rather than a copy-paste convention, but that is an architecture call you did not ask for here, so I left it. Say the word and I will send it.Full suite green at 3169 tests, including the official JSON-Schema-Test-Suite. PHPStan clean.