Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

Updates for php8 #1092

Merged
merged 39 commits into from
Sep 23, 2022
Merged

Updates for php8 #1092

merged 39 commits into from
Sep 23, 2022

Conversation

whyisjake
Copy link
Contributor

Let's do this.

whyisjake and others added 29 commits September 14, 2022 20:08
- Add minimum-stability and prefer-stable properties.
- Add phpcompatbility/php-compatibility:dev-develop so that PHP 8-related sniffs are used.
- Add dealerdirect/phpcodesniffer-composer-installer to the allow-plugins configuration.
- Run through the Composer normalizer.
Reformat composer.json to use tab indentation.
Remove more development files from the root
Composer lock files should only be used at the project level, not the plugin dependency level. This supports those who run Composer at the site-level.
Save PHPCS configuration into a file to make running it on the command line consistent.
- Renames a test class file to follow PSR-4, since other file names are deprecated as of PHPUnit 9.
- Renames and refreshes the PHPUnit configuration file. Since there are only a few tests, coverage-related features are not enabled.
- Integrates Yoast\wp-test-utils package, which allows for easier testing of WordPress plugins across multiple PHPUnit versions, which is needed when constrained by which PHP versions are being tested against. This also allows for future BrainMonkey support if any WP functions need patching/mocking for non-WP unit tests (they can run quicker without having to spin up WP in a database test environment).
- Updated the install-wp-tests.sh script to a newer version from WP-CLI.
- Tests pass locally.
- Fix deprecated license SPDX identifier.
- Move PHPC_CodeSniffer Composer installer plugin to require-dev, and expand the constraints.
- Switch out Yoast PHPUnit Polyfills for WPTestUtils, which include PHPUnit Polyfills and more.
- Add scripts and script descriptions for running PHPCS, linting, and PHPUnit tests.
This particular extra space was causing PHPCS to have a fatal error for some reason, so this fix works around that.
- Validates the `composer.json` file.
- Lint PHP files.
- Lint PHPUnit and PHPCS config files.

PHPCS checks of PHP files are commented out, as there are currently a lot of issues to address.
GitHub Actions are in use now.
The Facebook SDK package has its own dependency which requires at least PHP 7.1, so we'll need to define the same.

Also updates some documentation to reflect the update in the repository name.
preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated under PHP 8.1
@GaryJones
Copy link
Contributor

As per https://github.com/Automattic/fb-instant-articles/actions/runs/3074081777 - the integration/WP tests (as little as they are) are now passing on PHP 7.1-PHP 8.0.

PHP 8.1 is failing on one item that needs fixing in @whyisjake's fork of the Facebook SDK. Jake, please see the tests errors on https://github.com/Automattic/fb-instant-articles/actions/runs/3074081777/jobs/4966631390 and adjust the strpos() as needed.

The branch is also passing linting (but now CS, when the full WPCS is added). There were no PHPCompatibility issues for 8.0.

I'll note that on plugin activation, I saw a PHP Warning: "Warning: Trying to access array offset on value of type null in /Users/gary/Sites/fbia/app/public/wp-content/plugins/fb-instant-articles/facebook-instant-articles.php on line 82", so there will still need to be a manual check and test of functionality because the automated tests are very very far from being comprehensive.

I'll also note that I renamed the repository to make the tests pass locally and on CI.

@GaryJones GaryJones marked this pull request as draft September 17, 2022 17:12
Warning: Trying to access array offset on value of type null in /Users/gary/Sites/fbia/app/public/wp-content/plugins/fb-instant-articles/facebook-instant-articles.php on line 82
@GaryJones
Copy link
Contributor

I saw a PHP Warning: "Warning: Trying to access array offset on value of type null

This PR now includes a commit to fix that PHP Warning.

@GaryJones
Copy link
Contributor

PHP 8.1 is failing on one item that needs fixing in @whyisjake's fork of the Facebook SDK. Jake, please see the tests errors on Automattic/fb-instant-articles/actions/runs/3074081777/jobs/4966631390 and adjust the strpos() as needed.

This should be fixed by merging whyisjake/facebook-instant-articles-sdk-php#2 and then running the tests here again.

@whyisjake whyisjake marked this pull request as ready for review September 21, 2022 21:54
@whyisjake
Copy link
Contributor Author

@GaryJones this is ready for review!

Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

Looks like some bad replacements in the readme file.

readme.txt Outdated Show resolved Hide resolved
readme.txt Outdated Show resolved Hide resolved
readme.txt Outdated Show resolved Hide resolved
readme.txt Outdated Show resolved Hide resolved
readme.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

These changes all look good. I'd like to get this out as a beta before bumping version and tagging as a final release. Might also give us chance to add in one or two more items from the 4.3.0 milestone.

@GaryJones GaryJones added this to the 4.3.0 milestone Sep 23, 2022
@GaryJones GaryJones changed the base branch from master to develop September 23, 2022 10:24
@GaryJones GaryJones merged commit 99b02e3 into develop Sep 23, 2022
@GaryJones GaryJones deleted the php8 branch September 23, 2022 10:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants