search

w3guy / persist-admin-notices-dismissal

Simple plugin that persists dismissal of admin notices across pages in WordPress dashboard.
http://w3guy.com/wordpress-admin-notices-dismissible/
87 stars 22 forks source link

Issue with Transient failing to save on some server #16

Closed w3guy closed 5 years ago

w3guy commented 5 years ago

See https://wordpress.org/support/topic/delete-notice-check-out-the-autoptimize-extra-settings-to-activate-this-option/page/3/#post-10778748

Any idea how to fix this @afragen ??

afragen commented 5 years ago

@collizo4sky from what I can tell, this is where PaND is called.

https://github.com/futtta/autoptimize/blob/0a841c72be5b95656d91e2660986aeac58fb6337/classes/autoptimizeMain.php#L541=L561

I don’t know of any inherent value for which set_site_transient doesn’t work for the $expiration, but the expiration is set for 123 days or 10627200 seconds. The transient key is limited to 191 characters in the database.

Here is where the admin_notices is called.

https://github.com/futtta/autoptimize/blob/0a841c72be5b95656d91e2660986aeac58fb6337/classes/autoptimizeMain.php#L181-L190

Seems like the user could add add_filter( 'autoptimize_filter_extra_activate', ‘__return_false’ ); and the notice would never show. But that doesn’t explain what might be going on.

I’ve found that certain caches memcache, Redis, etc. can clear all the transients at unexpected times. I’ve seen this before. The only way around this is to store the value in the options table instead of as a transient. The problem, as I see it, is then there is no way to remove the value from the options table should the plugin be removed. Transients were made for this.

Without installing and debugging the plugin I can’t tell for sure what the issue may be. Below is the commit that included the admin notice. https://github.com/futtta/autoptimize/commit/31df9d34ca6897b5ce372fe2e022b010a0504563#diff-1afe934307c9c6d17e6bfd36eb1ce875

I would dig into the conditional. I’m not sure why it needs to have many of those checks.

afragen commented 5 years ago

It might be that the following hook is being called before PaND initializes. Setting the priority lower may work. Just throwing out ideas.

https://github.com/futtta/autoptimize/blob/0a841c72be5b95656d91e2660986aeac58fb6337/classes/autoptimizeMain.php#L57

afragen commented 5 years ago

It seems that the setup containing the notice loading is called during plugins_loaded and PaND is loaded in admin_init. This might be the issue. Calling maybe_run_ao_extra in admin_init might fix it.

I’m not sure what calling PaND in plugins_loaded would do, $user is not initialized there.

afragen commented 5 years ago

I installed the plugin on my Siteground multisite test server and the notice seems to be staying dismissed. ¯_(ツ)_/¯

This with version 2.4.1

w3guy commented 5 years ago

It seems to work excellently well for most users of Autoptimize. Also worked perfectly for me on my test.

This is not a bug in PanD. I'll close this issue.

I advice the developer over Facebook chat to add a filter to dismiss the notice for any user that might have trouble dismissing the notice.

futtta commented 5 years ago

I tend to agree this is probably not a bug in PanD (indeed the issue seems not to happen in all or even most cases and I have never seen it fail myself), but it would be nice to know what the problem is.

If this were an issue with priorities, it would be a problem for all, no?

Some other pointers;

futtta commented 5 years ago

@afragen re. all the checks; i only want the notice to show if the user is not optimizing images already, if he has acces to the feature (we're launching gradually) and if the notice has not been dismissed yet.

w3guy commented 5 years ago

@futtta Regarding the expire int being big, I wanted him to increase memcache item size so we are sure this is the issue.

w3guy commented 5 years ago

So what do your suggest as a solution @futtta??

I think adding a filter to disable the notice via code could eventually be the solution going forward incase anyone using the lib come across this issue.

Thought?

afragen commented 5 years ago

@futtta I was simply brainstorming ideas for why it might be failing. It actually works as expected for me.

@collizo4sky I don’t think memcache size is the issue. The data stored by the transient is minimal.

afragen commented 5 years ago

Doesn’t the filter I referenced above already do this?

w3guy commented 5 years ago

I was pretty sure the issue is not memcache size related that was why I wanted him to test with the size increased.

Doesn’t the filter I referenced above already do this?

I don't know. Maybe @futtta would be able to answer that.

futtta commented 5 years ago

that filter would hide the entire "extra" tab, I would (as a last resort) add a filter to disable the notice.

On Sun, Oct 14, 2018 at 8:50 PM Andy Fragen notifications@github.com wrote:

Doesn’t the filter I referenced above already do this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/issues/16#issuecomment-429651708, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMeQ5F_dQ9T3McLwsJko5dM_CgT0Bks5uk4eAgaJpZM4XbHVX .

afragen commented 5 years ago

I believe the issue is related to a specific user’s object caching. There can clearly be a lot of variability even among users on the same host.

w3guy commented 5 years ago

This issue is officially considered closed.

Adding a filter as an alternative for user having problem dismissing the notice very much look like the only solution.

futtta commented 5 years ago

probably yeah, although webfactory's test results with expiry int length are ... weird.

On Sun, Oct 14, 2018 at 8:52 PM Andy Fragen notifications@github.com wrote:

I believe the issue is related to a specific user’s object caching. There can clearly be a lot of variability even among users on the same host.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/issues/16#issuecomment-429651813, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMT3IopjuZgEDFXozSzTufNJ-q1H6ks5uk4fVgaJpZM4XbHVX .

w3guy commented 5 years ago

Weird indeed..

futtta commented 5 years ago

I now use ao-img-opt-plug-123, if I need a transient with expiration 0 I make that ao-img-opt-plug-0, but how is that different from ao-img-opt-plug-forever?

afragen commented 5 years ago

0 is automatically converted to 1.

https://github.com/collizo4sky/persist-admin-notices-dismissal/blob/b80673631d14f10af9747283d08434619232c003/persist-admin-notices-dismissal.php#L85

ao-img-opt-plug-0 == ao-img-opt-plug-1

futtta commented 5 years ago

one being expiry of 1 second, vs forever becoming 0 so no expiry set?

On Sun, Oct 14, 2018 at 9:18 PM Andy Fragen notifications@github.com wrote:

0 is automatically converted to 1.

https://github.com/collizo4sky/persist-admin-notices-dismissal/blob/b80673631d14f10af9747283d08434619232c003/persist-admin-notices-dismissal.php#L85

ao-img-opt-plug-0 == ao-img-opt-plug-1

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/collizo4sky/persist-admin-notices-dismissal/issues/16#issuecomment-429654022, or mute the thread https://github.com/notifications/unsubscribe-auth/AALEMeZWjiRO9VsQlvfkB-H4rIQrnU1Hks5uk44PgaJpZM4XbHVX .

afragen commented 5 years ago

@futtta no, the unit is days. The minimum amount of a dismissal is 1 day.

I seem to recall that numbers between 0 and 1 can work for less than a day, but I could be wrong. I’m wrong.