fix(theme): split includes/excludes on every separator, not just newlines - #227
fix(theme): split includes/excludes on every separator, not just newlines#227ZaredRogers wants to merge 1 commit into
Conversation
…ines
The Price Includes/Excludes lists collapsed into a single bullet whenever a
tour was imported from WETU or the field was touched in the editor.
`included` and `not_included` are CMB2 wysiwyg fields that the WETU importer
also writes to verbatim, so a value arrives in one of four shapes. The filter
split on newline characters only, but neither writer produces newlines:
- `<p>One<br>Two</p>` TinyMCE soft breaks, and every WETU payload
- `<p>One</p><p>Two</p>` TinyMCE hard breaks
- `"One\r\nTwo"` legacy plain text, the only shape that worked
- `<ul><li>One</li></ul>` real list markup, passed through without the
class, so it never received its icons
Only 4 of 76 stored values were in the shape the filter understood.
Normalise `<br>`, `<p>`/`</p>` and `\r\n`/`\r` to one delimiter before
splitting, and add the list class to values already stored as list markup so
those get icons too. Also collapse ` ` so an emptied wysiwyg field
renders nothing instead of a stray non-breaking space.
Icon injection now matches `<li>` with attributes, handles `<ol>` and nested
lists, and the two near-identical branches collapse into one loop. The CSS
drops its `ul` qualifier so an editor-entered `<ol>` keeps the layout.
Verified against every stored value on a local copy of the site: 76/76 rows
render as multi-item lists (563 items, up from ~76), 563/563 icons injected,
correct icon on all 76 lists, plus a 12-case edge matrix. No stored data
changed; the fix is entirely in the render path.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this to Backlog
in Client - African Safaris New Zealand
📝 WalkthroughSummary by CodeRabbit
WalkthroughList metadata processing now supports multiple separators, removes empty values, preserves existing list markup, and adds list classes. Icon rendering uses those classes to inject SVGs into list items. CSS now targets the classes on any element type. ChangesList rendering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MetadataFilter
participant ListHelpers
participant WP_HTML_Tag_Processor
participant IconRenderer
participant CSS
MetadataFilter->>ListHelpers: normalise and split metadata
ListHelpers->>WP_HTML_Tag_Processor: add list class
MetadataFilter->>IconRenderer: provide classified list markup
IconRenderer->>IconRenderer: prepend matching SVG to each list item
CSS->>IconRenderer: apply class-based list styles
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.14.0)style.cssConfigurationError: Could not find " Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Tick the box to add this pull request to the merge queue (same as
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@functions.php`:
- Around line 349-352: Update the `<li>` opening-tag injection in the
bound-paragraph handling around `$block_content` so `>` characters inside quoted
attribute values do not terminate the match; use an HTML-aware insertion
approach or a pattern that consumes quoted attributes while preserving the full
tag. Add a regression test covering an attribute such as `data-label="A > B"`
and verify the icon is inserted only after the complete opening tag.
- Around line 238-240: Resolve the WordPress compatibility mismatch around the
WP_HTML_Tag_Processor guard in asnz_add_list_meta_class(): either raise the
theme’s minimum required WordPress version to 6.2, or implement a safe fallback
parser for supported 5.8 installs that still adds
lsx-included-list/lsx-not_included-list and preserves icon styling. Keep the
existing processor path unchanged where available.
In `@style.css`:
- Around line 1301-1307: Update the .lsx-included-list li and
.lsx-not_included-list li layout so direct nested ul or ol elements wrap beneath
the parent item text rather than appearing beside it; preserve the existing icon
alignment and add indentation for nested content.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eca0c0e-f993-4e31-b4a2-7888e6b88ac5
📒 Files selected for processing (2)
functions.phpstyle.css
| if (!class_exists('\WP_HTML_Tag_Processor')) { | ||
| return $html; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -i -C 3 \
'Requires at least:|Tested up to:|WP_HTML_Tag_Processor|wp_version' \
-g '*.php' -g 'readme.txt' -g '*.md' -g 'composer.json' .Repository: lightspeedwp/asnz-block-theme
Length of output: 1435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== theme header =="
sed -n '1,24p' readme.txt
echo "== functions.php relevant sections =="
if [ -f functions.php ]; then
wc -l functions.php
sed -n '220,285p' functions.php
echo "== references to asnz_add_list_meta_class / list renderer / css =="
rg -n "asnz_add_list_meta_class|list_meta_class|WP_HTML_Tag_Processor|<style|wp_enqueue_style|customize_reader_visible|render" functions.php readme.txt -g '*.php' -g '*.txt'
fi
echo "== composer/package files =="
git ls-files | rg '(^composer\.(json|lock)$|package\.json$|readme\.txt$|README\.md$|LICENSE|style\.css$)' || trueRepository: lightspeedwp/asnz-block-theme
Length of output: 5786
🌐 Web query:
WordPress WP_HTML_Tag_Processor introduced version
💡 Result:
The WP_HTML_Tag_Processor class was introduced in WordPress 6.2 [1][2]. It is part of the HTML API, designed to provide a safe, reliable, and HTML5-spec-compliant way to parse HTML and modify tag attributes in PHP [3][4]. The class was developed initially within the Gutenberg repository before being merged into WordPress Core [4][5].
Citations:
- 1: https://make.wordpress.org/core/2023/03/07/introducing-the-html-api-in-wordpress-6-2/
- 2: https://wp-kama.com/function/WP_HTML_Tag_Processor
- 3: Editor: Introduce HTML Tag Processor WordPress/wordpress-develop#3920
- 4: https://core.trac.wordpress.org/ticket/57575
- 5: Editor: Introduce HTML Tag Processor WordPress/wordpress-develop#3920.
Require WordPress 6.2+ or add a list-class injection fallback.
WP_HTML_Tag_Processor is available from WordPress 6.2, but the theme metadata still allows 5.8. On supported 5.8 installs, existing list markup bypasses asnz_add_list_meta_class(), so lsx-included-list / lsx-not_included-list and the icon styles are not applied. Update Requires at least: 6.2 or add an alternative parser that injects the class while remaining safe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions.php` around lines 238 - 240, Resolve the WordPress compatibility
mismatch around the WP_HTML_Tag_Processor guard in asnz_add_list_meta_class():
either raise the theme’s minimum required WordPress version to 6.2, or implement
a safe fallback parser for supported 5.8 installs that still adds
lsx-included-list/lsx-not_included-list and preserves icon styling. Keep the
existing processor path unchanged where available.
| // A bound paragraph holds exactly one meta value, so every list item in | ||
| // the block belongs to this list, including any the editor nested. | ||
| $block_content = preg_replace('/(<li\b[^>]*>)/i', '$1' . $icon, $block_content); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle > inside quoted <li> attributes.
A small parser gremlin is here. The matcher treats the first > as the end of the opening tag. Valid markup such as <li data-label="A > B"> becomes malformed when the SVG is inserted. Use an HTML-aware insertion method, or a matcher that recognises quoted attribute values. Add this case to the icon-injection tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@functions.php` around lines 349 - 352, Update the `<li>` opening-tag
injection in the bound-paragraph handling around `$block_content` so `>`
characters inside quoted attribute values do not terminate the match; use an
HTML-aware insertion approach or a pattern that consumes quoted attributes while
preserving the full tag. Add a regression test covering an attribute such as
`data-label="A > B"` and verify the icon is inserted only after the complete
opening tag.
There was a problem hiding this comment.
Pull request overview
This PR fixes the “Price Includes / Price Excludes” rendering on single tour pages by making the theme’s render-time processing correctly split WYSIWYG/imported values on HTML separators (e.g. <br>, <p>) rather than only on newline characters, and by ensuring already-stored list markup receives the expected list class so icons apply.
Changes:
- Add helpers to normalise/split
included/not_includedmeta values into distinct list items regardless of whether they arrive via TinyMCE/WETU HTML or legacy plain text. - Ensure pre-formatted
<ul>/<ol>values get thelsx-{key}-listclass added viaWP_HTML_Tag_Processor, so icon styling/injection applies consistently. - Simplify icon injection to handle
<li>with attributes,<ol>, and nested lists; update CSS selectors to support ordered lists as well.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
functions.php |
Normalises/splits includes/excludes meta into proper list output, adds list class to existing list markup, and injects the correct SVG icon per list type at render time. |
style.css |
Makes list styling apply to both <ul> and <ol> by removing the ul qualifier from includes/excludes list selectors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Problem
The Price Includes / Price Excludes lists on single tour pages collapse into a single bullet with a single icon, instead of one checked/crossed item per line.
Reported on 21 Day Cape Town to Victoria Falls. It reappears every time a tour is imported from WETU, or the field is edited and saved in the editor.
Live markup — one
<li>holding everything:Root cause
includedandnot_includedare CMB2 wysiwyg fields (config-tour.php) that the WETU importer also writes to verbatim, so a value arrives in one of four shapes. The theme filter split on newline characters only:Neither writer produces newlines — TinyMCE and WETU both emit
<br>/<p>markup. Sopreg_splitreturned a single element.<p>One<br>Two</p>— TinyMCE soft breaks, every WETU payload<li>, all items lumped<p>One</p><p>Two</p>— TinyMCE hard breaks<li>, words run together (DinnerNational Park FeeLunch)"One\r\nTwo"— legacy plain text<ul><li>One</li></ul>— real list markupOnly 4 of 76 stored values were in the shape the filter understood. The 4 that worked are legacy plain-text values nobody has opened in the editor since — which is why editing a tour "breaks" it.
Fix
All in the render path. No stored data is modified.
asnz_split_list_meta_value()— normalises<br>,<p>/</p>and\r\n/\rto one delimiter before splitting. Also collapses , so an emptied wysiwyg field renders nothing instead of a stray non-breaking space.asnz_add_list_meta_class()— values already stored as list markup keep their own HTML but get thelsx-{key}-listclass added viaWP_HTML_Tag_Processor, so those 6 rows finally get their icons.<li>with attributes, handles<ol>, and handles nested lists. The two near-identical branches collapse into one loop over a class→icon map.ulqualifier (ul.lsx-included-list→.lsx-included-list) so an editor-entered<ol>keeps the layout.Verification
Bootstrapped the site locally (SQLite, real plugins, real theme) and rendered the actual bound blocks from
templates/single-tour.htmlfor every tour:<li>emitted across all toursPlus a 12-case edge matrix through the same render path, all passing: XHTML
<br />, hard-break paragraphs, trailing empty paragraph, inline<strong>/<em>preserved,<li class="…">,<ol>, nested lists, single item, empty value, array value.Confirmed no stored data changed: still 76 non-empty rows, 62 with
<br>, 6 with<ul>.Scope
Only the theme's
templates/single-tour.htmlbinds these keys, so the change is confined to the Price Includes/Excludes section. The per-day itinerary includes uselsx_to_itinerary_includes(), which reads the serialiseditinerarysub-field and never passes throughlsx_to_custom_field_query— untouched.Known issues left alone
<p><ul>…</ul></p>— the binding target is acore/paragraph. Browsers auto-close the<p>so it renders correctly, but the markup is invalid. Fixing it means changing the template binding, and unwrapping the<p>would shift the section's vertical spacing.class-bindings.phpcallswp_strip_all_tags( $value, [ 'a', 'ul', 'li', … ] ). That second parameter is$remove_breaks(a boolean), not an allow-list — the truthy array strips all tags and breaks from itinerary includes/excludes.🤖 Generated with Claude Code