Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PHP 7.4 Support #251

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Conversation

shawnhooper
Copy link

This patch adds support for PHP 7.4 compatibility checking. To test it, add this function to your theme/plugin:

	function ternary_deprecation() {

		return 1 ? 2 : 3 ? 4 : 5; // deprecated in 7.4

	}

and run the plugin.

@shawnhooper
Copy link
Author

It looks like the Travis configuration needs to be updated with an image that includes PHP 7.4.

@jrfnl
Copy link

jrfnl commented Nov 17, 2019

It looks like the Travis configuration needs to be updated with an image that includes PHP 7.4.

Yes, Travis has been very slow with that. For now you can use the unofficial "7.4snapshot" image which was created by one of the PHP core devs.

@kazazor kazazor mentioned this pull request Dec 14, 2019
@kazazor
Copy link

kazazor commented Dec 14, 2019

It seems you can make it work with Travis now without 7.4snapshot.
Please check this github issue & pull request of Google's site kit WP plugin.

@jrfnl
Copy link

jrfnl commented Dec 14, 2019

@kazazor Correct, Travis added a workable 7.4 image last week.

@kazazor
Copy link

kazazor commented Dec 14, 2019

Thanks @jrfnl ! Would be great to have the ability to check for php 7.4 on my website (https://tachlescalcala.com/) once this will be merged :)

@jrfnl
Copy link

jrfnl commented Dec 14, 2019

@kazazor Well, you already can using the underlying software: https://github.com/PHPCompatibility/PHPCompatibilityWP

@kazazor
Copy link

kazazor commented Dec 14, 2019

@jrfnl well I'm using this plugin but it does not currently have 7.4 option in it.
This is how I ended up in this PR :) It points here

@jrfnl
Copy link

jrfnl commented Dec 14, 2019

@kazazor I understand. I just meant to point out that this plugin is a wrapper around other software and that that original software already has the PHP 7.4 checks in place.

Copy link
Member

@anthonyburchell anthonyburchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @shawnhooper ! I've added a note in my review about the new 7.4 images being available, so we'll no longer need to use the snapshot.

.travis.yml Outdated
@@ -6,6 +6,7 @@ php:
- 7.1
- 7.2
- 7.3
- 7.4snapshot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems per #251 (comment) that Travis has the 7.4 image available now. I think that we should probably be using that before merge. Here's an example of folks using the new image already: https://github.com/google/site-kit-wp/pull/946/files

@stevenkword stevenkword self-requested a review January 16, 2020 22:03
Copy link
Member

@anthonyburchell anthonyburchell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @shawnhooper ! There are a few more 7.4snapshot references in the yaml. I also noticed CI failures, I think because it is being tested against (5.2) which is not php 7.4 compatible. 7.4 compatibility was added in 5.3 so things should pass CI if we use that one.

.travis.yml Outdated
@@ -30,12 +31,17 @@ matrix:
env: WP_VERSION=5.2 WP_MULTISITE=1
- php: 7.3
env: WP_VERSION=latest WP_MULTISITE=1
- php: 7.3
- php: 7.4snapshot
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a few more references to the snapshot version in the yaml and also the version tested against is 5.2 which is not php 7.4 compatible. We may need to use 5.3 as our target version or CI will fail and not allow us to merge/publish this release. There are two other references to the 7.4 snapshot and 5.2 WordPress.

@shawnhooper
Copy link
Author

@anthonyburchell Thanks. I've removed the rest of the 7.4snapshot references and updated the 7.4 multisite test to use WP 5.3. We'll see how that goes. My suspicion is that the problem is actually a problem between the version of PHP CodeSniffer we're using and PHP 7.4. Going to try to play with that a bit tonight to confirm.

@jrfnl
Copy link

jrfnl commented Jan 29, 2020

FYI:

  • Correct: WP itself wasn't fully compatible with PHP 7.4 until WP 5.3.
  • PHPCS is compatible with PHP 7.4 as of version 3.5.0.

Also: PHPCompatibility 9.3.5 has been released in the mean time, as well as version 0.6.2 of the dealerdirect/phpcodesniffer-composer-installer package (needs a change in the composer.json as Composer treats minors < 1.0.0 as majors).

@anthonyburchell
Copy link
Member

Thanks @jrfnl ! @shawnhooper thanks for those updates! We're getting closer! I'm seeing that sniffs are failing and my best guess is that given the info in the comment above, the php_codesniffer package should be changed to 3.5.0.

It probably wouldn't hurt to change phpcodesniffer-composer-installer to 0.6.2 as well as they are likely very compatible with each other. :)

I believe we'll need an updated composer.lock as well after these changes. With those changes, I believe we'll be passing all tests and ready to merge.

@shawnhooper
Copy link
Author

@anthonyburchell Right now we're on PHP CodeSniffer 2.9.2, which is the last version of the 2.x branch. Support for PHP 7.4 was introduced in PHPCodeSniffer 3.5.0.

The upgrade from PHPCS 2.x to 3.x has breaking changes, including that the PHP_CodeSniffer_CLI class no longer exists. It looks like we have to use the new \PHP_CodeSniffer\Runner() class instead. Haven't got that working yet (this isn't a documented use, so it's reading through the PHPCS code to trying things)

@anthonyburchell
Copy link
Member

Yeah looks like it does need to use the runner class. I found this gist that shows how they are structured now if that's helpful: https://gist.github.com/gsherwood/aafd2c16631a8a872f0c4a23916962ac

@Rubaka
Copy link

Rubaka commented Dec 10, 2020

Hello, any updates on this? since 1 year already from the initial commit, 8.0 already released but we still waiting for 7.4 support.
Could you prioritize this please?

@jeffpaul jeffpaul added this to the Future Release milestone Jun 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants