Make WordPress Core

Opened 4 months ago

Closed 3 weeks ago

#61574 closed task (blessed) (fixed)

[PHP] Remove PHP < 7.2 code

Reported by: ayeshrajans's profile ayeshrajans Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.7 Priority: normal
Severity: normal Version: 6.6
Component: General Keywords:
Focuses: Cc:

Description (last modified by SergeyBiryukov)

Follow-up to [57985] / #58719, remove a few other code that target PHP < 7.2.24.

Change History (25)

#2 @jrf
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: @ayeshrajans
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 :)

#4 @SergeyBiryukov
4 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#5 @SergeyBiryukov
4 months ago

  • Description modified (diff)

#6 @SergeyBiryukov
4 months ago

In 58678:

Code Modernization: Remove obsolete code targeting PHP < 7.2.24.

Follow-up to [44488], [45262], [53426], [57985].

Props ayeshrajans, jrf.
See #61574.

@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 @jrf
4 months ago

Replying to ayeshrajans:

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 :)

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.

#10 @ayeshrajans
4 months ago

Nice finds, those changes look great!

@jrf commented on PR #6975:


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-available is_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

#13 @jrf
4 months ago

Both follow-up GH PRs - GH 6975 and GH 6978 - have in the mean time been reviewed by a second person and approved.

Both are ready for commit.

#14 @SergeyBiryukov
4 months ago

In 58682:

Code Modernization: Remove obsolete comments about older PHP versions.

This commit:

  • Removes various comments referencing PHP versions which are no longer supported.
  • Removes various comments containing “hints” of things to do after a particular PHP version drop. These hints are incorrect/not actionable for various reasons, so have no value:
    • Even though a function could be turned into a closure, removing the function would be a backward compatibility 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.

Follow-up to [1243/tests], [6543], [11816], [29861], [29864], [34928], [35369], [36698], [38694], [50786], [58678].

Props jrf, ayeshrajans.
See #61574.

#15 @SergeyBiryukov
4 months ago

In 58683:

Code Modernization: Simplify a conditional in wp_is_ini_value_changeable().

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 workaround for the PHP 5.2 bug is no longer needed.

Follow-up to [38015], [38017], [44950], [45058], [57985], [58678], [58682].

Props jrf, ayeshrajans.
See #61574.

#16 @jrf
4 months ago

Thanks @SergeyBiryukov !

#17 follow-up: @dmsnell
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 @jrf
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.

#19 @SergeyBiryukov
4 months ago

In 58684:

Code Modernization: Replace substr( PHP_OS, 0, 3 ) calls with PHP_OS_FAMILY.

The PHP_OS_FAMILY constant indicates the operating system family PHP was built for, and is available as of PHP 7.2.0.

Reference: PHP Manual: Predefined Constants: PHP_OS_FAMILY.

Follow-up to [23255], [57753], [57985], [58678].

Props ayeshrajans, jrf.
See #61574.

@SergeyBiryukov commented on PR #6978:


4 months ago
#20

Thanks for the PR! Merged in r58684.

#21 @peterwilsoncc
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 @jorbin
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

This ticket was mentioned in Slack in #hosting by crixu. View the logs.


5 weeks ago

#25 @SergeyBiryukov
3 weeks ago

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.