Theme Directory: handle the theme description consistently across both writers - #874
Theme Directory: handle the theme description consistently across both writers#874obenland wants to merge 1 commit into
Conversation
…h writers A theme's description is the one-line `Description:` header of the author's `style.css`. Two writers put it into `post_content` and they disagreed about it: `create_or_update_theme_post()` ran it through `strip_shortcodes()`, and `wporg_themes_approve_version()`, which re-reads `style.css` from SVN when a version goes live, assigned it as-is. The second is the only one that runs for an update to an existing theme, so the first never applied there. `strip_shortcodes()` is the wrong tool either way. It only removes tags that are registered in the process that calls it, and it deliberately unwraps an escaped `[[tag]]` into a live `[tag]`, so a header carrying a tag and its escaped twin comes out of it with a shortcode still in place. Editing the syntax out is no better in general: removing one shortcode can splice the remaining text into another, as `[gal[caption]lery ids="1"]` does. Refuse the header at upload instead, where the author can be told about it, and encode the delimiters on the SVN path, where there is nobody to tell. Detection does not have either problem, and the encoded form still reads as it was written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughTheme uploads now reject descriptions containing shortcodes. Approved SVN descriptions escape square brackets before storage. Tests cover upload validation, approval behavior, cleanup, and ordinary descriptions. ChangesTheme description safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to Theme descriptions now reject shortcode syntax on upload and store SVN-imported shortcode delimiters inertly, while ordinary bracketed descriptions remain supported. The change is covered across both write paths and is ready to merge. Sequence Diagram(s)sequenceDiagram
participant SVN
participant ThemeDirectory
participant ThemePost
SVN->>ThemeDirectory: provide theme description
ThemeDirectory->>ThemeDirectory: validate or encode square brackets
ThemeDirectory->>ThemePost: update theme post content
ThemePost-->>ThemeDirectory: store description without shortcode execution
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new upload-time detection relies on get_shortcode_regex() (registered shortcodes only), which can miss shortcode syntax for unregistered tags and undermine the long-lived safety goal described in the PR.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes Theme Directory handling of the one-line Description: header consistent across the upload path and the SVN “version goes live” path, preventing shortcode execution by rejecting shortcode syntax on upload and encoding delimiters on the SVN writer.
Changes:
- Add upload-time validation to refuse theme descriptions that contain shortcode syntax.
- Stop running
strip_shortcodes()when writingDescription:intopost_contentduring upload. - Encode
[/]in the SVN approval path and add PHPUnit coverage for both writers.
File summaries
| File | Description |
|---|---|
| wordpress.org/public_html/wp-content/plugins/theme-directory/theme-directory.php | Encodes square brackets in the SVN approval writer before storing post_content. |
| wordpress.org/public_html/wp-content/plugins/theme-directory/class-wporg-themes-upload.php | Adds upload-time shortcode detection error and stores the description as-is (no strip_shortcodes()). |
| wordpress.org/public_html/wp-content/plugins/theme-directory/tests/Theme_Description_Shortcodes_Test.php | Adds tests covering upload refusal and SVN-path delimiter encoding behavior. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A theme description is the one-line
Description:header of the author’sstyle.css. Two writers put it intopost_contentand they disagreed about it:create_or_update_theme_post()ran it throughstrip_shortcodes(),while
wporg_themes_approve_version()— which re-readsstyle.cssfrom SVN when a version goes live — assigned itas-is. The second is the only one that runs for an update to an existing theme, so the first never applied there.
strip_shortcodes()is the wrong tool for a header either way:and rendered for a long time afterwards.
[[tag]]into a live[tag], so a header carrying a tag and its escapedtwin still has a shortcode in it afterwards.
Editing the syntax out is no better in general — removing one shortcode can splice the remaining text into another,
which is what
[gal[caption]lery ids="1"]does.So: refuse the header at upload, where the author can be told, and encode the delimiters on the SVN path, where
there is nobody to tell. Detection has neither problem, and the encoded form still reads as written (
[rendersas
[).Complements WordPress/wporg-theme-directory#179, which stops the theme page interpreting the description at all;
that side also covers descriptions stored before either change.
Also narrows the scope of #871 — the description handling there is superseded, the
Name/Author/URI headers are not.Testing steps
style.csshasDescription: Fixture [caption width="1" caption="x"]y[/caption] theme.The upload is refused with a message pointing at the
Description:line. Try[[caption …]y[/caption]]and[gal[caption]lery ids="1"]too; both are refused.A theme for [developers] and designers— uploads fine.post_contentreads
[caption …], renders as[caption …], and runs nothing.npm run themes:testfromenvironments/→ 145 tests, 215 assertions. The pre-existingSearch_Published_Filter_Testrisky-test notice is unrelated.🤖 Generated with Claude Code
Summary by CodeRabbit