WordPress.org

Make WordPress Core

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#13915 closed enhancement (fixed)

WP_Http_Fopen is PHP4 only

Reported by: jacobsantos Owned by:
Milestone: 3.2 Priority: normal
Severity: trivial Version: 3.0
Component: HTTP API Keywords: 3.2-early has-patch
Focuses: Cc:

Description

Patch checks for PHP5 and then prevents Fopen transport from being used on PHP5.

Attachments (3)

13915.patch (536 bytes) - added by jacobsantos 11 years ago.
Prevent WP_Http_Fopen from being used on PHP5+
13915.2.diff (7.5 KB) - added by sivel 11 years ago.
We are dropping PHP4 so let's just drop WP_Http_Fopen
13915.2.patch (8.1 KB) - added by hakre 10 years ago.
refreshed against trunk, incl. check for allow_url_fopen

Download all attachments as: .zip

Change History (26)

@jacobsantos
11 years ago

Prevent WP_Http_Fopen from being used on PHP5+

#1 @jacobsantos
11 years ago

  • Type changed from defect (bug) to enhancement

#2 @jacobsantos
11 years ago

Not really a bug, since everything works as it should, but this was really only for PHP4 for fopen support.

#3 @jacobsantos
11 years ago

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

#4 @gazouteast
11 years ago

  • Cc gazouteast added
  • Keywords (are you sure?) added

Jacob - not so sure about that - earlier this year, I had major tickets with WPMU regarding non-displaying avatars and images that were traced to fopen not being available under PHP 5.x (can't find the original discussions since WPMU and BuddyPress changed their forum engines and structures).

A number of people including Andrea_r got roped in on that one, I think johnjamesjacoby did too, and all conceded the catch when I finally tracked it down.

Can you absolutely assure that all requirements for fopen have been removed from WP 3.0 otherwise you might be being a bit premature here, and have to leave backwards compatibility in place - this also rubs against the many users reporting images not displaying after upgrade to 3.0 from earlier versions.

Gaz

#5 @jacobsantos
11 years ago

  • Keywords (are you sure?) removed

The WP_Http_Fopen class was written to enable fopen support for PHP4. PHP5 added support for Streams and the WP_Http_Streams class uses the streams method when using fopen on PHP5+. The problem is that this class is in rare cases being used on PHP5 setups where WP_Http_Streams is failing (because of SSL or another reason) and the WP_Http_Fopen is passing. The goal was to allow fsockopen to be used if at all possible on PHP5 when fopen is not available and hope that fsockopen is still available.

I have to confess I did not follow the discussion with BuddyPress or WPMU, as neither of those really have anything to do with this ticket. I don't know the specifics of the issues that were discussed but with my limited information, I don't believe they are relevant to this ticket.

If you have more information, on that, I would like to see.

#6 follow-up: @johnjamesjacoby
11 years ago

@jacobsantos, if you can explain how to duplicate the environment I'll gladly test it against BuddyPress and help if a fix is necessary.

If I recall, this issue may have been almost (if not over) a year ago, so I'm a little fuzzy on the topic myself.

At first glance, it doesn't seem like the patch would hurt. Although the WP_Http_Streams::test method puts that check after the function_exists('fopen') check. That doesn't mean it wouldn't wreck our days if we tried it. :)

#7 in reply to: ↑ 6 @jacobsantos
11 years ago

Replying to johnjamesjacoby:

@jacobsantos, if you can explain how to duplicate the environment I'll gladly test it against BuddyPress and help if a fix is necessary.

I still don't understand. This is a PHP4 environment, so you would simply install PHP4. The Streams will not be available. If you Install PHP5, then this class should never be used, the next fail over should be fsockopen.

If I recall, this issue may have been almost (if not over) a year ago, so I'm a little fuzzy on the topic myself.

That is unfortunate as I'm trying to figure out what the other guy means. If fopen doesn't exist, then I'm unsure how the HTTP API has anything to do with avatars and what not with BuddyPress and WPMU (which is part of WP now). There were problems with specific versions of fopen when cURL was installed and used, but that generally has no good fix.

I'm to assume that either it has to do with the filesystem which is outside the scope of this ticket and also HTTP API. Or the problem was outside the scope of this class and API and is a PHP issue. Neither of which has anything to do with this ticket.

Until more information is given, I can't really give a solution and therefore have to ignore it. As of right now, until more information is given, I believe it has nothing to do with this ticket or the patch, I would frankly rather it not be discussed further.

@sivel
11 years ago

We are dropping PHP4 so let's just drop WP_Http_Fopen

#8 @sivel
11 years ago

Since we are dropping PHP4 support in 3.2, no reason to keep this around. It was only kept because PHP4 didn't support streams with fopen. Now that we don't have this restriction no need to keep this code around.

#9 @sivel
10 years ago

  • Keywords 3.2-early added
  • Owner jacobsantos deleted
  • Status changed from accepted to assigned

#10 @dd32
10 years ago

if this is removed based on PHP5+ availability, Note that WP_Http_Fopen::test() is used in image-edit.php to test to see if external fopen() calls are allowed.

#11 @dd32
10 years ago

  • Keywords needs-refresh added; has-patch tested removed

refresh needed based on my previous comment

@hakre
10 years ago

refreshed against trunk, incl. check for allow_url_fopen

#12 @hakre
10 years ago

I reduced the check for allow_url_fopen for a single ini_get call.

The previous one was checking for function existance of fopen(), ini_get() and then to check ini_get( 'allow_url_fopen' ). Any Idea dd32? I

#14 @hakre
10 years ago

  • Keywords has-patch added; needs-refresh removed

#15 follow-up: @dd32
10 years ago

The previous one was checking for function existance of fopen(), ini_get() and then to check ini_get( 'allow_url_fopen' ). Any Idea dd32? I

Perhaps you're not the best person to patch it if you're unaware of why it's done as it is?

Off the top of my head, disabled_functions setting will cause fopen to not exist, similarly, some hosts disable ini_get, and finally, need to check that allow_url_fopen is allowed.

#16 in reply to: ↑ 15 @hakre
10 years ago

Replying to dd32:

The previous one was checking for function existance of fopen(), ini_get() and then to check ini_get( 'allow_url_fopen' ). Any Idea dd32? I

Perhaps you're not the best person to patch it if you're unaware of why it's done as it is?

Just was asking for a second opinion, I know why that code was originally in, I just don't think it's necessary to check for those with such a detail at that place.

#17 @dd32
10 years ago

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

(In [17534]) Remove PHP4 based WP_Http_Fopen class. Props sivel for initial patch. See #16917 Fixes #13915

#18 @dd32
10 years ago

  • Milestone changed from Future Release to 3.2

the image-edit.php change was originally made in [11985].

#19 @dd32
10 years ago

(In [17535]) Prune PHP5 compat checks in WP_Http, WP_Http_Curl & WP_Http_Streams. Only removes pre PHP 5.2.4 conditional code. See #13915

#20 @hakre
10 years ago

Related: #16920

#21 @hakre
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Looks like in [17535] you removed the SSL proxy, compare: http://core.trac.wordpress.org/attachment/ticket/16920/16920.2.patch , there is a second patch available as well reg. class-http so you can compare.

#22 @hakre
10 years ago

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

But might not be, can't properly check right now.

#23 @dd32
10 years ago

Looks like in [17535] you removed the SSL proxy

Thats correct. The block was disabling HTTPS requests throgh proxies on PHP older than 5.1

Note: See TracTickets for help on using tickets.