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

cleanup at uninstall issue #21

Closed futtta closed 5 years ago

futtta commented 5 years ago

Great to have the notice dismissal status being strored in options iso transients (asked some people to test for whom transients did not work) and nice you thought of cleanup code to remove PAnD-options, but ... assuming a use case where 2 or more plugins are installed that use PAnD, this approach deletes the PAnD-options of all plugins, not only of the one being uninstalled?

So what if we save the option as

$cache_key = 'pand-' .md5(<calling plugin slug>) . '-' . md5( $id );

And delete only the pand-options for the plugin being uninstalled.

Question is if we can auto-populate or if we would need that to be passed as an option (which is ... tedious)?

frank

afragen commented 5 years ago

Yes, you have identified a potential issue. Unfortunately trying to individually remove the options is much more difficult. The only drawback is that the user will see a notice one more time that they must dismiss.

afragen commented 5 years ago

We can only hope that everyone using PAnD is cleaning up after themselves. But they likely aren’t. Think of this as a service to everyone else using PAnD with a minimal cost. 😉

futtta commented 5 years ago

hmmm ... I would prefer not getting complaints of users seeing my notice(s) because someone else deleted all PAnD options ;-)

Maybe we could we use something like

debug_backtrace(2,1)[0]['file'] which automagically provides us with calling plugin's filename including path (did some tests and seems to work), from which we could extract the slug and then indeed add this to $cache_key as

$cache_key = 'pand-' .md5(<calling plugin slug>) . '-' . md5( $id ); and then only delete

%pand-' .md5(<this plugin slug>) . '-%' from the DB?

afragen commented 5 years ago

Personally I think the added complexity outweighs the half second of user interaction.

I'm happy to evaluate a PR and see what @collizo4sky has to say.

w3guy commented 5 years ago

It’s just one row in option table. I will prefer we do not recommend deleting the option table after unistall.

futtta commented 5 years ago

-> "complexity"; agreed. I'm somewhat wary of using debug_backtrace, no idea if is can/ should be used like this in a production-setting -> "outweighs half second user interaction"; problem is we're impacting other plugin's notices, so we're affecting user interactions of other plugins. -> "not recommending deleting from the option table" indeed is a possibility as well, but not wild about that either to be honest

w3guy commented 5 years ago

@afragen shall we now remove the uninstall section from readme?

afragen commented 5 years ago

@collizo4sky perhaps a warning about the potential interaction with others in addition to the uninstall section? I have no problem with removing the uninstall section. I added it to be complete.

afragen commented 5 years ago

@futtta I don’t believe you can actually pass anything to the uninstall.php file as the plugin would be deactivated and it runs on delete.

w3guy commented 5 years ago

I am in favor of removing it altogether with a warning not to remove it on uninstall for the sake of other plugins using it.

Fine with everyone?

afragen commented 5 years ago

If we don't mention how to remove it, do we need the notice about removing it?

w3guy commented 5 years ago

Just to warn those that may have the intention of doing it.

On Mon, Nov 5, 2018 at 5:45 PM Andy Fragen notifications@github.com wrote:

If we don't mention how to remove it, do we need the notice about removing it?

— 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/21#issuecomment-435945954, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc6oCaC7qz3nXzLRYdiJk436rto4zks5usGsqgaJpZM4YNau5 .

afragen commented 5 years ago

@collizo4sky would you like me to submit the PR?

w3guy commented 5 years ago

Yes please 🙏🏻

On Mon, 5 Nov 2018 at 7:20 PM, Andy Fragen notifications@github.com wrote:

@collizo4sky https://github.com/collizo4sky would you like me to submit the PR?

— 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/21#issuecomment-435979482, or mute the thread https://github.com/notifications/unsubscribe-auth/AD8uc023_I_deH_wKPTCDZ_JNGdJ6aOuks5usIF6gaJpZM4YNau5 .

afragen commented 5 years ago

Already done. #22

w3guy commented 5 years ago

Merged.