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.

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

Allow DROP TABLE schema change in uninstall.php #2359

Open
Asgaros opened this issue Aug 18, 2023 · 6 comments
Open

Allow DROP TABLE schema change in uninstall.php #2359

Asgaros opened this issue Aug 18, 2023 · 6 comments

Comments

@Asgaros
Copy link

Asgaros commented Aug 18, 2023

Plugins often require their own tables - therefore those tables naturally should be removed during the uninstall process of the plugin. This brings the requirement to execute a query like DROP TABLE IF EXISTS wp_my_table; which is placed in the uninstall.php file.

Currently, the CodeSniffer creates warnings for those database queries, like:

Attempting a database schema change is discouraged. (WordPress.DB.DirectDatabaseQuery.SchemaChange)

Since deleting unnecessary data during the uninstall process is a proper way to avoid leaving trash behind, I suggest to add an exception which allows the DROP TABLE query within the uninstall.php file.

@dingo-d
Copy link
Member

dingo-d commented Aug 18, 2023

You can just silence the direct database query sniffs in your plugin's ruleset

<rule ref="WordPress.DB.DirectDatabaseQuery.SchemaChange">
	<severity>0</severity>
</rule>

I recommend you read about modifying your ruleset, and about customizing WPCS ruleset.

@Asgaros
Copy link
Author

Asgaros commented Aug 18, 2023

Yes, I know that. Basically, I can silence every annoying warning, so at the end my code will be perfectly fine.

My point is more: This ruleset makes perfectly sense in general, so I want to keep and test code against it to ensure no schema changes are happening in the main plugin files. But at the same time, it should be a good idea to allow this kind of query during the uninstall process because it is an essential part of cleaning up and therefore should not trigger a warning there.

@dingo-d
Copy link
Member

dingo-d commented Aug 18, 2023

@jrfnl @GaryJones Could we add this exclusion to the sniff? I mean, the way I'd approach this would be to just add an exclusion for this one file in my ruleset and that'd be it.

But uninstall.php is a plugin-specific file, so it could be used as a special case.

@dingo-d dingo-d reopened this Aug 18, 2023
@GaryJones
Copy link
Member

I think checking if the file name is uninstall.php and exiting early makes sense. We already have file-specific conditions for other things (like for template files).

@smileBeda

This comment was marked as off-topic.

@jrfnl

This comment was marked as off-topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants