Skip to content

Pattern directory: extend validation test coverage - #773

Merged
obenland merged 1 commit into
trunkfrom
pattern-directory/validation-test-coverage
Sep 14, 2026
Merged

obenland merged 1 commit into
trunkfrom
pattern-directory/validation-test-coverage

Conversation

@obenland

Copy link
Copy Markdown
Member

Follow-up test coverage for the content validation helpers in pattern-validation.php.

content_has_block_directives() scans both the submitted markup and its sanitised
form, since some elements hide their contents from WP_HTML_Tag_Processor while KSES
drops the wrapper on save and keeps what it held. The data providers covered that for
raw-text elements in the HTML namespace, but not for the foreign-content case, which
reaches the same end state by a different route and so is worth pinning separately.

  • pattern-content-validation-test.php — two cases covering the foreign-content shape,
    closed and unclosed.
  • pattern-translated-content-test.php — the same shape on the translated-content path,
    which shares the helper but is a separate entry point.

Both cases fail if the sanitised-markup pass is removed from the helper, so they pin the
current behaviour rather than restating it.

Tests only; no change to validation behaviour. Full PHPUnit suite (226 tests) and phpcs
both pass locally.

🤖 Generated with Claude Code

The directive checks already scan both the submitted markup and its sanitised
form, but the data providers only exercised elements the tokenizer skips in the
HTML namespace. Add the foreign-content equivalent, where a nested raw-text
element hides its contents from the tokenizer for a different reason, and cover
the same shape on the translated-content path that shares the helper.

Tests only; no change to validation behaviour.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings September 14, 2026 18:35

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@obenland
obenland merged commit 8462462 into trunk Sep 14, 2026
4 checks passed
@obenland
obenland deleted the pattern-directory/validation-test-coverage branch September 14, 2026 18:50
@bor0

bor0 commented Sep 16, 2026

Copy link
Copy Markdown
Member

Late to this one (already merged), but a note for whoever touches these providers next.

The rows do what the description says — they fail if the sanitised-markup pass is removed — so the coverage is real. The stated reason isn't, though: <svg> isn't what hides the anchor. WP_HTML_Tag_Processor walks straight into <svg>, <desc> and <annotation-xml>; a directive on a tag inside plain <svg> is found on the first pass. What hides it is <script> being a raw-text element, which is the row immediately above in the same provider. So these are the existing case with a wrapper that doesn't change the outcome, rather than a second route — and the new comments in both files now assert the opposite.

While looking at it I found the shape that genuinely isn't covered, which I'd rather not spell out here. Short version: the two-pass approach works because the sanitiser removes the element that did the hiding, and that isn't true for every such element — for the ones it keeps, both passes miss. Sent the detail and a suggested fix through the usual channel; happy to open a follow-up PR if that's easier.

@obenland

Copy link
Copy Markdown
Member Author

@bor0 Please feel free to just fix it

@bor0

bor0 commented Sep 16, 2026

Copy link
Copy Markdown
Member

Done — #777.

It turned out the cleanest fix was to stop reading the content as markup at all. The rule is that a pattern may not carry data-wp- anywhere, and that is a substring test; the helper already ran exactly that as its fast-path negative, so promoting it to the decision covers every hiding place and drops the sanitise-and-tokenize pass. The shape that wasn't covered was an element whose contents are text and which the sanitiser keeps, so neither pass ever reached the tag.

Your rows are kept as regression coverage, with the explanations corrected. CI is green.

One thing worth your call in the PR: it widens the rule, so prose that merely mentions data-wp- is refused too. Zero of the 600 published patterns I sampled and zero of the 1,858 seed patterns carry the marker in the fields this reads, so it looks free — but there's a provider row pinning that case, and it's easy to drop if you'd rather leave prose alone.

bor0 added a commit that referenced this pull request Sep 18, 2026
…arkup (#777)

`content_has_block_directives()` tokenized the content, and the sanitised
copy of it, looking for a tag with a `data-wp-*` attribute. That is narrower
than the rule it enforces: a pattern may not carry the marker at all.

`WP_HTML_Tag_Processor` does not descend into an element whose contents are
text, so a tag inside one is invisible to it. Scanning the `wp_kses_post()`
form as well covers that only where KSES removes the element that did the
hiding. It removes `<script>` and `<style>`, which is why the existing cases
are caught, and it keeps `<title>` and `<textarea>`, which hold their
contents as text just the same -- so for those, neither pass ever reads the
tag and the check returns false.

Test the marker directly. It is what the helper already did as a fast-path
negative, it cannot be hidden by an element boundary, and it drops the
sanitise-and-tokenize pass.

The widening this brings is deliberate: content that merely mentions
`data-wp-` in prose is now refused too. Checked against the fields the
validator reads (`post_content`, `post_title`, `post_excerpt`) on 600
published patterns from the public API and the 1,858 in the seed exports --
none carries the marker. The 40 of 600 that do carry it hold it in the
rendered output core emits for blocks like `core/details`, which this never
reads.

Tests: rows for `<title>`, `<textarea>` and the marker in plain prose, in
both the submission and translated-content providers; the first two fail
before this change. The comments on the existing rows described tokenizer
behaviour that no longer decides the outcome, and the two added in #773
described it incorrectly -- `<svg>` is not a raw-text element and the tag
processor reads straight into it; the nested `<script>` was doing the hiding.
Rows kept as regression coverage, explanations corrected.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants