Opened 2 years ago

Closed 17 months ago

Last modified 17 months ago

#16325 closed defect (bug) (fixed)

kses uses deprecated (PHP 5.3.0) split

Reported by: duck_ Owned by: duck_
Priority: normal Milestone: 3.4
Component: General Version: 3.0
Severity: normal Keywords: has-patch
Cc: knut@…

Description

wp-includes/kses.php line 1407, part of safecss_filter_attr.

split is deprecated in PHP as of 5.3.0, we should probably replace with explode here.

Attachments (3)

16325.diff (437 bytes) - added by duck_ 2 years ago.
garyc40.16325.diff (5.7 KB) - added by garyc40 2 years ago.
fix deprecated split() in other files
rss-#16325.diff (465 bytes) - added by j_schumann 18 months ago.
replace split() in wp-includes/rss.php, all other occurences seem to be coverd in the previous attachments

Download all attachments as: .zip

Change History (15)

duck_2 years ago

Actually used in approximately 20 places overall (probably should have checked this first).

garyc402 years ago

fix deprecated split() in other files

comment:2 follow-up: ↓ 4   garyc402 years ago

I didn't replace split() in 3 places:

  • atomlib
  • rss (magpieRSS)
  • phpmailer

These are external packages. They probably have upstream changes already.

I also checked each instances whether to use explode() or preg_split(). Seems like all of them can be explode(), but we should be careful if there's any additional replacement we need to make.

magpieRSS is dead upstream and deprecated here, so not touching it is fine.

atomlib.php looks dead upstream as well -- DOA in fact. Only change other than whitespace and docs is actually downstream (r4892). We should probably take ownership of it again formally, considering it was originally a contribution to WordPress, rather than an external package we brought in. I'm also noting that it is licensed under Apache upstream, but was originally contributed to WordPress as GPL. (Not sure why it was even treated as an external library due to the way it was contributed.)

Reference: http://code.google.com/p/phpatomlib/source/browse/trunk/lib/atomlib.php

phpmailer, I believe there exists a PHP5 version upstream, so we should update to that for 3.2.

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

comment:4 in reply to: ↑ 2   duck_2 years ago

Replying to garyc40:

I also checked each instances whether to use explode() or preg_split(). Seems like all of them can be explode(), but we should be careful if there's any additional replacement we need to make.

Yeah that's why I didn't rush into it last night. I imagine explode will be fine in these cases, but do need to make sure.

I wouldn't worry about atomlib we will like stop using it soon - https://core.trac.wordpress.org/attachment/ticket/7652/7652-wpapp-done.diff

  • Version set to 3.0

As a quick note, these are different:

list($domain, $video_id) = split(".com/", $url);
list($domain, $video_id) = explode(".com/", $url);

Technically in split (being a POSIX Regex) the . is a single-character wildcard. I actually think that the way explode works is what was intended for this particular bit of code, but I thought I'd point it out.

comment:8 follow-up: ↓ 12   j_schumann18 months ago

+1

I would highly appreciate seeing this fixed in 3.3, this bug is open for 10 months and unchanged, with patches available that do not change functionality or break b/c so it should not be of any problems.

replace split() in wp-includes/rss.php, all other occurences seem to be coverd in the previous attachments

  • Cc knut@… added
  • Milestone changed from Awaiting Review to 3.4
  • Owner set to duck_
  • Resolution set to fixed
  • Status changed from new to closed

In [19726]:

split was deprecated in PHP 5.3.0, so use explode instead. Props garyc40. Fixes #16325.

comment:12 in reply to: ↑ 8   duck_17 months ago

Replying to j_schumann:

replace split() in wp-includes/rss.php, all other occurences seem to be coverd in the previous attachments

See nacin's comment above for why it wasn't removed from wp-includes/rss.php.

Note: See TracTickets for help on using tickets.