WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 2 months ago

#42789 closed task (blessed) (fixed)

Bump recommended PHP version from 7.0 -> 7.2

Reported by: rachelbaker Owned by: rachelbaker
Milestone: 4.9.5 Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch fixed-major
Focuses: Cc:

Description

The recommended version of PHP in the readme.html file is 7.0. As of December 3, 2017, PHP 7.0 is no longer actively supported. The recommended version should be bumped to 7.1

Attachments (1)

42789.diff (698 bytes) - added by rachelbaker 6 months ago.

Download all attachments as: .zip

Change History (22)

#1 @rachelbaker
6 months ago

Failing unit test:

1) Tests_External_HTTP_Basic::test_readme readme.html's Recommended PHP version is too old. Remember to update the WordPress.org Requirements page, too. Failed asserting that an array contains '7'. /home/travis/build/WordPress/wordpress-develop/tests/phpunit/tests/external-http/basic.php:23

@rachelbaker
6 months ago

#2 @rachelbaker
6 months ago

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

In 42789.diff bumping the recommended PHP version to 7.1, which is under active support until December 1, 2018.

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


6 months ago

#4 follow-up: @johnbillion
6 months ago

  • Type changed from defect (bug) to task (blessed)

PHP 7.2 was released last week. WordPress 4.9 is fully tested and compatible. Should that be the recommended version?

#5 @rachelbaker
6 months ago

  • Keywords has-patch needs-refresh added

#6 in reply to: ↑ 4 @rachelbaker
6 months ago

Replying to johnbillion:

PHP 7.2 was released last week. WordPress 4.9 is fully tested and compatible. Should that be the recommended version?

@johnbillion Aah. I didn’t realize we were able to recommend 7.2 yet. Honestly, I don’t even know if there is a process around what versions we recommend. I will leave this open for others to weigh in as well.

#7 @johnbillion
6 months ago

I think we can just yolo it to 7.2. There's no reason _not_ to recommend 7.2.

#8 @jorbin
6 months ago

I've been running on 7.2 and it is a smooth experience. Let's go for 7.2.

@pento - Can you commit that and also update https://wordpress.org/about/requirements/ ?

#9 @jorbin
6 months ago

@otto42 made the changes on .org to say 7.2

#10 @johnbillion
6 months ago

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

#11 @johnbillion
6 months ago

  • Milestone changed from Awaiting Review to 5.0

#12 @netweb
6 months ago

  • Summary changed from Bump recommended PHP version from 7.0 -> 7.1 to Bump recommended PHP version from 7.0 -> 7.2

#13 @ayeshrajans
6 months ago

Awesome to hear about bumping version to 7.2! I work quite a lot on legacy code, and from what I have seen, PHP 7.2 can raise some PHP notices that are often not seen from tests or and pop up in edge cases. I have worked on a few tickets for 7.2 and tests are passing(yay!), but personally, I think there is still a room for improvement.

One very frequent problem with 7.2 is that count() calls on uncountable variables raise a notice. This kind of bugs are very difficult to grep from the repo and often missed from tests. count(), each() and such deprecation notices are easy to fix but annoying nonetheless. If we can actively hunt those bugs quickly as they are reported, that can help a lot.

#14 @lisota
6 months ago

Here is an example count() bug, referenced by @ayeshrajans

#42860

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

#15 follow-up: @SergeyBiryukov
3 months ago

  • Keywords fixed-major added; needs-refresh removed
  • Milestone changed from 5.0 to 4.9.5
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.5 consideration.

@mikeschroder noted that the test still fails on the 4.9 branch. Per comment:4, it should be safe to bump the recommended version there as well.

There were some additional fixes: #35560, #43201, #43216; some more are coming: #42814, #43312.

#16 in reply to: ↑ 15 ; follow-up: @dd32
3 months ago

Replying to SergeyBiryukov:

Reopening for 4.9.5 consideration.

@mikeschroder noted that the test still fails on the 4.9 branch. Per comment:4, it should be safe to bump the recommended version there as well.

While it should be safe, the test is only supposed to run on master/trunk: (Although that check is specifically only for Travis AFAICT) https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/external-http/basic.php?marks=8,9

#17 in reply to: ↑ 16 ; follow-up: @mikeschroder
3 months ago

Replying to dd32:

Replying to SergeyBiryukov:

Reopening for 4.9.5 consideration.

@mikeschroder noted that the test still fails on the 4.9 branch. Per comment:4, it should be safe to bump the recommended version there as well.

While it should be safe, the test is only supposed to run on master/trunk: (Although that check is specifically only for Travis AFAICT) https://core.trac.wordpress.org/browser/trunk/tests/phpunit/tests/external-http/basic.php?marks=8,9

I noticed when running the tests locally on the 4.9 branch before a backport.

Edit: In case it's not supposed to work like this, running grunt test or grunt precommit locally both appear to run this test when the 4.9 branch is checked out.

Last edited 3 months ago by mikeschroder (previous) (diff)

#18 in reply to: ↑ 17 ; follow-up: @dd32
3 months ago

Replying to mikeschroder:

Replying to dd32: I noticed when running the tests locally on the 4.9 branch before a backport.

Edit: In case it's not supposed to work like this, running grunt test or grunt precommit locally both appear to run this test when the 4.9 branch is checked out.

Yeah, That's kind of what I'm hinting at - We can't backport these changes forever to branches we're backporting to. Backporting it to our current stable seems like a sane idea, but once 5.0 is out, the unit test would once again start failing on 4.9 backports. For 4.9, I say back-porting it is OK, but that we should look at something else going forward.

#19 in reply to: ↑ 18 @SergeyBiryukov
3 months ago

Replying to dd32:

For 4.9, I say back-porting it is OK, but that we should look at something else going forward.

Maybe adjust the test to skip when running on a 3-digit version?

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

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


2 months ago

#21 @ocean90
2 months ago

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

In 42847:

Readme: Update recommended PHP version to 7.2.

Merge of [42358] to the 4.9 branch.

Props otto42, johnbillion, rachelbaker, jorbin.
Fixes #42789.

Note: See TracTickets for help on using tickets.