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.

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

Output admin notice if persistent object caching is not present #1050

Merged
merged 3 commits into from Apr 3, 2018

Conversation

Projects
None yet
3 participants
@oscarssanchez
Copy link
Contributor

oscarssanchez commented Mar 30, 2018

Hi @westonruter,

Could you use this PR which addresses #960 ?

I was not sure where it was best to write the method in, so I opted to write it in class-amp-validation-utils but i'm open to your suggestions.

I'm also not sure if the output message would be good as it is now. I opted to include a link to https://codex.wordpress.org/Class_Reference/WP_Object_Cache#Persistent_Caching since users might not know what persistent object cache is and how to enable it.

git

Fixes #960.

@oscarssanchez oscarssanchez force-pushed the oscarssanchez:add/960-persistent-object-notice branch from b20723d to 4f2f8dd Mar 30, 2018

@kienstra

This comment has been minimized.

Copy link

kienstra commented on includes/utils/class-amp-validation-utils.php in 59e15fb Mar 31, 2018

This looks good, @oscarssanchez. How about using a single printf() for this line and the one above?

This comment has been minimized.

Copy link

kienstra replied Mar 31, 2018

It's really nice how you have a link for more details. That link might not need to be on a separate line, though. There's an example of a notice in AMP_Validation_Utils::plugin_notice().

This comment has been minimized.

Copy link

kienstra replied Mar 31, 2018

'More details' should be internationalized.

@kienstra

This comment has been minimized.

It's nice having $using here, as it's easy to see what you're testing. But it's probably not needed. It'd be cleaner to just pass the value straight to wp_using_ext_object_cache().

@kienstra

This comment has been minimized.

This should probably go in the tearDown(), right above parent::tearDown().

@kienstra

This comment has been minimized.

How about naming this persistent_object_caching_notice, as it relates to object caching?

@kienstra

This comment has been minimized.

These nested conditionals could be on one line.

This comment has been minimized.

Copy link

kienstra replied Mar 31, 2018

You might be able to just use get_current_screen()->id, instead of $screen->id. There's something similar in AMP_Validation_Utils::remaining_error_notice().

@kienstra

This comment has been minimized.

$needle makes sense, in that you're searching for this. But a name like $text or $notice might be more descriptive.

@oscarssanchez oscarssanchez reopened this Mar 31, 2018

@oscarssanchez oscarssanchez force-pushed the oscarssanchez:add/960-persistent-object-notice branch from ca4b925 to 59e15fb Mar 31, 2018

@oscarssanchez

This comment has been minimized.

Copy link
Contributor

oscarssanchez commented Mar 31, 2018

Thanks for your feedback @kienstra .

I made the corresponding changes.

@westonruter westonruter added this to the v1.0 milestone Apr 2, 2018

esc_html__( 'The AMP plugin performs at its best when persistent object cache is enabled.', 'amp' ),
esc_url( 'https://codex.wordpress.org/Class_Reference/WP_Object_Cache#Persistent_Caching' ),
esc_html__( 'More details', 'amp' ),
esc_html__( 'Dismiss this notice.', 'amp' )

This comment has been minimized.

@westonruter

westonruter Apr 3, 2018

Member

I think we should eliminate the dismissal because as soon as the page reload the notice comes right back.

This comment has been minimized.

@westonruter

westonruter Apr 3, 2018

Member

@oscarssanchez once this change is made the PR will be good to merge.

@oscarssanchez

This comment has been minimized.

Copy link
Contributor

oscarssanchez commented Apr 3, 2018

Hi @westonruter,

Thanks for your comments, I just did the changes.

@westonruter westonruter merged commit 2bcfc19 into ampproject:develop Apr 3, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment