WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 5 months ago

#36036 assigned defect (bug)

Multiple CDATA regressions in wp-includes

Reported by: sephr Owned by: SergeyBiryukov
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)

36036.patch (592 bytes) - added by subharanjan 2 years ago.
Added CDATA
patch.diff (7.6 KB) - added by sephr 2 years ago.
Fix wp-includes CDATA
patch-fix.diff (7.6 KB) - added by sephr 2 years ago.
(fix missing .firstChild on one line)
patch-reduced-js.diff (7.2 KB) - added by sephr 2 years ago.
Simplified wp-playlist.js changes
with-functions.php.diff (7.6 KB) - added by sephr 2 years ago.
Missed one in functions.php

Download all attachments as: .zip

Change History (29)

#2 @sephr
2 years ago

Whoever made that change must not have realized that wp-includes is not only used in the admin interface.

Last edited 2 years ago by sephr (previous) (diff)

#4 in reply to: ↑ 3 ; follow-up: @sephr
2 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.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#5 in reply to: ↑ 3 @sephr
2 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 @SergeyBiryukov
2 years ago

  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

Replying to sephr:

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.

You're right, I guess we should restore CDATA for scripts that are loaded on front end.

#7 @SergeyBiryukov
2 years ago

FWIW, this is not a 4.4 regression, wp_customize_support_script() was added in 3.4 without CDATA: [20893].

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#8 @swissspidy
2 years ago

  • Version changed from 4.4.2 to 3.4

#9 @sephr
2 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.

Last edited 2 years ago by sephr (previous) (diff)

#10 @sephr
2 years ago

  • Version changed from 4.4.2 to 3.4

@subharanjan
2 years ago

Added CDATA

@sephr
2 years ago

Fix wp-includes CDATA

#11 @sephr
2 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 @sephr
2 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.

@sephr
2 years ago

(fix missing .firstChild on one line)

#13 @SergeyBiryukov
2 years ago

  • Milestone changed from Future Release to 4.5

#14 @SergeyBiryukov
2 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

@sephr
2 years ago

Simplified wp-playlist.js changes

#15 @sephr
2 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.

Last edited 2 years ago by sephr (previous) (diff)

#16 @SergeyBiryukov
2 years ago

#36037 was marked as a duplicate.

@sephr
2 years ago

Missed one in functions.php

#17 @sephr
2 years ago

There was one I missed in wp-includes/functions.php

#18 @jorbin
2 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by mike. View the logs.


2 years ago

#22 @mikeschroder
2 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.

#23 @dd32
5 months ago

#42972 was marked as a duplicate.

#24 @sephr
5 months ago

I will re-do this patch if someone on the dev team is willing to assist with review & merge.

Note: See TracTickets for help on using tickets.