Fix: update database schema to use selector as unique identifier for issues - #1324
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR changes the unique identifier for accessibility checker rules from the "object" field to the "selector" field in database queries, introduces a database migration to backfill existing records with legacy selector values, updates the database version constant, and refines REST API violation filtering logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Summary of ChangesHello @SteveJonesDev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the accuracy of accessibility issue tracking by transitioning the database's unique identification mechanism from relying on the generic 'object' field to the more precise 'selector' field. This change addresses the limitation where identical code elements in different page locations were not properly distinguished. The update includes a necessary database migration to ensure backward compatibility, comprehensive code adjustments across data insertion and batch processing, and new tests to validate the system's improved ability to uniquely identify and manage issues based on their specific DOM location. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a solid improvement that correctly changes the unique identifier for accessibility issues from object to selector. This change is crucial for accurately tracking distinct issues that might share the same code snippet but appear in different locations. The implementation is thorough, including a database migration for legacy data, corresponding logic updates for data handling, and a new unit test to confirm the fix. I've identified one critical issue regarding a potential race condition that could lead to data duplication.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4301e7496
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
admin/class-insert-rule-data.php (2)
157-157: Selector sanitization uses null coalescing but may store empty strings.Line 157 sanitizes the selector field with
sanitize_text_field( $rule_data['selector'] ?? '' ), which means if$rule_data['selector']is null, it becomes an empty string''.This creates an inconsistency with the migration logic in
admin/class-update-database.php(line 106), which treats bothNULLand empty strings ('') as missing selectors. Storing empty strings for new records could trigger the migration logic to run on them later, which may not be the intended behavior.Consider using
nullas the fallback instead of an empty string, or add validation earlier in the process as suggested in the previous comment.Suggested fix to maintain consistency
- 'selector' => sanitize_text_field( $rule_data['selector'] ?? '' ), + 'selector' => ! empty( $rule_data['selector'] ) ? sanitize_text_field( $rule_data['selector'] ) : null,
84-98: Address NULL selector handling in duplicate detection query.When
$selectorsis empty (the default parameter value),$rule_data['selector']becomesnull. The SELECT query at line 89 will then useselector = NULL, but there's an inconsistency: the INSERT sanitization on line 157 converts thisnullto an empty string''viasanitize_text_field($rule_data['selector'] ?? ''). This creates a mismatch where duplicates may not be properly detected.The test
testRuleInserterReturnLogic()exercises this path by callinginsert()twice without providing selectors, expecting the second call to returnnull(duplicate detection). The deprecated functionedac_insert_rule_data()also callsinsert()without selectors, meaning this edge case can occur in production.While the REST API properly populates selectors from violation data, the method's default behavior leaves it vulnerable to NULL/empty string inconsistencies. Either validate that a selector is provided before proceeding, or generate a fallback identifier as suggested in the original comment to ensure duplicate detection works reliably.
🧹 Nitpick comments (1)
admin/class-update-database.php (1)
87-110: Migration logic is sound but lacks observability.The migration correctly backfills missing selectors with unique
legacy-id-{id}values to ensure backward compatibility. The SQL logic properly handles NULL and empty strings.Consider adding basic observability to track migration success:
Suggested improvement for observability
private function migrate_to_selector_based_unique_id() { global $wpdb; $table_name = $wpdb->prefix . 'accessibility_checker'; // Find records with NULL or empty selectors and update them with a fallback value. // Using the record ID ensures each record has a unique selector for backward compatibility. // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- One-time migration query. - $wpdb->query( + $rows_affected = $wpdb->query( $wpdb->prepare( "UPDATE %i SET selector = CONCAT('legacy-id-', id) WHERE selector IS NULL OR selector = ''", $table_name ) ); + + // Log migration result for debugging purposes. + if ( defined( 'EDAC_DEBUG' ) && EDAC_DEBUG ) { + error_log( sprintf( 'EDAC: Migrated %d records to selector-based unique identifiers.', $rows_affected ) ); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
accessibility-checker.phpadmin/class-ajax.phpadmin/class-insert-rule-data.phpadmin/class-update-database.phptests/phpunit/Admin/InsertRuleDataTest.php
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.php: Follow WordPress Coding Standards (WPCS) in all PHP files
Class names use CamelCase (ClassNameConvention) for new classes
Use edac_ prefix for all custom action/filter hook names
Ensure PHP 7.4+ compatibility
Use type hints where appropriate (parameters, return types, properties)
Sanitize inputs, validate data, and escape outputs following WordPress security best practices; use nonces for forms/AJAX
Use the WordPress database API ($wpdb) for all database operations
Prefix functions and classes in the global namespace with edac_
Use WordPress transients for caching temporary data where appropriate
All user-facing text in PHP must be translatable using the accessibility-checker text domain
Use PHPDoc for all public classes, methods, and properties
Document all custom hooks (actions/filters) with docblocks including parameters and types
Files:
admin/class-update-database.phptests/phpunit/Admin/InsertRuleDataTest.phpaccessibility-checker.phpadmin/class-insert-rule-data.phpadmin/class-ajax.php
admin/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place admin classes and admin-only PHP code in the /admin directory
Files:
admin/class-update-database.phpadmin/class-insert-rule-data.phpadmin/class-ajax.php
**/class-*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/class-*.php: Legacy PHP class files must use WordPress style naming class-class-name.php
Legacy class names use WordPress underscore style (Class_Name_Convention)
Files:
admin/class-update-database.phpadmin/class-insert-rule-data.phpadmin/class-ajax.php
tests/phpunit/**/*.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place and write PHPUnit tests under /tests/phpunit
Files:
tests/phpunit/Admin/InsertRuleDataTest.php
accessibility-checker.php
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Implement proper plugin activation and deactivation hooks
Files:
accessibility-checker.php
🧠 Learnings (1)
📚 Learning: 2025-08-26T11:25:48.236Z
Learnt from: pattonwebz
Repo: equalizedigital/accessibility-checker PR: 1208
File: includes/options-page.php:401-410
Timestamp: 2025-08-26T11:25:48.236Z
Learning: In the Accessibility Checker plugin, the edac_is_pro() function uses a robust detection pattern by checking both EDACP_VERSION (which comes from the pro plugin) and EDAC_KEY_VALID existence before looking at its value. This prevents PHP notices and ensures reliable pro feature detection.
Applied to files:
accessibility-checker.php
🧬 Code graph analysis (2)
tests/phpunit/Admin/InsertRuleDataTest.php (1)
admin/class-insert-rule-data.php (2)
Insert_Rule_Data(25-179)insert(43-178)
admin/class-ajax.php (2)
src/admin/index.js (2)
ids(288-288)rule(377-378)src/pageScanner/index.js (2)
selector(131-131)selector(379-379)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Integration Test: PHP 8.2 | WP latest (+ ms)
- GitHub Check: Integration Test: PHP 8.1 | WP latest (+ ms)
🔇 Additional comments (5)
accessibility-checker.php (1)
44-44: LGTM! Version bump aligns with schema changes.The database version increment to
1.0.5correctly reflects the schema change from object-based to selector-based unique identifiers.admin/class-update-database.php (1)
77-80: Migration trigger correctly scoped to version upgrades.The version check ensures the migration runs only when upgrading from versions prior to 1.0.5, preventing redundant execution.
admin/class-insert-rule-data.php (2)
15-22: Excellent documentation of schema change rationale.The docblock clearly explains the transition from object-based to selector-based unique identifiers and the business reason (allowing duplicate code objects at different page locations).
110-129: UPDATE query correctly uses selector in WHERE clause.The query properly updates existing records by matching on
selectorinstead ofobject, which aligns with the new unique identifier scheme.tests/phpunit/Admin/InsertRuleDataTest.php (1)
87-132: Excellent test coverage for selector-based uniqueness.The test effectively validates the core functionality change:
- Duplicate objects with different selectors are stored as separate issues ✓
- Same selector+object combinations are detected as duplicates ✓
- Row count assertions verify database state ✓
The existing test suite already covers the edge case of null/empty selectors. The
testRuleInserterReturnLogic()method tests inserting issues without providing selectors (which defaults to null), and duplicate detection works correctly in this scenario. The migration inadmin/class-update-database.phphandles legacy records with null selectors by assigning unique legacy-based identifiers during upgrade.
…ique This is no longer required as we have a new method of determining unique items. You can test it by pasting 2 or more empty paragraph tags on a page and seeing them both be stored. Removing this code also ensures a new test that was added is actually validating the real stored values
The changes here didn't allow large batch ignores to work - those need to continue working on object, not selector
2 participants
This pull request updates the way unique issues are identified in the accessibility checker database. Instead of using the
objectfield as part of the unique identifier, the system now uses theselectorfield. This allows the plugin to distinguish between duplicate code objects (such as multiple empty paragraphs) that appear in different locations on a page. The update includes a database migration, code changes to useselectorfor lookups and updates, and new tests to verify the improved behavior.Database schema and migration:
postid + rule + object + type + siteidtopostid + rule + selector + type + siteid, allowing duplicate code objects in different locations to be stored as separate issues.class-update-database.phpto update existing records, assigning a fallback selector to legacy records that lack a selector.1.0.5inaccessibility-checker.php.Code changes for selector-based identification:
class-insert-rule-data.phpto useselectorinstead ofobjectfor selecting and updating records, ensuring correct handling of duplicate objects with different selectors. [1] [2]class-ajax.phpto useselectorandruleas identifiers instead ofobject, for more accurate updates when handling large batches.Testing improvements:
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.