Skip to content

"Install::verify_db()" removed. #1201

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.

Sign up for GitHub

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

Jump to bottom
Merged
kidunot89 merged 3 commits into from
Jan 20, 2021
Merged

"Install::verify_db()" removed. #1201

kidunot89 merged 3 commits into from
Jan 20, 2021

Conversation

@kidunot89
Copy link
Contributor

@kidunot89 kidunot89 commented Nov 27, 2020 •
edited
Loading

Fixes #1169

Describe your approach and how it fixes the issue.

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).
  • Removes the verify_db() from the WP_Stream\Install class.

@kidunot89 kidunot89 force-pushed the bugfix/remove-unnecessary-db-calls branch from cea575e to 3516cb8 Compare November 27, 2020 17:19
kasparsd
kasparsd approved these changes Dec 3, 2020
View reviewed changes
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.

This looks good!

I guess it was added originally to prevent the plugin from failing silently if the custom tables were not there. WP core should now deactivate the plugin automatically if it causes a PHP error so activating it again will run the installation steps.

kasparsd
kasparsd reviewed Dec 3, 2020
View reviewed changes
classes/class-install.php Outdated
add_action( 'init', array( $this, 'verify_db' ) );

// Install the plugin.
add_action( 'wp_stream_before_db_notices', array( $this, 'check' ) );
Copy link
Contributor

@kasparsd kasparsd Dec 3, 2020

Choose a reason for hiding this comment

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

I think this can also be removed since we're removing the wp_stream_before_db_notices call already.

Copy link
Contributor

@kasparsd kasparsd Jan 4, 2021

Choose a reason for hiding this comment

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

@kidunot89 Can we remove this now that the do_action( 'wp_stream_before_db_notices' ) is being removed?

@kidunot89 kidunot89 added this to the 3.6.1 milestone Jan 4, 2021
@kidunot89 kidunot89 force-pushed the bugfix/remove-unnecessary-db-calls branch from 3516cb8 to 9b88a07 Compare January 5, 2021 22:33
kidunot89
kidunot89 commented Jan 5, 2021
View reviewed changes
classes/class-install.php

// Install the plugin.
add_action( 'wp_stream_before_db_notices', array( $this, 'check' ) );

Copy link
Contributor Author

@kidunot89 kidunot89 Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@kasparsd Removing this may have broken CI. The tests are still passing locally tho. Any ideas what's going on with Travis-CI?

kasparsd
kasparsd reviewed Jan 11, 2021
View reviewed changes
classes/class-install.php
$this->stream_url = self_admin_url( $this->plugin->admin->admin_parent_page . '&page=' . $this->plugin->admin->settings_page_slug );

// Check DB and display an admin notice if there are tables missing.
add_action( 'init', array( $this, 'verify_db' ) );
Copy link
Contributor

@kasparsd kasparsd Jan 11, 2021

Choose a reason for hiding this comment

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

@kidunot89 I think removing this now makes the Travis checks fail because the table for the plugin is not created on the first run https://travis-ci.com/github/xwp/stream/jobs/468947527#L511

@kidunot89 kidunot89 force-pushed the bugfix/remove-unnecessary-db-calls branch from 9b88a07 to 0afc790 Compare January 16, 2021 01:58
@kidunot89 kidunot89 requested a review from kasparsd January 16, 2021 02:09
kidunot89
kidunot89 commented Jan 16, 2021
View reviewed changes
kasparsd
kasparsd reviewed Jan 19, 2021
View reviewed changes
@kidunot89 kidunot89 force-pushed the bugfix/remove-unnecessary-db-calls branch 2 times, most recently from c3a8ad7 to e4de12d Compare January 19, 2021 21:23
@kidunot89 kidunot89 force-pushed the bugfix/remove-unnecessary-db-calls branch from c37f168 to c98fad9 Compare January 20, 2021 19:02
@kidunot89 kidunot89 merged commit 21696dc into develop Jan 20, 2021
@kidunot89 kidunot89 deleted the bugfix/remove-unnecessary-db-calls branch January 20, 2021 20:47
@kidunot89 kidunot89 mentioned this pull request Jan 20, 2021
Merged
10 tasks
@kasparsd kasparsd mentioned this pull request Sep 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivankruchkoff ivankruchkoff Awaiting requested review from ivankruchkoff

1 more reviewer

@kasparsd kasparsd kasparsd approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.6.1

Development

Successfully merging this pull request may close these issues.

Excessive DB queries in admin

2 participants

@kidunot89 @kasparsd