Fix deprecation warnings by thelovekesh · Pull Request #7614 · ampproject/amp-wp

thelovekesh · 2023-09-03T12:15:48Z

Summary

Fix deprecation warnings due to PHP 8.1 in the AMP Validation module.
Fix registration of the AMP Onboarding Wizard submenu page.

Checklist

My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

thelovekesh · 2023-09-12T14:12:08Z

@westonruter Can you please update the patch at westonruter/PHP-CSS-Parser#2 which is causing this deprecation warning

PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 157
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 157
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 158
PHP Deprecated:  Using ${var} in strings is deprecated, use {$var} instead in /tmp/wordpress/src/wp-content/plugins/amp/vendor/sabberworm/php-css-parser/src/Parsing/ParserState.php on line 159

github-actions · 2023-09-12T14:17:34Z

Plugin builds for e8f73c5 are ready 🛎️!

Download development build
Download production build

src/Admin/OnboardingWizardSubmenu.php

… cases

westonruter · 2023-09-12T20:55:20Z

includes/validation/class-amp-validation-error-taxonomy.php

-					'edit_terms'   => 'do_not_allow', // Terms are created (and updated) programmatically.
+					// Terms are created (and updated) programmatically, but we still need to assign capabilities while generating edit link for term.
+					// @see <https://github.com/ampproject/amp-wp/issues/7604#issuecomment-1704244763>.
+					'edit_terms'   => AMP_Validation_Manager::VALIDATE_CAPABILITY,


Won't this then allow users to access the edit term page when they shouldn't be able to access it?

At the very least we'll also need to unset($actions['edit']) in:

amp-wp/includes/validation/class-amp-validation-error-taxonomy.php

Lines 1679 to 1738 in 828f229

public static function filter_tag_row_actions( $actions, WP_Term $tag ) {

global $pagenow;

$term_id = $tag->term_id;

$term = get_term( $term_id ); // We don't want filter=display given by $tag.

/*

* Hide deletion link since a validation error should only be removed once

* it no longer has an occurrence on the site. When a validated URL is re-checked

* and it no longer has this validation error, then the count will be decremented.

* When a validation error term no longer has a count, then it is hidden from the

* list table. A cron job could periodically delete terms that have no counts.

*/

unset( $actions['delete'] );

if ( 'post.php' === $pagenow ) {

$actions['details'] = sprintf(

'<button type="button" aria-label="%s" class="single-url-detail-toggle button-link">%s</button>',

esc_attr__( 'Toggle error details', 'amp' ),

esc_html__( 'Details', 'amp' )

);

$actions['copy'] = sprintf(

'<button type="button" class="single-url-detail-copy button-link" data-error-json="%s">%s</button>',

esc_attr( self::get_error_details_json( $term ) ),

esc_html__( 'Copy to clipboard', 'amp' )

);

} elseif ( 'edit-tags.php' === $pagenow ) {

$actions['details'] = sprintf(

'<a href="%s">%s</a>',

admin_url(

add_query_arg(

[

self::TAXONOMY_SLUG => $term->name,

'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,

],

'edit.php'

)

),

esc_html__( 'Details', 'amp' )

);

if ( 0 === $term->count ) {

$actions['delete'] = sprintf(

'<a href="%s">%s</a>',

wp_nonce_url(

add_query_arg( array_merge( [ 'action' => 'delete' ], compact( 'term_id' ) ) ),

'delete'

),

esc_html__( 'Delete', 'amp' )

);

}

}

$actions = wp_array_slice_assoc( $actions, [ 'details', 'delete', 'copy' ] );

self::$current_validation_error_row_index++;

return $actions;

}

Then there's the question of the REST API. I suppose they can't access it because it's not public and show_in_rest is not true.

On the other hand, is this actually a bug that should rather be fixed in core?

Won't this then allow users to access the edit term page when they shouldn't be able to access it?

We are not providing an edit link so it will be hard to generate the link with the correct tag_ID.

At the very least we'll also need to unset($actions['edit']) in:

I think it will do it.

amp-wp/includes/validation/class-amp-validation-error-taxonomy.php

Line 1733 in 828f229

$actions = wp_array_slice_assoc( $actions, [ 'details', 'delete', 'copy' ] );

On the other hand, is this actually a bug that should rather be fixed in core?

Yes, and I have filed a ticket for that - https://core.trac.wordpress.org/ticket/59336

westonruter · 2023-09-12T21:07:09Z

src/Admin/OnboardingWizardSubmenu.php

 	public function register() {
 		add_submenu_page(
-			'',
+			'options.php',


Linking your pervious #7614 (comment) here.

This was made an empty string in e30168c.

You say:

But adding options.php provides the same behavior without breaking anything. So updating it to options.php.

Can you clarify further what this is doing? Is it that there is no menu item for options.php so it won't add anything to the menu?

Yes. It won't be accessible from a submenu item but rather only accessible via the URL which is options.php?page=amp-onboarding-wizard.

Or if we want to hook it to amp-options page and keep it hidden in the submenu items then we need to only register it when it's called via URL - e0ce678

This reverts commit 308e17c.

westonruter · 2023-09-14T16:52:15Z

includes/validation/class-amp-validation-error-taxonomy.php

 			add_filter( 'pre_term_description', 'wp_filter_kses', $has_pre_term_description_filter );
 		}

+		// Bail if redirect_is passed as null.


redirect_is?

It should be $redirect_to. Will update it.

Fixed in 6d8a722

westonruter · 2023-09-14T16:54:19Z

tests/php/src/Admin/OptionsMenuTest.php

+
+		// Set title to be used in the screen.
+		global $title;
+		$title = 'Test Title';


I see this is coming from 1e8de9a. It was needed due to a PHP warning during tests?

Yes, while rendering the screen it requires the title, but the title is not being set anywhere.

tests/php/validation/test-class-amp-validation-error-taxonomy.php

westonruter · 2023-09-14T16:57:53Z

tests/php/validation/test-class-amp-validation-error-taxonomy.php

Is this commit needed to work around the issue in tests which you're fixing in core?

Which commit? I am unable to see any diff with this comment.

Sorry, I meant 2c43d0d

It's different from that one. handle_single_url_page_bulk_and_inline_actions() uses get_edit_post_link() which requires edit_post capability in general. In test cases we were not setting up the user hence it was resulting into deprecation warnings.

What was the deprecation warning?

Those were the same as passing null to add_query_arg()

Deprecated: stripos(): Passing null to parameter #1 ($haystack) of type string is deprecated in /tmp/wordpress/src/wp-includes/functions.php on line 1157 Deprecated: str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in /tmp/wordpress/src/wp-includes/functions.php on line 1164

westonruter

🎉

thelovekesh added 3 commits September 3, 2023 17:37

Add guard to avoid using add_query_arg() if URL does not exists

1bd40ef

Fix deprecation warnings due to AMP validation test cases

2c43d0d

Update PHPUnit config to convert deprecations to error

020eaaa

westonruter added this to the v2.4.3 milestone Sep 5, 2023

thelovekesh added 3 commits September 12, 2023 14:26

Merge branch 'develop' into fix/deprecation-warning

6e9f2cf

Update capability for amp_validation_error taxonomy

308e17c

Add title in OptionsMenu tests before rendering settings page

1e8de9a

thelovekesh force-pushed the fix/deprecation-warning branch from 7ffc0eb to 1e8de9a Compare September 12, 2023 11:43

thelovekesh marked this pull request as ready for review September 12, 2023 14:12

thelovekesh commented Sep 12, 2023

View reviewed changes

src/Admin/OnboardingWizardSubmenu.php Outdated Show resolved Hide resolved

Add amp-onboarding-wizard submenu page on options.php menu

14b70d7

thelovekesh force-pushed the fix/deprecation-warning branch from e0ce678 to 14b70d7 Compare September 12, 2023 14:32

Add options.php as parent slug in onboarding wizard submenu page test…

41b2ff6

… cases

thelovekesh requested a review from westonruter September 12, 2023 14:53

westonruter reviewed Sep 12, 2023

View reviewed changes

Revert "Update capability for amp_validation_error taxonomy"

091fdfd

This reverts commit 308e17c.

thelovekesh requested a review from westonruter September 14, 2023 16:44

westonruter reviewed Sep 14, 2023

View reviewed changes

tests/php/validation/test-class-amp-validation-error-taxonomy.php Show resolved Hide resolved

westonruter reviewed Sep 14, 2023

View reviewed changes

thelovekesh added 2 commits September 14, 2023 22:54

Fix typo in inline comments

6d8a722

Try increasing navigation timeout for e2e tests to 60000 ms

e8f73c5

westonruter approved these changes Sep 14, 2023

View reviewed changes

westonruter merged commit 2c4df9e into develop Sep 14, 2023

westonruter deleted the fix/deprecation-warning branch September 14, 2023 17:54

westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Nov 6, 2023

	public static function filter_tag_row_actions( $actions, WP_Term $tag ) {
	global $pagenow;

	$term_id = $tag->term_id;
	$term = get_term( $term_id ); // We don't want filter=display given by $tag.

	/*
	* Hide deletion link since a validation error should only be removed once
	* it no longer has an occurrence on the site. When a validated URL is re-checked
	* and it no longer has this validation error, then the count will be decremented.
	* When a validation error term no longer has a count, then it is hidden from the
	* list table. A cron job could periodically delete terms that have no counts.
	*/
	unset( $actions['delete'] );

	if ( 'post.php' === $pagenow ) {
	$actions['details'] = sprintf(
	'<button type="button" aria-label="%s" class="single-url-detail-toggle button-link">%s</button>',
	esc_attr__( 'Toggle error details', 'amp' ),
	esc_html__( 'Details', 'amp' )
	);

	$actions['copy'] = sprintf(
	'<button type="button" class="single-url-detail-copy button-link" data-error-json="%s">%s</button>',
	esc_attr( self::get_error_details_json( $term ) ),
	esc_html__( 'Copy to clipboard', 'amp' )
	);
	} elseif ( 'edit-tags.php' === $pagenow ) {
	$actions['details'] = sprintf(
	'<a href="%s">%s</a>',
	admin_url(
	add_query_arg(
	[
	self::TAXONOMY_SLUG => $term->name,
	'post_type' => AMP_Validated_URL_Post_Type::POST_TYPE_SLUG,
	],
	'edit.php'
	)
	),
	esc_html__( 'Details', 'amp' )
	);

	if ( 0 === $term->count ) {
	$actions['delete'] = sprintf(
	'<a href="%s">%s</a>',
	wp_nonce_url(
	add_query_arg( array_merge( [ 'action' => 'delete' ], compact( 'term_id' ) ) ),
	'delete'
	),
	esc_html__( 'Delete', 'amp' )
	);
	}
	}

	$actions = wp_array_slice_assoc( $actions, [ 'details', 'delete', 'copy' ] );

	self::$current_validation_error_row_index++;

	return $actions;
	}

Conversation

thelovekesh commented Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

thelovekesh commented Sep 12, 2023

Uh oh!

github-actions bot commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

thelovekesh commented Sep 3, 2023 •

edited

Loading

github-actions bot commented Sep 12, 2023 •

edited

Loading