Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses multiple QA issues identified in the OneAccess plugin. The changes include fixing the hook used for user deduplication to ensure user metadata is available, updating the uninstall script to use the correct placeholder for table names in prepared statements, and refactoring role/table creation from deferred execution to immediate execution during plugin initialization.
Key changes:
- Changed user deduplication hook from
user_registertoprofile_updatewith priority 99 to ensure user metadata is set - Updated
uninstall.phpto use%iidentifier placeholder instead of%sstring placeholder for table names in DROP TABLE queries - Modified error messages in REST API responses to differentiate between success and partial failure scenarios
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| uninstall.php | Fixed indentation and corrected the wpdb::prepare() placeholder from %s to %i for table identifiers in DROP TABLE queries |
| inc/Modules/Rest/Governing_Site_Controller.php | Updated error messages for user operations, removed URL normalization in some places, fixed array casting, and removed debug response data from error logs |
| inc/Modules/Core/User_Roles.php | Changed role creation from deferred plugins_loaded hooks to immediate execution during register_hooks() |
| inc/Modules/Core/Hooks.php | Changed user deduplication hook from user_register to profile_update with priority 99 and added documentation explaining the rationale |
| inc/Modules/Core/DB.php | Changed table creation from deferred plugins_loaded hook to immediate execution during register_hooks() |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach ( $sites as $site_url ) { | ||
| $url = untrailingslashit( $site_url['url'] ?? '' ); | ||
| if ( ! isset( $oneaccess_sites_info[ $url ] ) ) { | ||
| if ( ! isset( $oneaccess_sites_info[ $site_url['url'] ] ) ) { |
There was a problem hiding this comment.
What was the reason for this change?
Not sure why this isn't being picked out by either linters, but isset( $x[ $y['z'] ] ) doesn't check internals, so at minimum you want to ! isset( $site_url['url'], $oneaccess_sites_info[ $site_url['url'] ] ) (depending on the internals ofc)
| $site = $oneaccess_sites_info[ $site_key ] ?? []; | ||
| $site_url = $site['url'] ?? ''; | ||
| $site = (array) ( $oneaccess_sites_info[ $site_key ] ?? [] ); | ||
| $site_url = trailingslashit( $site['url'] ?? '' ); |
There was a problem hiding this comment.
You want to trailing-slash the empty space or should it ! empty( $site['url'] ) ? trailingslashit(...) : ''?
|
|
||
| foreach ( $sites as $site ) { | ||
| $site_url = $site['url'] ?? ''; | ||
| $site_url = untrailingslashit( $site['url'] ?? '' ); |
justlevine
left a comment
justlevine
left a comment
There was a problem hiding this comment.
Other than the isset() this LGTM.
( Beyond the scope of this PR/QA, but if this comes up elsewhere, i'd much rather us enforce a single and uniform {un}trailingslashit at a single point of truth (our db setters/getters, a REST args schema for the sites array) + right before a check where it matters. Right now it's hard for someone who isn't the codeowner to tell when a url is indented to be trailingslashed at any specific point in the codebase. )
In general I would like to add |
What
user_registertoprofile_updateas userdata was not available. https://developer.wordpress.org/reference/hooks/user_register/#more-informationuninstall.phpfile format and issue of table deletionRelated Issue(s):
Part of https://github.com/rtCamp/OnePress/issues/58
Checklist