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

Create the database tables if they do not exist when resetting the DB #1285

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

tomjn
Copy link
Contributor

@tomjn tomjn commented Sep 16, 2021

Create the database tables if they do not exist when resetting the database

Fixes #1284

If the plugin is loaded without activation, tables are never created and resetting the database does not create tables. This PR changes that so that if tables are missing, a reset creates them.

Checklist

  • Project documentation has been updated to reflect the changes in this pull request, if applicable.
  • I have tested the changes in the local development environment (see contributing.md).
  • I have added phpunit tests.

Release Changelog

  • Fix: Describe a bug fix included in this release.
  • New: Describe a new feature in this release.

Release Checklist

  • This pull request is to the master branch.
  • Release version follows semantic versioning. Does it include breaking changes?
  • Update changelog in readme.txt.
  • Bump version in stream.php.
  • Bump Stable tag in readme.txt.
  • Bump version in classes/class-plugin.php.
  • Draft a release on GitHub.

Copy link
Contributor

@kasparsd kasparsd left a comment

Choose a reason for hiding this comment

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

@tomjn Would this still be necessary if we merge #1286 which creates the tables when the admin loads?

@tomjn
Copy link
Contributor Author

tomjn commented Sep 17, 2021

@kasparsd I don't believe it's strictly necessary, but it is nice and makes sense that a DB reset would make sure the tables are present and up to date. This is explicit where the other PR is implicit.

@lkraav
Copy link
Contributor

lkraav commented Sep 17, 2021

I don't believe it's strictly necessary, but it is nice and makes sense that a DB reset would make sure the tables are present and up to date. This is explicit where the other PR is implicit.

I've been wondering if x+1 plugins with custom tables all do their "TABLE EXISTS" checks, does it have any real performance impact or not.

In the big picture, it does seem kind of redundant to guard against a real far-edge case millions of times / year * # of plugins.

@tomjn
Copy link
Contributor Author

tomjn commented Sep 17, 2021

@lkraav for this PR it'll happen when a user clicks a button, as for WP Admin or CLI it's pretty safe. On the frontend it can add up quickly on a busy site

@@ -673,6 +673,9 @@ public function wp_ajax_reset() {
esc_html__( "You don't have sufficient privileges to do this action.", 'stream' )
);
}

// Create and update the database tables if they do not exist.
$this->plugin->install->check();
Copy link
Contributor

@kasparsd kasparsd Sep 17, 2021

Choose a reason for hiding this comment

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

Is this check here defined( 'DOING_AJAX' ) && DOING_AJAX going to stop this particular call from doing anything?

if ( defined( 'DOING_AJAX' ) && DOING_AJAX ) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏻 I wouldn't have noticed without your help :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think $this->plugin->install->install( $this->plugin->get_version() ); might make more sense?

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

Successfully merging this pull request may close these issues.

Stream does not check database tables exist
3 participants