Bug/#5110 fix php81 warnings #5968

kuasha420 · 2022-10-05T01:31:55Z

Summary

Addresses issue:

Deprecated PHP Errors on Site Kit with PHP 8.1 #5110

Relevant technical choices

The remaining strpos and str_replace errors are related to how we use add_submenu_page and will be addressed in #5998.

The remaining rtrim errors are caused by load_script_textdomain function and we are consuming the function correctly in our code. It's something that will probably be fixed in Core.

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

asvinb

@kuasha420 I left some comments in the PR. Can you check them out? Additionally, I see some other warnings cropping up which I think we can suppress:

Since we are supporting PHP 5, I think maybe we can add the #[ReturnTypeWillChange] attribute: https://php.watch/versions/8.1/ReturnTypeWillChange Curious to hear your thoughts @eugene-manuilov

There is also a warning here: https://github.com/google/site-kit-wp/blob/develop/includes/Modules/Thank_With_Google.php#L325 Maybe we can cast widgets to an array to prevent the warning.

asvinb · 2022-10-13T12:10:00Z

includes/Core/Admin/Screens.php

@@ -367,7 +367,10 @@ private function get_screens() {
 					'render_callback'  => function( Context $context ) {
 						$is_view_only = ! $this->authentication->is_authenticated();

-						$setup_slug = $context->input()->filter( INPUT_GET, 'slug', FILTER_SANITIZE_STRING );
+						$setup_slug = $context->input()->filter( INPUT_GET, 'slug' );


@kuasha420 Is there a reason why we can't call htmlspecialchars here itself? 🤔

The value of slug can be empty (null) and it throws another error for calling htmlspecialchars with null if we don't check.

In that case @kuasha420 is it possible to use the Elvis operator here to be consistent with how it's done in other places where you used htmlspecialchars?

asvinb · 2022-10-13T12:15:16Z

includes/Core/Assets/Assets.php

@@ -878,18 +878,14 @@ private function get_inline_tracking_data() {
 	 * @return array The inline data to be output.
 	 */
 	private function get_inline_data() {
-		$current_user = wp_get_current_user();


asvinb · 2022-10-13T12:20:45Z

includes/Modules/Analytics.php

@@ -277,17 +277,17 @@ protected function handle_provisioning_callback() {
 		}

 		// Check for a returned error.
-		$error = $input->filter( INPUT_GET, 'error', FILTER_SANITIZE_STRING );
+		$error = htmlspecialchars( $input->filter( INPUT_GET, 'error' ) );


@kuasha420 I think error might be null which in that case PHP will complain about passing null to parameter #1 ($string) of type string is deprecated

kuasha420 · 2022-10-14T04:59:30Z

Thank you for the review, @asvinb !

The errors you mention, I am not seeing them in InstaWP. What's your php 8.1 version. Are those development only error?

I have addressed your other comments. Cheers.

eugene-manuilov · 2022-10-17T14:24:47Z

Since we are supporting PHP 5, I think maybe we can add the #[ReturnTypeWillChange] attribute: https://php.watch/versions/8.1/ReturnTypeWillChange Curious to hear your thoughts @eugene-manuilov

Good question, @asvinb. I lean toward using the ReturnTypeWillChange attribute although I would also like to hear thoughts from @aaemnnosttv.

asvinb · 2022-10-18T12:17:16Z

includes/Core/Admin/Screens.php

@@ -367,7 +367,10 @@ private function get_screens() {
 					'render_callback'  => function( Context $context ) {
 						$is_view_only = ! $this->authentication->is_authenticated();

-						$setup_slug = $context->input()->filter( INPUT_GET, 'slug', FILTER_SANITIZE_STRING );
+						$setup_slug = $context->input()->filter( INPUT_GET, 'slug' );


In that case @kuasha420 is it possible to use the Elvis operator here to be consistent with how it's done in other places where you used htmlspecialchars?

asvinb · 2022-10-18T12:19:58Z

@kuasha420 I'am on PHP 8.1.9 where i can see those errors via QueryMonitor.

kuasha420 · 2022-11-01T12:43:02Z

@asvinb Thank you for giving me your PHP version.

However, I am not seeing the same errors in my local or an InstaWP site when using PHP 8.1 and rebased to latest develop. I am seeing different errors which are similar and coming from guzzlehttp package.

Other than that, I've also tried to add the attribute to the methods based off of your screenshot, but not sure if that's the correct approach and the attribute comment also conflicts with phpcs rules. I have not spent much time on it but looks like we need to upgrade our php codensiffer to properly use attributes without adding bunch of phpcs ignores around it.

        // phpcs:disable Squiz.Commenting.InlineComment.WrongStyle,Squiz.Commenting.FunctionComment.WrongStyle, Squiz.PHP.CommentedOutCode.Found
	#[\ReturnTypeWillChange]
	public function offsetUnset( $key ) {

Not sure how to proceed here. Cheers.

cc @eugene-manuilov @aaemnnosttv

asvinb · 2022-11-02T14:18:53Z

@kuasha420 I checked there are no no longer warnings from our code. I'll let @eugene-manuilov @aaemnnosttv check the ReturnTypeWillChange attribute

eugene-manuilov · 2022-11-02T21:51:47Z

Other than that, I've also tried to add the attribute to the methods based off of your screenshot, but not sure if that's the correct approach and the attribute comment also conflicts with phpcs rules. I have not spent much time on it but looks like we need to upgrade our php codensiffer to properly use attributes without adding bunch of phpcs ignores around it.

Yes, i think we can allow it in phpcs config file, but i am not sure how to do it. @aaemnnosttv do you know if it is feasible?

aaemnnosttv · 2022-11-10T15:43:48Z

@asvinb @eugene-manuilov Can we add the ignore inline with the attribute rather than around it?

kuasha420 · 2022-11-30T10:33:12Z

@aaemnnosttv I have updated the phpcs disables with ignores. Feels quite a bit repitetive but that's the best we can do for now. We should explore proper solution about using PHP attributes with phpcs in a follow up issue that I've created here. Assigning to you for a merge review. Cheers.

techanvil

Code looks good with just a couple of comments from me to take a look at.

I've given this another successful smoke test on a PHP 8.1 install, and the remaining work has been suitably captured in followup issues, so, minor comments aside this looks good to go.

techanvil · 2022-12-14T16:22:17Z

includes/Core/REST_API/Data_Request.php

-	public function offsetExists( $key ) {
+	// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
+	#[\ReturnTypeWillChange]
+	public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle


When I run phpcs with PHP 8, these lines still raise errors:

Loosening up the rule definition allows linting to pass in PHP 8 as well as older versions.

Suggested change

public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle

public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

techanvil · 2022-12-14T16:22:37Z

includes/Core/REST_API/Data_Request.php

-	public function offsetGet( $key ) {
+	// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
+	#[\ReturnTypeWillChange]
+	public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle


As above

Suggested change

public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle

public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

techanvil · 2022-12-14T16:22:48Z

includes/Core/REST_API/Data_Request.php

-	public function offsetSet( $key, $value ) {
+	// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
+	#[\ReturnTypeWillChange]
+	public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle


As above

Suggested change

public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle

public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment

techanvil · 2022-12-14T16:23:05Z

includes/Core/REST_API/Data_Request.php

-	public function offsetUnset( $key ) {
+	// phpcs:ignore Squiz.Commenting.InlineComment.WrongStyle,Squiz.PHP.CommentedOutCode.Found
+	#[\ReturnTypeWillChange]
+	public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle


As above

Suggested change

public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle

public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

techanvil · 2022-12-14T16:28:03Z

tests/phpunit/integration/ContextTest.php


 		$_GET['dirty'] = '<script>dirt</script>';
-		$this->assertEquals( 'dirt', $context->input()->filter( INPUT_GET, 'dirty', FILTER_SANITIZE_STRING ) );
+		$this->assertEquals( '&lt;script&gt;dirt&lt;/script&gt;', htmlspecialchars( $context->input()->filter( INPUT_GET, 'dirty' ) ) );


This test doesn't seem so useful now as it's effectively just testing the standard htmlspecialchars function. I'd suggest it should be removed.

Suggested change

$_GET['dirty'] = '<script>dirt</script>';

$this->assertEquals( 'dirt', $context->input()->filter( INPUT_GET, 'dirty', FILTER_SANITIZE_STRING ) );

$this->assertEquals( '<script>dirt</script>', htmlspecialchars( $context->input()->filter( INPUT_GET, 'dirty' ) ) );

techanvil

Nice one @kuasha420, LGTM!

kuasha420 force-pushed the bug/#5110-fix-php81-warnings branch from d6eba0f to a192c4c Compare October 13, 2022 00:42

asvinb requested changes Oct 13, 2022

View reviewed changes

asvinb requested changes Oct 18, 2022

View reviewed changes

kuasha420 force-pushed the bug/#5110-fix-php81-warnings branch from 556789e to 1578f30 Compare November 1, 2022 11:00

kuasha420 added 9 commits November 30, 2022 16:06

Refactor usage of FILTER_SANITIZE_STRING.

1ec76fd

Fix failing phpunit test.

e8f99a9

Change default value of filter_input's options param to 0.

36f7040

Fix htmlspecialchars null warnings.

fc51d2a

Do not call htmlspecialchars with null setup_slug.

0252423

Check $error to be not empty before running htmlspecialchars.

0533f2b

Use Elvis operator.

6930f15

Add ReturnTypeWillChange attribute to supress warnings.

575cc70

Add spaces in phpcs multiple ignores.

0493f60

kuasha420 force-pushed the bug/#5110-fix-php81-warnings branch from 04ae767 to 0493f60 Compare November 30, 2022 10:06

Replace phpcs disables with inline ignores.

d6d889b

kuasha420 mentioned this pull request Dec 1, 2022

Several PHP Deprecated warnings #6236

Closed

techanvil requested changes Dec 14, 2022

View reviewed changes

Address CR feedback.

9197ff7

techanvil approved these changes Dec 16, 2022

View reviewed changes

techanvil merged commit 8beca04 into develop Dec 16, 2022

techanvil deleted the bug/#5110-fix-php81-warnings branch December 16, 2022 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug/#5110 fix php81 warnings #5968

Bug/#5110 fix php81 warnings #5968

kuasha420 commented Oct 5, 2022 •

edited by techanvil

Loading

asvinb left a comment

asvinb Oct 13, 2022

kuasha420 Oct 14, 2022

asvinb Oct 18, 2022

asvinb Oct 13, 2022

asvinb Oct 13, 2022

kuasha420 commented Oct 14, 2022 •

edited

Loading

eugene-manuilov commented Oct 17, 2022 •

edited

Loading

asvinb Oct 18, 2022

asvinb commented Oct 18, 2022

kuasha420 commented Nov 1, 2022 •

edited

Loading

asvinb commented Nov 2, 2022

eugene-manuilov commented Nov 2, 2022

aaemnnosttv commented Nov 10, 2022

kuasha420 commented Nov 30, 2022

techanvil left a comment

techanvil Dec 14, 2022

techanvil Dec 14, 2022

techanvil Dec 14, 2022

techanvil Dec 14, 2022

techanvil Dec 14, 2022

techanvil left a comment

	public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
	public function offsetExists( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

	public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
	public function offsetGet( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

	public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
	public function offsetSet( $key, $value ) { // phpcs:ignore Squiz.Commenting.FunctionComment

	public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment.WrongStyle
	public function offsetUnset( $key ) { // phpcs:ignore Squiz.Commenting.FunctionComment

Bug/#5110 fix php81 warnings #5968

Bug/#5110 fix php81 warnings #5968

Conversation

kuasha420 commented Oct 5, 2022 • edited by techanvil Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

asvinb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuasha420 commented Oct 14, 2022 • edited Loading

eugene-manuilov commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

asvinb commented Oct 18, 2022

kuasha420 commented Nov 1, 2022 • edited Loading

asvinb commented Nov 2, 2022

eugene-manuilov commented Nov 2, 2022

aaemnnosttv commented Nov 10, 2022

kuasha420 commented Nov 30, 2022

techanvil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

techanvil left a comment

Choose a reason for hiding this comment

kuasha420 commented Oct 5, 2022 •

edited by techanvil

Loading

kuasha420 commented Oct 14, 2022 •

edited

Loading

eugene-manuilov commented Oct 17, 2022 •

edited

Loading

kuasha420 commented Nov 1, 2022 •

edited

Loading