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

Improve detection of missing AS tables #5440

Closed
4 tasks
piotrbak opened this issue Sep 16, 2022 · 8 comments · Fixed by #5441
Closed
4 tasks

Improve detection of missing AS tables #5440

piotrbak opened this issue Sep 16, 2022 · 8 comments · Fixed by #5441
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: preload module: remove unused css priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Milestone

Comments

@piotrbak
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
In the current approach we have a bug related to the not expected characters in the database name. Our query here is not using `` causing fail of the check and displaying false message:

$wpdb->prepare( 'SHOW TABLES FROM ' . DB_NAME . ' WHERE Tables_in_' . DB_NAME . ' LIKE %s AND Tables_in_' . DB_NAME . ' REGEXP ' . $exp, '%actionscheduler%' )

Expected behavior
We should check the existence of the tables only in the specific conditions:

  1. When activating the plugin, before Preload do any operation on the tables
  2. When enabling RUCSS
  3. When enabling Preload
  4. During each update of the plugin

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: bug Indicates an unexpected problem or unintended behavior module: preload priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available module: remove unused css labels Sep 16, 2022
@piotrbak piotrbak added this to the 3.12.1.1 milestone Sep 16, 2022
@piotrbak
Copy link
Contributor Author

This will resolve the following issue:
#5426

@Tabrisrp
Copy link
Contributor

Are we removing the existing notice?

@piotrbak
Copy link
Contributor Author

Yes, the notice won't be needed anymore.

@jeawhanlee jeawhanlee self-assigned this Sep 16, 2022
@jeawhanlee jeawhanlee added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Sep 16, 2022
@jeawhanlee
Copy link
Contributor

Scope a solution ✅

I propose creating a new class and subscriber in https://github.com/wp-media/wp-rocket/tree/develop/inc/Engine/Admin

use ActionScheduler_StoreSchema;
use ActionScheduler_LoggerSchema;

public function maybe_recreate_as_tables() {
	if ( is_valid_as_tables() ) {
           return;
        }

	$store_schema  = new ActionScheduler_StoreSchema();
	$logger_schema = new ActionScheduler_LoggerSchema();
	$store_schema->register_tables( true );
	$logger_schema->register_tables( true );
}

private function is_valid_as_tables() {
        $cached_count = get_transient( 'rocket_rucss_as_tables_count' );
	if ( false !== $cached_count && ! is_admin() ) { // Stop caching in admin UI.
		return 4 === (int) $cached_count;
	}

	global $wpdb;

	$exp = "'^" . $wpdb->prefix . "actionscheduler_(logs|actions|groups|claims)$'";
	// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching
	$found_as_tables = $wpdb->get_col(
		// phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
		$wpdb->prepare( 'SHOW TABLES FROM `' . DB_NAME . '` WHERE `Tables_in_' . DB_NAME . '` LIKE %s AND `Tables_in_' . DB_NAME . '` REGEXP ' . $exp, '%actionscheduler%' )
	);

	set_transient( 'rocket_rucss_as_tables_count', count( $found_as_tables ), rocket_get_constant( 'DAY_IN_SECONDS', 24 * 60 * 60 ) );

	return 4 === count( $found_as_tables );
}

When activating the plugin, before Preload do any operation on the tables

In the new subscriber hook a new method to rocket_activation that calls maybe_recreate_as_tables()

  • remove
    use ActionScheduler_StoreSchema;
    use ActionScheduler_LoggerSchema;
    and
    public function is_valid_as_tables() {
    global $wpdb;
    $exp = "'^" . $wpdb->prefix . "actionscheduler_(logs|actions|groups|claims)$'";
    // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery,WordPress.DB.DirectDatabaseQuery.NoCaching
    $found_as_tables = $wpdb->get_col(
    // phpcs:ignore WordPress.DB.PreparedSQL.NotPrepared
    $wpdb->prepare( 'SHOW TABLES FROM ' . DB_NAME . ' WHERE Tables_in_' . DB_NAME . ' LIKE %s AND Tables_in_' . DB_NAME . ' REGEXP ' . $exp, '%actionscheduler%' )
    );
    return 4 === count( $found_as_tables );
    }

When enabling RUCSS

in new subscriber hook a new method to
$slug = rocket_get_constant( 'WP_ROCKET_SLUG', 'wp_rocket_settings' );
'update_option_' . $slug and check if RUCSS is enabled then call maybe_recreate_as_tables()

When enabling Preload

in new subscriber hook a new method to
$slug = rocket_get_constant( 'WP_ROCKET_SLUG', 'wp_rocket_settings' );
'update_option_' . $slug and check if preload is enabled then call maybe_recreate_as_tables()

During each update of the plugin

in new subscriber hook a new method to wp_rocket_upgrade that calls maybe_recreate_as_tables()

Estimate the effort ✅

[S]

Can you validate this grooming @Tabrisrp 🙏

@jeawhanlee jeawhanlee added effort: [S] 1-2 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Sep 16, 2022
@Tabrisrp
Copy link
Contributor

that sounds good to me 👍🏼

@camilamadronero-zz
Copy link

Mai-Saad pushed a commit that referenced this issue Sep 21, 2022
* remove notices classes
* remove AS tables check
* remove AS missing table notice method
* remove notices classes instantiation
* add action scheduler check class
* fix PHPCS
* remove old tests
* move fixture
* add is_valid_as_tables() test
* move fixtures
* update test
* update preload activate test
* add test for check_on_update_options
* update return types and add more conditions to check_on_update_options()
* add AS check
* add AS check to activation provider
* implement activation interface
* fix PHPCS
Co-authored-by: Michael Lee <38788055+jeawhanlee@users.noreply.github.com>
* move subscriber later
* remove usage of interface
* resolve conflicts
Co-authored-by: Michael Lee <38788055+jeawhanlee@users.noreply.github.com>
Co-authored-by: Ahmed Saeed <eng.ahmeds3ed@gmail.com>
@Tabrisrp Tabrisrp mentioned this issue Sep 21, 2022
@worldwildweb
Copy link
Contributor

@vmanthos
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: preload module: remove unused css priority: high Issues which should be resolved as quickly as possible severity: major Feature is not working as expected and no work around available type: bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants