Opened 4 months ago
Closed 3 weeks ago
#61574 closed task (blessed) (fixed)
[PHP] Remove PHP < 7.2 code
Reported by: | ayeshrajans | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.7 | Priority: | normal |
Severity: | normal | Version: | 6.6 |
Component: | General | Keywords: | |
Focuses: | Cc: |
Description (last modified by )
Change History (25)
This ticket was mentioned in PR #6969 on WordPress/wordpress-develop by @ayeshrajans.
4 months ago
#1
#2
@
4 months ago
- Focuses php-compatibility removed
- Keywords commit added
- Milestone changed from Awaiting Review to 6.7
There are some more things which can be removed, but the changes as proposed in the PR are good to go and can be committed.
#3
follow-up:
↓ 9
@
4 months ago
Yay, thank you!
I went through the compat files, PHP_VERSION
constants, and function_exists
calls to look for snippets to remove. But I think you are absolutely right, there has to be more things that we could remove.
Happily offering an extra pair of hands if you have any suggestions where to look for snippets to remove :)
@SergeyBiryukov commented on PR #6969:
4 months ago
#7
Thanks for the PR! Merged in r58678.
This ticket was mentioned in PR #6975 on WordPress/wordpress-develop by @jrf.
4 months ago
#8
- Keywords has-unit-tests added
### Remove redundant PHP version check
What with the minimum supported PHP version currently being PHP 7.2.x, this check has become redundant.
### Docs: remove redundant comments [1]
This commit removes various comments containing "hints" of things to do after a particular PHP version drop. These hints are incorrect/not actionable for various reasons (see below), so have no value.
Some typical reasons:
- Even though a function could be turned into a closure, removing the function would be a BC-break which is not acceptable, so this suggestion is not actionable.
- Short ternaries are forbidden by the coding standard exactly to prevent the faulty code suggested in the comment from getting into the codebase.
### Docs: remove redundant comments [2]
This commit removes various comments referencing PHP versions which are no longer supported. These PHP version references have no value anymore no support for those PHP versions has been dropped.
### wp_is_ini_value_changeable(): minor simplification
This commit reverts the code to the code from before the bug fix related to PHP 5.2.6-5.2.17.
As support for PHP 5.2 has been dropped, the work-around for the PHP 5.2 bug is no longer needed.
Trac ticket: https://core.trac.wordpress.org/ticket/61574
#9
in reply to:
↑ 3
@
4 months ago
Replying to ayeshrajans:
Yay, thank you!
I went through the compat files,PHP_VERSION
constants, andfunction_exists
calls to look for snippets to remove. But I think you are absolutely right, there has to be more things that we could remove.
Happily offering an extra pair of hands if you have any suggestions where to look for snippets to remove :)
I've rebased what I prepared earlier this week after the commit made by @SergeyBiryukov (thanks!) and opened PR 6975. Definitely not claiming completeness, but this should get us another step closer.
@ayeshrajans Would you like to review that PR ?
I came across these when searching for similar things, including ini_get()
, @requires
(in the tests), ->markTestSkipped()
(tests), PHP_VERSION_ID
, phpversion()
, function_exists()
, class_exists()
, extension_loaded()
and some more things (searches made in relation to some changes which will be proposed for the PHPUnit workflow).
I've fixed things of which I felt sufficiently confident, but only skimmed through the results of those searches, so there may well be more which can be removed based on those searches alone.
4 months ago
#11
This is perhaps outside of the scope of this ticket/PR, but I was wondering what's your opinion on simplifying methods such as
\WpOrg\Requests\Utility\InputValidator::is_iterable
to use the now-availableis_iterable
.
Well, for one, Requests is an external library, so that's out of scope. Additionally, Requests still has a PHP 5.6 minimum, so again, no go.
I also noticed a few
substr(PHP_OS, 0, 3)
checks that we can simplify with (PHP 7.2+)PHP_OS_FAMILY
. Perhaps in a separate ticket? because these are not strictly removals.
That sounds like a perfectly sensible change to make and would really simplify some code snippets. In my opinion, it's fine to do that under the same ticket umbrella as it's along the same lines, i.e. "removing work-arounds which were needed to support PHP < 7.2".
This ticket was mentioned in PR #6978 on WordPress/wordpress-develop by @ayeshrajans.
4 months ago
#12
Trac ticket: https://core.trac.wordpress.org/ticket/61574
Follow-up to #6969, and responding to a comment in @jrfnl's #6975
#17
follow-up:
↓ 18
@
4 months ago
Hi @SergeyBiryukov - it looks like a few of the changes in [58682] remove comments that were left in the past to provide insight into how to adapt the code once PHP minimum versions passed a certain milestone.
Now that those milestones have passed, would it make sense to apply the advice in the comments? I'm worried that in removing the comments without changing the code we're removing helpful context for someone who wants to improve the code quality and consistency in the future. Was it intentional to remove the guidance without changing the code?
#18
in reply to:
↑ 17
@
4 months ago
Replying to dmsnell:
Hi @SergeyBiryukov - it looks like a few of the changes in [58682] remove comments that were left in the past to provide insight into how to adapt the code once PHP minimum versions passed a certain milestone.
@dmsnell Please read the commit message - the two "hints" about improving the code are not actionable, so those comments have no value.
Other than that, the other comments only provided historical context, which is no longer relevant (and can still be seen in the commit history).
The actionable bits - like for wp_parse_url()
- were already actioned years ago. At the time, the docblock was not updated as the "new" minimum PHP version at that time was 5.6, but by now those comments really aren't relevant anymore.
@SergeyBiryukov commented on PR #6978:
4 months ago
#20
Thanks for the PR! Merged in r58684.
#21
@
7 weeks ago
- Keywords has-patch commit has-unit-tests removed
Ticket/report maintenance as the linked PRs have all been committed. I've left it on the milestone to allow for any further changes to be made during the release cycle.
This ticket was mentioned in Slack in #core by jorbin. View the logs.
6 weeks ago
#23
@
6 weeks ago
- Type changed from enhancement to task (blessed)
Going to allow until beta 2 for any follow-ups necessary so converting this to a blessed task with the blessing of @davidbaumwald
Trac ticket: https://core.trac.wordpress.org/ticket/61574