Make WordPress Core

Opened 18 months ago

Closed 18 months ago

Last modified 17 months ago

#22344 closed defect (bug) (fixed)

Fatal error with asp_tags on

Reported by: knutsp Owned by: koopersmith
Milestone: 3.5 Priority: normal
Severity: blocker Version: 3.5
Component: Media Keywords: has-patch
Focuses: Cc:


PHP Parse error:  syntax error, unexpected ')', expecting '&' or T_VARIABLE in /wp-includes/media.php on line 1453

I upgraded to 3.5-beta2-22352 with the beta plugin, and got the "white screen of death" on one system, but not on another. Debugged and found this error. Uploaded a .user.ini file with the line asp_tags = Off to the root folder, and everything was back to normal.

It seems media.php uses a script templating method with <% %> as placeholders. This is dangerous and will break sites.

Attachments (3)

22344.diff (8.3 KB) - added by rmccue 18 months ago.
Replace <% template tags with <#
22344-moustache.diff (8.4 KB) - added by rmccue 18 months ago.
Replace <% with {{ (Mustache-style)
22344-hybrid.diff (8.5 KB) - added by rmccue 18 months ago.
Hybrid version, uses {{ for escaping, {{{ for interpolating and <# for executing

Download all attachments as: .zip

Change History (16)

comment:1 SergeyBiryukov18 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:2 nacin18 months ago

  • Severity changed from major to blocker

I agree this is an issue. Seems like all of these will need to be <?php echo '<%'; ?> instead. It is times like these I really, really don't like PHP.

comment:3 rmccue18 months ago

Changing this to use <?php echo '<%'; ?> is really ugly and makes it much harder to work with. What if we change it to, say, <# instead (_.template supports that)? It seems much cleaner.

I'll submit a patch for that, since changing it to echo it every time just doesn't make sense.

rmccue18 months ago

Replace <% template tags with <#

comment:4 rmccue18 months ago

  • Keywords has-patch needs-testing added

Attached patch changes all instances of <% in the output code into <# (likewise for the closing tags) and sets Underscore up to use those instead. Looks like it's working perfectly to me, but I'd recommend double-checking in case I missed something.

comment:5 follow-up: scribu18 months ago

Why not use {{ }} and {{{ }}} like in Mustache?

comment:6 in reply to: ↑ 5 rmccue18 months ago

Replying to scribu:

Why not use {{ }} and {{{ }}} like in Mustache?

I'd be happy to switch it to use that if we'd prefer, but I stuck to the closest one I could for now (and # was a handy symbol on my keyboard; it might be the case that it's hard to access on other keyboards though).

comment:7 georgestephanis18 months ago

I stand in support of Mustache, partially because we've just begun Movember http://us.movember.com/

comment:8 rmccue18 months ago

  • Keywords needs-patch added; has-patch needs-testing removed

koop has given support for Moustache-style, so I'll switch the patch over this afternoon.

rmccue18 months ago

Replace <% with {{ (Mustache-style)

comment:9 rmccue18 months ago

  • Keywords has-patch added; needs-patch removed

New patch uses {{ instead of <%, giving a Mustache-style templating. {{ a }} is used for execution, {{= a }} is used for interpolating and {{- a }} is used for escaping.

I've tested this out, and it appears to work perfectly. Only breakage I can see being possible is if plugins are using wp.media.template() with <% a %>, but since it's not in an actual release version, that should be fine.

comment:10 scribu18 months ago

An actual Mustache-inspired syntax would be something like this:

{{{a}}} - interpolating
{{ a }} - escaping
<# a #> - execution

No curly braces for executing because {{ if (...) { } }} is confusing and Mustache (wisely IMO) doesn't support it anyway.

A counter argument would be that underscore doesn't support the majority of Mustache features, like blocks, comments etc.

I still think the {{escaped}} and {{{unescaped}}} syntax has merit, since it encourages escaping by default.

rmccue18 months ago

Hybrid version, uses {{ for escaping, {{{ for interpolating and <# for executing

comment:11 rmccue18 months ago

Attached hybrid version, seems to be the best option. Only thing is, it's not totally Mustache-compliant. I don't think that'll matter too much personally, as long as we document this somewhere.

See also, IRC discussion on which method to use.

comment:12 koopersmith18 months ago

  • Owner set to koopersmith
  • Resolution set to fixed
  • Status changed from new to closed

In 22415:

Use Mustache-insipired template tags.

Underscore's default ERB-style templates are incompatible with PHP when asp_tags is enabled. As a result, we've settled on an alternative syntax that should be familiar to devs: Mustache-inspired for interpolating and escaping content, and ERB-inspired for execution.

{{{a}}} - interpolating
{{ a }} - escaping
<# a #> - execution

props rmccue. fixes #22344, see #21390.

comment:13 koopersmith17 months ago

In 22539:

Media templates: Alter escaping regular expression to prevent it from aggresively consuming input meant for interpolation. see #22344, #21390.

Note: See TracTickets for help on using tickets.