191 test helper php 81 improvements #192

vraja-pro · 2023-07-25T10:03:13Z

Summary

This PR can be summarized in the following changelog entry:

Removes filter_input and filter_var for PHP 8.1.

Relevant technical choices:

Go to Tools->Yoast Test and check the following:

Domain dropdown

Go to Domain dropdown card and change to local.
Refresh and check option is saved.
Restore to live and check it is saved.

Downgrade Yoast SEO

Go to Downgrade Yoast SEO and add a previous version and save.
Refresh and check version is saved.

Inline script

Go to Inline script card and check the checkbox.
Add to the Script box console.log('test'); and save.
Refresh and check it is saved.
Clear Script box and disable the Inline script and save.
Refresh and check it is saved.

Plugin toggler

Go to Plugin toggler card and enable.
Follow the test instructions here.

Schema

Go to Schema card and select different option for Influence the Breadcrumb Graph piece.
Select different option for Influence the WebPage Graph piece and save.
Refresh and check it is saved.

XML Sitemaps

Go to XML Sitemaps and check the checkbox.
Refresh and check it is saved.

Milestone

I've attached the next release's milestone to this pull request.

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

Test instructions for QA when the code is in the RC

QA should use the same steps as above.

QA can test this PR by following these steps:

Fixes #191

igorschoester

CR 🏗️

igorschoester · 2023-08-22T14:10:20Z

.gitignore

+
+.vscode/


I think this can live in the upper section next to say .idea (from PHPStorm) just fine?

src/domain-dropdown.php

igorschoester · 2023-08-22T14:15:39Z

src/downgrader.php

@@ -58,7 +58,8 @@ public function handle_submit() {
 			return;
 		}

-		$target_version = \filter_var( \wp_unslash( $_POST['target_version'] ) );
+		$target_version = ( isset( $_POST['target_version'] ) ) ? \sanitize_text_field( \wp_unslash( $_POST['target_version'] ) ) : null;


I think you want to add a is_string check here too. I'm not sure unslash and sanitize text field like operating on non-strings.

Also, the null fallback does not seem compatible with the downgrade method! What did you want to achieve?

igorschoester · 2023-08-22T14:18:19Z

src/plugin-toggler.php

+			// phpcs:ignore WordPress.Security.NonceVerification.Recommended -- The nonce is verified above.
+			$group = ( isset( $_GET['group'] ) ) ? \sanitize_text_field( \wp_unslash( $_GET['group'] ) ) : null;
+			// phpcs:ignore WordPress.Security.NonceVerification.Recommended -- The nonce is verified above.
+			$plugin = ( isset( $_GET['plugin'] ) ) ? \sanitize_text_field( \wp_unslash( $_GET['plugin'] ) ) : null;


Same as above, about the is_string. As well as using null as fallback, that does not seem compatible with the methods they are passed too.

igorschoester · 2023-08-22T14:21:34Z

src/plugin-toggler.php

