Opened 5 years ago
Closed 5 years ago
#48091 closed task (blessed) (fixed)
Remove conditional use of PHP stream_get_contents()
Reported by: |
|
Owned by: |
|
---|---|---|---|
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:
- 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.
- 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)
Change History (8)
#1
@
5 years ago
- Milestone changed from Awaiting Review to 5.4
- Status changed from assigned to reviewing
#2
in reply to:
↑ description
@
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
@
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
@
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 ?
[src] Remove work-arounds for stream_get_contents()