Add inbox item to try setting up db again#2263
Conversation
|
Warning Rate limit exceeded@Crabcyborg has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces new functionality for handling missing database tables and improving error reporting. A new condition in the admin controller now triggers a missing tables recovery flow via a static method in the forms controller. Additionally, error detection has been enhanced by instantiating an inbox checker in form listings and adding dedicated methods in both the inbox and migration models to verify table existence and report issues. The XML import process also now requires a specific flag to proceed. Changes
Sequence Diagram(s)sequenceDiagram
participant UA as User/Admin
participant AC as FrmAppController
participant FC as FrmFormsController
participant DB as Database
UA->>AC: Load admin page with frm_add_tables parameter
AC->>AC: Check if page is 'formidable' and parameter present
AC->>FC: Call add_missing_tables()
FC->>FC: Verify permissions and table creation error state
alt Tables Missing
FC->>DB: Attempt to create missing tables
DB-->>FC: Return table creation status
FC->>UA: Redirect or update status based on result
else
FC->>UA: Proceed without table creation
end
sequenceDiagram
participant Up as FrmMigrate
participant DB as Database
participant FI as FrmInbox
Up->>DB: Execute SQL query to check tables
alt Tables Exist
Up->>FI: Dismiss table error message
else
Up->>FI: Add error message with documentation link
end
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
classes/controllers/FrmFormsController.php (3)
3304-3304: Update the @SInCE version tag.The @SInCE tag mentions "x.x" which appears to be a placeholder that needs to be updated with the actual version number where this functionality is being introduced.
- * @since x.x + * @since 6.x
3297-3327: Consider adding a nonce check for the table recovery process.The table recovery process performs sensitive operations but lacks a nonce check to verify the request origin.
A nonce check would provide additional security for this operation. Consider adding one like this:
public static function add_missing_tables() { FrmAppHelper::permission_check( 'frm_view_forms' ); + // Verify nonce + check_admin_referer( 'frm_fix_tables_nonce', 'frm_fix_tables' ); $inbox = new FrmInbox(); $error = $inbox->check_for_error();
3317-3318: Improve database table check logic.The current code only checks for the existence of the forms table, but there might be other required tables that could also be missing.
Consider using a more comprehensive check for all required tables or let FrmMigrate handle the table existence check:
-global $wpdb; -$exists = $wpdb->get_results( "SHOW TABLES LIKE '$wpdb->prefix . 'frm_forms''" ); -if ( $exists ) { +$migrator = new FrmMigrate(); +if ( $migrator->table_exists('frm_forms') ) { // Exit early if the table already exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmAppController.php(1 hunks)classes/controllers/FrmFormsController.php(2 hunks)classes/helpers/FrmXMLHelper.php(1 hunks)classes/models/FrmInbox.php(1 hunks)classes/models/FrmMigrate.php(2 hunks)
🔇 Additional comments (6)
classes/controllers/FrmAppController.php (1)
569-571: Good addition for database table recoveryThis code nicely implements a recovery path for missing database tables. When a user visits the Formidable admin page with the
frm_add_tablesparameter, it will attempt to recreate any missing tables through theFrmFormsController::add_missing_tables()method.classes/models/FrmInbox.php (1)
520-536: New error checking method looks goodThis method provides a clean way to check for error messages in the inbox, which will be used to display critical issues on the form list page.
Consider updating the
@since x.xtag with the actual version number before final release.classes/models/FrmMigrate.php (2)
45-45: Good integration of table existence checkAdding this check after creating tables and migrating data is an appropriate placement to ensure proper error reporting.
77-98: Well-structured table existence verificationThis method provides a robust way to check for missing database tables and notify users through the inbox system. The error message includes helpful documentation links and a clear path to recovery.
Some observations:
- The error message is clear and actionable
- The "click here" link properly includes the
frm_add_tablesparameter to trigger the recovery- The method properly dismisses any existing error message if the tables exist
Consider updating the
@since x.xtag with the actual version number before final release.classes/helpers/FrmXMLHelper.php (1)
937-937: Good defensive programming to prevent action creation with incomplete data.Adding this check ensures that form actions are only created when
$imported['form_status']exists, preventing potential issues when importing XML content with database table problems. This aligns well with the PR's goal of improving database table recovery functionality.classes/controllers/FrmFormsController.php (1)
1080-1085: Added inbox error checking.This addition enhances error handling in the form listing page, where errors detected by the FrmInbox system will be displayed to the user.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/models/FrmMigrate.php (2)
72-76: Update version placeholder in documentation.The
@since x.xshould be updated with the actual version number before release.- * @since x.x + * @since 6.x.x
77-98: Consider optimizing the FrmInbox instantiation.The code instantiates the
FrmInboxclass twice. It would be more efficient to instantiate it once at the beginning of the method.public function check_that_tables_exist() { // Check the DB that the table $this->forms exists. global $wpdb; + $inbox = new FrmInbox(); $exists = $wpdb->get_results( $wpdb->prepare( "SHOW TABLES LIKE %s", $this->forms ) ); if ( $exists ) { - $inbox = new FrmInbox(); $inbox->dismiss( 'failed-to-create-tables' ); return; } $message = array( 'key' => 'failed-to-create-tables', 'subject' => 'Something went wrong setting up the database', 'message' => 'For steps to continue, see our <a href="https://formidableforms.com/knowledgebase/install-formidable-forms/#kb-missing-database-tables">documentation</a>. If you need assistance, we recommend that you reach out to your hosting provider. Then <a href="' . esc_url( admin_url( 'admin.php?page=formidable&frm_add_tables=1' ) ) . '">click here</a> to try again.', 'cta' => '<a href="https://formidableforms.com/knowledgebase/install-formidable-forms/#kb-missing-database-tables">Learn More</a>', 'type' => 'error', ); - $inbox = new FrmInbox(); $inbox->add_message( $message ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
classes/models/FrmMigrate.php(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: PHP 8 tests in WP trunk
- GitHub Check: PHP 7.4 tests in WP trunk
- GitHub Check: Cypress
🔇 Additional comments (2)
classes/models/FrmMigrate.php (2)
45-45: Good addition to the upgrade process.Adding a database table verification step after creation attempts is a good defensive programming practice. This ensures that if tables fail to create, users get actionable feedback.
100-104: Documentation improvement for collation method.Good addition of PHPDoc to the existing
collation()method, improving code readability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
classes/controllers/FrmAppController.php (1)
1390-1421: Comprehensive implementation for handling missing database tables.The implementation properly handles permissions, checks if the error notification exists, verifies if tables already exist to prevent redundant operations, and triggers the necessary database setup.
A few minor suggestions:
- Replace the
x.xplaceholder in the version documentation with the actual version number:-/** - * @since x.x + /** + * @since 6.x
- Consider adding a more descriptive comment about what this method does and why it's needed:
-// Check the DB that the table $this->forms exists. +// Check if the forms table exists. If not, we'll attempt a recovery process +// by clearing the DB version, which will trigger a fresh table creation.classes/models/FrmMigrate.php (1)
77-98: Well-implemented error detection and reporting mechanism.This method effectively checks for the existence of database tables and either dismisses stale error messages or adds helpful guidance when tables are missing.
Suggestions for improvements:
- Update the version placeholder in the documentation:
-/** - * @since x.x + /** + * @since 6.x
- Consider adding a more informative error type to the message array. While "error" correctly identifies the severity, having a more specific identifier might help with programmatic filtering later:
- 'type' => 'error', + 'type' => 'error', + 'error_type' => 'database_tables_missing',
- Add a comment explaining the importance of checking if the tables exist after the migration process has completed, as this helps explain the purpose of this method to other developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmAppController.php(2 hunks)classes/controllers/FrmFormsController.php(1 hunks)classes/models/FrmMigrate.php(2 hunks)
🔇 Additional comments (3)
classes/controllers/FrmFormsController.php (1)
1080-1084: Good addition for database error detection.This code introduces error checking through the FrmInbox class, allowing the system to detect potential database table issues and display them in the forms list. This improves system diagnostics and helps administrators identify missing tables early.
classes/controllers/FrmAppController.php (1)
569-571: Good addition of a retry mechanism for table creation.This adds a feature to handle missing database tables by allowing users to retry the table creation process from an inbox notification.
classes/models/FrmMigrate.php (1)
45-45: Good placement for table verification.This call to
check_that_tables_exist()is strategically placed after the tables creation and data migration steps, ensuring the system can detect and handle cases where tables failed to create properly.
1 participant
Fixes https://github.com/Strategy11/formidable-pro/issues/5157
Related Slack conversation https://strategy11.slack.com/archives/CJFQ599V5/p1740769963601729?thread_ts=1722554756.049169&cid=CJFQ599V5