@@ -422,7 +424,7 @@ private function deactivate_plugin_group( $group ) {
 	 */
 	private function verify_nonce() {
 		// Get the nonce value.
-		$ajax_nonce = \filter_input( \INPUT_GET, 'ajax_nonce' );
+		$ajax_nonce = isset( $_GET['ajax_nonce'] ) ? sanitize_text_field( wp_unslash( $_GET['ajax_nonce'] ) ) : null;


In this case I think we can just check isset and then pass it to the nonce.
wp_verify_nonce does a (string) and hash_equals, which I think takes care of the need to is_string and sanitize_text_field and possibly the wp_unslash.
But as said in another PR, I'm not entirely sure on this 😅

igorschoester · 2023-08-22T14:23:59Z

src/schema.php

-		$is_needed_breadcrumb = $this->validate_submit( \filter_input( \INPUT_POST, 'is_needed_breadcrumb' ) );
-		$is_needed_webpage    = $this->validate_submit( \filter_input( \INPUT_POST, 'is_needed_webpage' ) );
+		if ( isset( $_POST['is_needed_breadcrumb'] ) ) {
+			$is_needed_breadcrumb           = \sanitize_text_field( \wp_unslash( $_POST['is_needed_breadcrumb'] ) );


validate_submit method is strict comparing only. Seems like we do not need sanitize_text_field here.

igorschoester · 2023-08-22T14:24:09Z

src/schema.php

-		$this->option->set( 'is_needed_breadcrumb', $is_needed_breadcrumb );
-		$this->option->set( 'is_needed_webpage', $is_needed_webpage );
+		if ( isset( $_POST['is_needed_webpage'] ) ) {
+			$is_needed_webpage           = \sanitize_text_field( \wp_unslash( $_POST['is_needed_webpage'] ) );


Same as above: validate_submit method is strict comparing only. Seems like we do not need sanitize_text_field here.

src/xml-sitemaps.php

Co-authored-by: Igor <[email protected]>

igorschoester

CR 🏗️

src/plugin-toggler.php

src/inline-script.php

Co-authored-by: Igor <[email protected]>

igorschoester

CR & AC ✅

About the test instructions:

Saving refreshes the page already
The plugin toggler functionality doesn't seem to work anymore, but that is unrelated to this PR. I'm pretty sure it is obsolete since we made it so that both plugins are active at the same time. It used to be that Premium totally included Free. I added a draft issue on our tech debt board.

vraja-pro added the changelog: non-user-facing label Jul 25, 2023

vraja-pro added this to the 1.19 milestone Jul 25, 2023

enricobattocchi modified the milestones: 1.19, 1.18 Aug 4, 2023

enricobattocchi changed the base branch from develop to release/1.18 August 4, 2023 12:24

enricobattocchi force-pushed the 191-test-helper-php-81-improvements branch 2 times, most recently from 0fb73ac to 4310295 Compare August 4, 2023 12:27

igorschoester requested changes Aug 22, 2023

View reviewed changes

vraja-pro changed the base branch from release/1.18 to develop August 23, 2023 08:56

vraja-pro changed the base branch from develop to main August 23, 2023 08:56

vraja-pro changed the base branch from main to release/1.18 August 23, 2023 08:56

vraja-pro and others added 18 commits August 23, 2023 12:18

php 8.1 improvements - removed filter_input

a2d35ca

php 8.1 improvements - removed filter_input

8f5b5d4

Improved function doc comment

b37826f

php 8.1 improvements - removed filter_input

8509d2d

php 8.1 improvements - removed filter_input

9296a6d

php 8.1 improvements - removed filter_input

a48fa97

php 8.1 improvements - removed filter_var

4266cdc

update ignore for launch.json vscode debug file

4cf3226

replace filter_input for php 8.1

58fd03f

php fix cs

3ab21c5

refactor vscode position

a931376

refactor target_version

d1a32ae

refactor ajax_toggle_plugin

89132e4

no need to sanitize nonce

c438246

Update src/domain-dropdown.php

8832fa2

Co-authored-by: Igor <[email protected]>

Update src/xml-sitemaps.php

bdf1ebc

Co-authored-by: Igor <[email protected]>

removed unnecessary sanitisation

2fb414a

Merge branch '191-test-helper' into 191-test-helper-php-81-improvements

d707698

vraja-pro force-pushed the 191-test-helper-php-81-improvements branch from a9d93f7 to d707698 Compare August 23, 2023 10:26

igorschoester requested changes Aug 23, 2023

View reviewed changes

src/plugin-toggler.php Outdated Show resolved Hide resolved

src/plugin-toggler.php Outdated Show resolved Hide resolved

src/inline-script.php Outdated Show resolved Hide resolved

src/inline-script.php Outdated Show resolved Hide resolved

vraja-pro and others added 5 commits August 23, 2023 13:36

Update src/inline-script.php

ff16949

Co-authored-by: Igor <[email protected]>

Update src/inline-script.php

e5b6fb0

Co-authored-by: Igor <[email protected]>

Update src/plugin-toggler.php

6193b46

Co-authored-by: Igor <[email protected]>

Update src/plugin-toggler.php

fd53497

Co-authored-by: Igor <[email protected]>

add php cs ignore comment for nonce sanitization

1dfe0f7

igorschoester approved these changes Aug 24, 2023

View reviewed changes

igorschoester merged commit 644fd77 into release/1.18 Aug 24, 2023
7 checks passed

igorschoester deleted the 191-test-helper-php-81-improvements branch August 24, 2023 07:00

enricobattocchi mentioned this pull request Oct 13, 2023

Test helper: Plugin toggler functionality seems obsolete #203

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

191 test helper php 81 improvements #192

191 test helper php 81 improvements #192

vraja-pro commented Jul 25, 2023 •

edited

Loading

igorschoester left a comment

igorschoester Aug 22, 2023

igorschoester Aug 22, 2023

igorschoester Aug 22, 2023

igorschoester Aug 22, 2023

igorschoester Aug 22, 2023

igorschoester Aug 22, 2023

igorschoester left a comment

igorschoester left a comment •

edited

Loading


		.vscode/

191 test helper php 81 improvements #192

191 test helper php 81 improvements #192

Conversation

vraja-pro commented Jul 25, 2023 • edited Loading

Summary

Relevant technical choices:

Milestone

Test instructions

Test instructions for the acceptance test before the PR gets merged

Test instructions for QA when the code is in the RC

igorschoester left a comment

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester Aug 22, 2023

Choose a reason for hiding this comment

igorschoester left a comment

Choose a reason for hiding this comment

igorschoester left a comment • edited Loading

Choose a reason for hiding this comment

vraja-pro commented Jul 25, 2023 •

edited

Loading

igorschoester left a comment •

edited

Loading