Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#16970 closed defect (bug) (fixed)

Remove PHP4 functionality from timezone selector code

Reported by: viper007bond's profile Viper007Bond Owned by:
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:

Description

All of the functions needed for the fancy timezone selector code are bundled with all versions of PHP since 5.2.0. This means we can deprecate wp_timezone_supported() and remove all references to it in core as well as strip all of the PHP4 fallback code.

Attachments (3)

16970.patch (1.6 KB) - added by hakre 13 years ago.
Deprecate wp_timezone_supported()
16970.diff (1.2 KB) - added by kawauso 13 years ago.
No need for function_exists checks anymore
16970.2.patch (4.5 KB) - added by hakre 13 years ago.

Download all attachments as: .zip

Change History (19)

#1 @Viper007Bond
13 years ago

  • Owner set to Viper007Bond
  • Status changed from new to accepted

It's been a while since I contributed anything to core. I might as well take this on.

#2 @aaroncampbell
13 years ago

We were talking about this on IRC, so I thought I'd throw in my two cents:

The manual says There is no installation needed to use these functions; they are part of the PHP core. And the most recently added was in 5.2.0, so there's just no need for wp_timezone_supported() anymore. I think it should just be deprecated and changed to return true (for plugins using it). Obviously we should clean up core accordingly.

For those that care, these are the functions that wp_timezone_supported() checks for:

@hakre
13 years ago

Deprecate wp_timezone_supported()

#3 @hakre
13 years ago

The function contains a filter, so core - even the function deprecates - can not assume that it always returns true.

So the other code needs to stay in core. Until the function can be completely removed. (Which I suggest as the chance had been passed to deprecate this in 3.1 already in the know of the 3.2 php requirements commming.)

#4 @hakre
13 years ago

  • Keywords has-patch added; needs-patch removed

@kawauso
13 years ago

No need for function_exists checks anymore

#5 follow-up: @scribu
13 years ago

What legitimate use-case would there be for adding a filter that returns false, in a PHP 5.2 context?

#6 in reply to: ↑ 5 @hakre
13 years ago

Replying to scribu:

What legitimate use-case would there be for adding a filter that returns false, in a PHP 5.2 context?

No idea, I only hear "Backwards Compatibility". But don't ask me because I'm for removing this function entirely personally and so I can not see a reason at all.

@kawauso - functions could be disabled per ini (no idea which we need to check actually, in HTTP we check fopen but that's I/O related, so more or less informative)

Last edited 13 years ago by hakre (previous) (diff)

#7 @nacin
13 years ago

My opinion: Deprecate, kill the filter, return true, and kill all code that handles when wp_timezone_supported() is false.

#8 @scribu
13 years ago

+1 for what nacin said.

@hakre
13 years ago

#9 @hakre
13 years ago

Patch: Deprecate, kill the filter, return true, kill all code that handles when wp_timezone_supported() is false and replace wp_timezone_supported() with TRUE, remove dead code then.

Related: #16920

Last edited 13 years ago by hakre (previous) (diff)

#10 follow-up: @aaroncampbell
13 years ago

The patch looks fine. Does exactly what Alex and I had first discussed.

My issue is that Alex said he was going to do the patch since he hadn't contributed anything to core for a while. What was the purpose of uploading a patch less than an hour after he said he would? Did he ask you to (or did you ask him)? If not, couldn't you have focused your time on another ticket (just offering input on this one like everyone else did) instead of stepping on someone's toes like that?

#11 in reply to: ↑ 10 @nacin
13 years ago

Replying to aaroncampbell:

The patch looks fine. Does exactly what Alex and I had first discussed.

My issue is that Alex said he was going to do the patch since he hadn't contributed anything to core for a while. What was the purpose of uploading a patch less than an hour after he said he would? Did he ask you to (or did you ask him)? If not, couldn't you have focused your time on another ticket (just offering input on this one like everyone else did) instead of stepping on someone's toes like that?

I never want to discourage someone from contributing. But among the more active contributors, there's a bit of informal code about things like this. Those who have commented to this ticket, we all have the ability (in terms of skill) to contribute to basically any ticket. When someone says they want to tackle something, I'd think it'd be common courtesy would be to at least circle back, oh, more than 25 minutes later. Chances are Alex was working on the patch at the time.

Given enough eyeballs, all bugs are shallow, but that doesn't mean it's a race or that duplicative efforts shouldn't be avoided when it isn't necessary. As Aaron says, certainly time could have been spent on another ticket.

Again, I never want to discourage contributions. If it was a newer contributor who posted a patch, I would not want this conversation to take place. But among us, let's have some mutual respect please?

#12 @scribu
13 years ago

Since we're meta-ing here, #16955 could definetly use some more eyeballs.

Version 0, edited 13 years ago by scribu (next)

#13 @Viper007Bond
13 years ago

  • Owner Viper007Bond deleted
  • Status changed from accepted to assigned

Too slow I guess. :)

#14 @Viper007Bond
13 years ago

Oh, and thankfully no I hadn't started working on the patch yet -- I was going to do it today. It seemed like a fun little project I could have spent a few minutes on, but oh well. :)

#15 @andrewryno
13 years ago

  • Cc andrewryno@… added

#16 @ryan
13 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [17637]) Remove PHP4 timezone support. Props hakre. fixes #16970

Note: See TracTickets for help on using tickets.