Opened 5 years ago
Last modified 8 weeks ago
#36036 assigned defect (bug)
Multiple CDATA regressions in wp-includes
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | 3.4 |
Component: | General | Keywords: | good-first-bug has-patch |
Focuses: | Cc: |
Description
There is invalid XHTML at lines 2084-2097 of wp-includes/theme.php ( https://github.com/WordPress/WordPress/blob/3f3fe5a7ed6473e995f9b96ee4bcf36fe4c71a35/wp-includes/theme.php#L2084-L2097 )
This script block needs to be escaped with /*<![CDATA[*/ ..script content.. /*]]>*/
Otherwise it will cause rendering errors for people serving XHTML as application/xhtml+xml
This was not always like this, as 5 years ago when I made my strict XHTMl theme there were no errors. I recently re-enabled serving as application/xhtml+xml and to my surprise find this regression in the WP codebase.
Attachments (5)
Change History (30)
#2
@
5 years ago
Whoever made that change must not have realized that wp-includes is not only used in the admin interface.
#4
in reply to:
↑ 3
;
follow-up:
↓ 6
@
5 years ago
Replying to SergeyBiryukov:
Also related: #25863, comment:12:ticket:14226.
My issue is with WordPress as a platform, not the pre-installed themes. That comment is for that specific theme. My XHTML5 theme has been working absolutely fine for many years, and it seems like the intention of #18788 was only to remove CDATA in the admin interface. The patch unintentionally affected all theme content though.
#5
in reply to:
↑ 3
@
5 years ago
Replying to SergeyBiryukov:
Also related: #25863, comment:12:ticket:14226.
https://core.trac.wordpress.org/ticket/14226#comment:19
XHTML is fine for other themes or child themes. They just didn't want it in the default theme.
#6
in reply to:
↑ 4
@
5 years ago
- Keywords needs-patch good-first-bug added
- Milestone changed from Awaiting Review to Future Release
#7
@
5 years ago
FWIW, this is not a 4.4 regression, wp_customize_support_script()
was added in 3.4 without CDATA: [20893].
#9
@
5 years ago
- Version changed from 3.4 to 4.4.2
Can you merge this and #36037 into a general XHTML regressions bug? I'll submit a patch for the few applicable areas.
#11
@
5 years ago
I can't edit issues so it would be nice if someone with the permissions could change this issue's title to 'multiple CDATA regressions in wp-includes' to better organize this, instead of splitting it up into one ticket per file.
#12
@
5 years ago
I also changed 2 comments that said CDATA is not necessary for HTML5. CDATA sections are not necessary for non-XHTML, and the HTML5 doctype has no bearing on how the browser parses them.
#14
@
5 years ago
- Component changed from Customize to General
- Summary changed from Invalid XHTML at lines 2084-2097 of wp-includes/theme.php to Multiple CDATA regressions in wp-includes
#15
@
5 years ago
For my first patch I implemented some CDATA normalization that directly accessed the contents of a CDATA node, but I decided that this simpler regex on html()
is better. As far as I can tell this is the minimum amount of changes necessary.
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by chriscct7. View the logs.
5 years ago
This ticket was mentioned in Slack in #core by mike. View the logs.
5 years ago
#22
@
5 years ago
- Keywords has-patch added; needs-patch removed
- Milestone changed from 4.5 to Future Release
Punting due to lack of movement on ticket and timeframe for 4.5.
@SergeyBiryukov, feel free to re-milestone/commit if it looks good for 4.5.
#24
@
3 years ago
I will re-do this patch if someone on the dev team is willing to assist with review & merge.
Related: [31034] (for #18788).