Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#48091 closed task (blessed) (fixed)

Remove conditional use of PHP stream_get_contents()

Reported by: jrf's profile jrf Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.3
Component: General Keywords: has-patch early
Focuses: performance, coding-standards Cc:

Description

While working on #48075 / looking for code which can be removed now support for PHP < 5.6.20 has been dropped, I came across three instances where checks were done for the existance of the stream_get_contents() function.

<?php
if ( function_exists( 'stream_get_contents' ) ) {
        // Code.
}

Now, there are only two reasons I can think for this code to exist:

  1. Support for PHP < 5.

The `stream_get_contents()` function was introduced in PHP 5.0.0. It might be that this code is from before that time.

  1. Hosts disabling the function using the `disabled_functions` ini directive.

I honestly don't know if that's a thing, but that's the only other reason I could think of for this code to exist.


Based on the comment found in the `class-wp-filesystem-ssh2.php` documentation, I strongly believe the reason is 1) and that the removal of these conditions is long overdue, if for no other reason than that the function is used unconditionally in the `mo.php` file.

So based on that I am attaching a patch to remove the condition.

Attachments (1)

48091-src-Remove-work-arounds-for-stream_get_contents.patch (2.3 KB) - added by jrf 5 years ago.
[src] Remove work-arounds for stream_get_contents()

Download all attachments as: .zip

Change History (8)

@jrf
5 years ago

[src] Remove work-arounds for stream_get_contents()

#1 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 5.4
  • Status changed from assigned to reviewing

#2 in reply to: ↑ description @SergeyBiryukov
5 years ago

Replying to jrf:

While working on #48075 / looking for code which can be removed now support for PHP < 5.6.20 has been dropped, I came across three instances where checks were done for the existance of the stream_get_contents() function.

Introduced in [11632] / #10093, as a follow-up to [11063] / #8210.

I think it should indeed be safe to remove these checks now that we're on PHP 5.6.20+, same as JSON workarounds and polyfills were removed in [46205-46206,46208] / #47699.

However, just in case, would it make sense to add an upgrade check for stream_get_contents(), like [46455]?

I strongly believe the reason is 1) and that the removal of these conditions is long overdue, if for no other reason than that the function is used unconditionally in the `mo.php` file.

Worth noting that mo.php only uses it in he MO::export() method, which is unused in core.

#3 @audrasjb
5 years ago

  • Milestone changed from 5.4 to Future Release

Hi,

With 5.4 Beta 3 approaching and the Beta period reserved for bugs introduced during the cycle, this is being moved to Future Release. If any maintainer or committer feels this should be included or wishes to assume ownership during a specific cycle, feel free to update the milestone accordingly.

#4 @jrf
5 years ago

  • Keywords early added
  • Milestone changed from Future Release to 5.5

I understand moving it out of the current release as it's in beta stage already. I do think this should be addressed sooner rather than later, so let's make it 5.5 early.

However, just in case, would it make sense to add an upgrade check for stream_get_contents(), like [46455]?

I honestly think that would be overkill. JSON is an extension and PHP can be compiled without it. stream_get_contents() is part of PHP core.

Either way, if this gets committed early in WP 5.5, it would allow for issues to be reported well before the release and if someone _does_ report an issue, we can always revisit and add the check in the upgrade routine before the actual release.

@SergeyBiryukov How does that sound to you ?

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


5 years ago

#6 @davidbaumwald
5 years ago

@SergeyBiryukov Is this still on your list to review for early 5.5?

#7 @SergeyBiryukov
5 years ago

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

In 47584:

Code Modernization: Remove conditional use of stream_get_contents() PHP function.

The function was introduced in PHP 5.0.0. Now that the minimum version of PHP required by WordPress is 5.6.20, these conditions are no longer needed.

Props jrf.
Fixes #48091.

Note: See TracTickets for help on using tickets.