Skip to content

Apply the same ECMA to PCRE pattern replacements everywhere - #943

Open
Amoifr wants to merge 1 commit into
jsonrainbow:mainfrom
Amoifr:fix-917-ecma-pcre-replacements
Open

Amoifr wants to merge 1 commit into
jsonrainbow:mainfrom
Amoifr:fix-917-ecma-pcre-replacements

Conversation

@Amoifr

@Amoifr Amoifr commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

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.

Replacement Needed? Why
\d[0-9] yes under /u, PCRE's \d matches U+0663 ٣; ECMA-262 defines \d as ASCII only
\D[^0-9] yes the negation of the above
\w[A-Za-z0-9_] yes under /u, PCRE's \w matches é; ECMA's does not
\W[^A-Za-z0-9_] yes the negation
\p{digit} → … yes PCRE rejects \p{digit} outright, preg_match() returns false
\p{Letter}\p{L} yes same, PCRE rejects the long name

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

\d is commented out in AdditionalPropertiesConstraint but active in PatternPropertiesConstraint, and the two are consulted for the same property name. On main:

{"patternProperties": {"^\\d+$": {"type": "string"}}, "additionalProperties": false}

{"٣": "ok"} is accepted. Per ECMA \d is [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 what AdditionalPropertiesConstraint already used.

One line I am not comfortable deciding for you

\s[\s\x{200B}] widens \s beyond ECMA rather than narrowing it to match: U+200B is Cf, not Zs, so ECMA's \s does not include it. I kept the mapping exactly as PatternConstraint has it today, so pattern behaviour 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 createPregMatchPattern are 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inconsistent and incomplete ECMA-to-PCRE pattern replacements in createPregMatchPattern

2 participants