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.

Sign up for GitHub

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

Fix deprecation warnings #7614

Merged
merged 11 commits into from
Sep 14, 2023
Merged

Fix deprecation warnings #7614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1bd40ef
Add guard to avoid using add_query_arg() if URL does not exists
thelovekesh Sep 3, 2023
2c43d0d
Fix deprecation warnings due to AMP validation test cases
thelovekesh Sep 3, 2023
020eaaa
Update PHPUnit config to convert deprecations to error
thelovekesh Sep 3, 2023
6e9f2cf
Merge branch 'develop' into fix/deprecation-warning
thelovekesh Sep 12, 2023
308e17c
Update capability for amp_validation_error taxonomy
thelovekesh Sep 12, 2023
1e8de9a
Add title in OptionsMenu tests before rendering settings page
thelovekesh Sep 12, 2023
14b70d7
Add amp-onboarding-wizard submenu page on options.php menu
thelovekesh Sep 12, 2023
41b2ff6
Add options.php as parent slug in onboarding wizard submenu page test…
thelovekesh Sep 12, 2023
091fdfd
Revert "Update capability for amp_validation_error taxonomy"
thelovekesh Sep 14, 2023
6d8a722
Fix typo in inline comments
thelovekesh Sep 14, 2023
e8f73c5
Try increasing navigation timeout for e2e tests to 60000 ms
thelovekesh Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions includes/validation/class-amp-validation-error-taxonomy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2923,10 +2923,19 @@ public static function handle_validation_error_update( $redirect_to, $action, $t
}
}

if ( $updated_count ) {
delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
}

if ( false !== $has_pre_term_description_filter ) {
add_filter( 'pre_term_description', 'wp_filter_kses', $has_pre_term_description_filter );
}

// Bail if `$redirect_to` is passed as null.
if ( null === $redirect_to ) {
return $redirect_to;
}

$term_ids_count = count( $term_ids );
if ( 'edit.php' === $pagenow && 1 === $updated_count ) {
// Redirect to error index screen if deleting an validation error with no associated validated URLs.
Expand All @@ -2947,10 +2956,6 @@ public static function handle_validation_error_update( $redirect_to, $action, $t
);
}

if ( $updated_count ) {
delete_transient( AMP_Validated_URL_Post_Type::NEW_VALIDATION_ERROR_URLS_COUNT_TRANSIENT );
}

return $redirect_to;
}

Expand Down
8 changes: 4 additions & 4 deletions phpunit.xml.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
bootstrap="tests/php/bootstrap.php"
backupGlobals="false"
colors="true"
convertDeprecationsToExceptions="false"
convertErrorsToExceptions="false"
convertNoticesToExceptions="false"
convertWarningsToExceptions="false"
convertDeprecationsToExceptions="true"
convertErrorsToExceptions="true"
convertNoticesToExceptions="true"
convertWarningsToExceptions="true"
defaultTestSuite="default"
>
<php>
Expand Down
2 changes: 1 addition & 1 deletion src/Admin/OnboardingWizardSubmenu.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static function get_registration_action() {
*/
public function register() {
add_submenu_page(
'',
'options.php',
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

__( 'AMP Onboarding Wizard', 'amp' ),
__( 'AMP Onboarding Wizard', 'amp' ),
'manage_options',
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/config/bootstrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,8 @@ beforeAll(async () => {
await cleanUpSettings();

// Keep navigation timeout high since CI resources can be slow.
await page.setDefaultNavigationTimeout(30000);
await page.setDefaultTimeout(30000);
await page.setDefaultNavigationTimeout(60000);
await page.setDefaultTimeout(60000);
});

// eslint-disable-next-line jest/require-top-level-describe
Expand Down
2 changes: 1 addition & 1 deletion tests/php/src/Admin/OnboardingWizardSubmenuTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,6 @@ public function test_register() {

$this->instance->register();

$this->assertEquals( end( $submenu[''] )[2], 'amp-onboarding-wizard' );
$this->assertEquals( end( $submenu['options.php'] )[2], 'amp-onboarding-wizard' );
}
}
7 changes: 7 additions & 0 deletions tests/php/src/Admin/OptionsMenuTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,13 @@ public function test_render_screen_for_admin_user() {
)
);

// Set current screen to be the options menu.
set_current_screen( $this->instance->screen_handle() );

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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


ob_start();
$this->instance->render_screen();
$this->assertStringContainsString( '<div class="wrap">', ob_get_clean() );
Expand Down
26 changes: 21 additions & 5 deletions tests/php/validation/test-class-amp-validation-error-taxonomy.php
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant 2c43d0d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

What was the deprecation warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use AmpProject\AmpWP\Services;
use AmpProject\AmpWP\Tests\Helpers\HandleValidation;
use AmpProject\AmpWP\Tests\TestCase;
use AmpProject\AmpWP\Tests\Helpers\MockAdminUser;

/**
* Tests for AMP_Validation_Error_Taxonomy class.
Expand All @@ -20,6 +21,7 @@
class Test_AMP_Validation_Error_Taxonomy extends TestCase {

use HandleValidation;
use MockAdminUser;

/**
* The tested class.
Expand Down Expand Up @@ -1429,27 +1431,41 @@ static function ( $redirect_url ) use ( &$redirected_url ) {
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $incorrect_post_type );
$this->assertEquals( get_term( $error_term->term_id )->term_group, $initial_accepted_status );

// Setup admin user which have edit_posts capability.
westonruter marked this conversation as resolved.
Show resolved Hide resolved
$this->mock_admin_user();

// Build the expected URL for the redirect.
$admin_post_url = admin_url( 'post.php' );
$redirect_query_args = [
'post' => $correct_post_type,
'action' => 'edit',
'amp_actioned' => AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_ACCEPT_ACTION,
'amp_actioned_count' => 1,
];

/*
* Although the post type is correct, this should not update the post accepted status to be 'accepted'.
* There should be a warning because wp_safe_redirect() should be called at the end of the tested method.
*/
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=amp_validation_error_accept&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( get_term( $error_term->term_id )->term_group, AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS );

// When the action is to 'reject' the error, this should not update the status of the error to 'rejected'.
$_REQUEST['action'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
$_REQUEST['action'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
$redirect_query_args['amp_actioned'] = AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_REJECT_ACTION;
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=amp_validation_error_reject&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( get_term( $error_term->term_id )->term_group, AMP_Validation_Error_Taxonomy::VALIDATION_ERROR_NEW_ACCEPTED_STATUS );

// When the action is to 'delete' the error, this should delete the error.
$_REQUEST['action'] = 'delete';
$_REQUEST['action'] = 'delete';
$redirect_query_args['amp_actioned'] = 'delete';
AMP_Validation_Error_Taxonomy::handle_single_url_page_bulk_and_inline_actions( $correct_post_type );

$this->assertSame( '?action=edit&amp_actioned=delete&amp_actioned_count=1', $redirected_url );
$this->assertSame( add_query_arg( $redirect_query_args, $admin_post_url ), $redirected_url );
$this->assertEquals( null, get_term( $error_term->term_id ) );
}

Expand Down
Loading