POC: Implement activity logging functionality with dedicated logger and handler classes - #1355
POC: Implement activity logging functionality with dedicated logger and handler classes#1355SteveJonesDev wants to merge 1 commit into
Conversation
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 establishes a foundational activity logging system within the Accessibility Checker plugin. Its primary goal is to provide a detailed audit trail of significant user interactions, such as scanning posts for accessibility issues, clearing identified issues, and ignoring specific issues. This system enhances the plugin's ability to monitor and understand user behavior, offering valuable insights into how accessibility issues are managed over time. 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
|
WalkthroughThis PR introduces an activity logging system that captures accessibility checker operations (post scans, issue clearing, issue ignoring) through action hooks, centralizes them in a new handler class, and persists logs to a newly created database table via a dedicated Logger class. Changes
Sequence DiagramsequenceDiagram
participant Op as Operation<br/>(Scan/Clear/Ignore)
participant Hook as WordPress<br/>Action Hook
participant Handler as Activity_Log_Handler
participant Query as wpdb Query
participant Table as activity_log<br/>Table
Op->>Hook: do_action('edac_after_*')
Hook->>Handler: Invoke registered callback
Handler->>Query: Query post/issue metadata
Query-->>Handler: Return counts & details
Handler->>Handler: Construct localized message
Handler->>Handler: Call Logger::log()
Handler->>Query: INSERT into activity_log
Query->>Table: Write record
Table-->>Query: Confirm insert
Query-->>Handler: Return log_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
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
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@includes/classes/ActivityLog/Activity_Log_Handler.php`:
- Around line 196-204: The current message construction inside
Activity_Log_Handler builds a post-specific "Unignored %1$s in "%2$s" (%3$d
issues)" even when ignore_global === 1 and the action is a global unignore;
update the conditional that sets $message to detect ignore_global === 1 for the
disable/unignore path and use a global variant like "Unignored %1$s globally
(%3$d issues)" (omit $post_title) while still using $rule_name and count($ids)
so global operations don't reference $post_title misleadingly.
In `@includes/classes/ActivityLog/Logger.php`:
- Around line 141-145: The ORDER BY clause in ActivityLog\Logger::$query
construction uses the `created` timestamp which can produce non-deterministic
ordering for rapid inserts; change the ORDER BY expression to use the sequential
`id` column instead. Update the SQL string in the $wpdb->prepare call that
currently contains "ORDER BY created {$order}" to "ORDER BY id {$order}"
(keeping $where_sql, $order and ...$prepare_args intact) so results are
consistently ordered by id as expected by the tests.
- Around line 48-62: The code sets nullable keys in $data to null but always
appends fixed format placeholders in $format, causing nulls to be coerced to
0/empty; update the logic that builds $data and $format (the arrays named $data
and $format in the ActivityLog\Logger class) to only add 'post_id', 'issue_id',
and 'rule_type' entries (and their corresponding '%d'/'%s' formats) when the
sanitized $metadata values are !== null, so the final $wpdb->insert($table,
$data, $format) call omits those columns entirely instead of inserting 0/empty.
Ensure you adjust any downstream assumptions about array keys accordingly.
In `@tests/phpunit/includes/classes/ActivityLog/LoggerTest.php`:
- Around line 16-17: The test failures are caused by the missing activity log
table; add a setUp() method to the LoggerTest class that creates the
accessibility_checker_activity_log table schema before each test runs so
Logger::log() and get_logs() operate against a real table; locate LoggerTest and
implement setUp() to use the WordPress testing DB functions or $wpdb to create
the table with the same columns/indexes the plugin expects (and optionally add
tearDown() to drop the table after each test).
🧹 Nitpick comments (9)
admin/class-ajax.php (1)
756-784: Inconsistent hook parameter counts may cause confusion.The
edac_after_ignore_issuehook is fired with 4 parameters in the large batch path (Line 767) but only 3 parameters in the small batch path (Line 784). While the docblocks clearly document this difference, handlers registered withadd_actionmay need to be aware of the varying arity.Per the
Activity_Log_Handlercontext,init()registers with 3 accepted arguments. The 4th parameter ($object) in the large batch case would be silently ignored unless handlers explicitly request it. Consider documenting this in the handler or normalizing the hook signature by always passing the 4th parameter (ornullfor small batches).♻️ Optional: Normalize hook signature for consistency
} else { // For small batches of IDs, we can just loop through. foreach ( $ids as $id ) { // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Safe variable used for table name, caching not required for one time operation. $wpdb->query( $wpdb->prepare( 'UPDATE %i SET ignre = %d, ignre_user = %d, ignre_date = %s, ignre_comment = %s, ignre_global = %d WHERE siteid = %d and id = %d', $table_name, $ignre, $ignre_user, $ignre_date, $ignre_comment, $ignore_global, $siteid, $id ) ); } /** * Fires after ignoring issues for activity logging. * * `@since` 1.36.0 * * `@param` array $ids Array of issue IDs. * `@param` string $action The action (enable or disable). * `@param` int $ignore_global Whether this is a global ignore. + * `@param` string|null $object The object/HTML (null for small batches). */ - do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global ); + do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global, null ); }admin/class-update-database.php (2)
83-84: Consider gating table creation with a version check.The
create_activity_log_table()method is called unconditionally on everyadmin_init, which triggersdbDelta()on every admin page load. WhiledbDelta()is idempotent, it still performs table inspection queries. Consider moving this call inside the version check block or adding a separate version tracking mechanism for the activity log table.♻️ Proposed fix to gate table creation
// Run migration for selector-based unique identifiers if upgrading from older versions. if ( version_compare( $db_version, '1.0.5', '<' ) ) { $this->migrate_to_selector_based_unique_id(); } - // Create activity log table. - $this->create_activity_log_table(); + // Create activity log table (introduced in 1.0.6). + if ( version_compare( $db_version, '1.0.6', '<' ) ) { + $this->create_activity_log_table(); + } // Update database version option. update_option( 'edac_db_version', sanitize_text_field( EDAC_DB_VERSION ) ); + }
115-146: Minor:@sincetag should reference plugin version, not DB version.Per WordPress documentation standards,
@sincetags typically reference the plugin version when the feature was introduced (e.g.,1.36.0), not the database schema version (1.0.6). This helps developers understand which plugin release introduced the functionality.Additionally, if queries will filter by
issue_id, consider adding an index for it. Otherwise, the schema and indexes look appropriate for the activity logging use case.♻️ Proposed changes
/** * Create activity log table. * - * `@since` 1.0.6 + * `@since` 1.36.0 * `@return` void */ private function create_activity_log_table() {If
issue_idqueries are expected:KEY post_id_index (post_id), + KEY issue_id_index (issue_id), KEY siteid_index (siteid)tests/phpunit/includes/classes/ActivityLog/LoggerTest.php (5)
22-26: Fix equals sign alignment per WPCS.Static analysis reports alignment issues on these lines.
♻️ Proposed fix
public function test_log_activity() { - $action = 'post_scan'; - $message = 'Test scan activity'; + $action = 'post_scan'; + $message = 'Test scan activity'; $metadata = [ 'post_id' => 1, ];
38-44: Fix equals sign alignment per WPCS.♻️ Proposed fix
public function test_log_activity_with_full_metadata() { - $action = 'ignore_issue'; - $message = 'Test ignore activity'; + $action = 'ignore_issue'; + $message = 'Test ignore activity'; $metadata = [ 'post_id' => 1, 'issue_id' => 42,
77-77: Multi-item associative arrays must have each value on a new line per WPCS.♻️ Proposed fix
- $logs = Logger::get_logs( [ 'action' => 'post_scan', 'limit' => 10 ] ); + $logs = Logger::get_logs( + [ + 'action' => 'post_scan', + 'limit' => 10, + ] + );
94-94: Multi-item associative arrays must have each value on a new line per WPCS.♻️ Proposed fix
- $logs = Logger::get_logs( [ 'post_id' => 1, 'limit' => 10 ] ); + $logs = Logger::get_logs( + [ + 'post_id' => 1, + 'limit' => 10, + ] + );
105-119: Avoidsleep()in tests; ordering by ID is deterministic.Using
sleep(1)slows down the test suite unnecessarily. Since theLogger::get_logs()method orders bycreatedtimestamp but IDs are sequential, consider ordering byidin the Logger class, or simply rely on the fact that sequential inserts will have sequential timestamps without needing a delay.Also, fix the array formatting per WPCS on lines 112 and 117.
♻️ Proposed fix
public function test_get_logs_ordering() { // Create test log entries. $id1 = Logger::log( 'post_scan', 'First', [ 'post_id' => 1 ] ); - sleep( 1 ); $id2 = Logger::log( 'post_scan', 'Second', [ 'post_id' => 1 ] ); // Test DESC ordering (default). - $logs = Logger::get_logs( [ 'limit' => 2, 'order' => 'DESC' ] ); + $logs = Logger::get_logs( + [ + 'limit' => 2, + 'order' => 'DESC', + ] + ); $this->assertEquals( $id2, (int) $logs[0]['id'] ); $this->assertEquals( $id1, (int) $logs[1]['id'] ); // Test ASC ordering. - $logs = Logger::get_logs( [ 'limit' => 2, 'order' => 'ASC' ] ); + $logs = Logger::get_logs( + [ + 'limit' => 2, + 'order' => 'ASC', + ] + ); $this->assertEquals( $id1, (int) $logs[0]['id'] ); $this->assertEquals( $id2, (int) $logs[1]['id'] ); }includes/classes/ActivityLog/Activity_Log_Handler.php (1)
206-217: Logging the same message for each issue ID creates redundant entries.The loop logs identical messages for each issue in
$ids, which creates database bloat for batch operations. Consider logging a single entry for the batch operation instead.♻️ Proposed fix: Log once per batch operation
- // Log each issue individually. - foreach ( $ids as $id ) { - Logger::log( - 'ignore_issue', - $message, - [ - 'post_id' => $issue['postid'], - 'issue_id' => $id, - 'rule_type' => $issue['ruletype'], - ] - ); - } + // Log once for the batch operation. + Logger::log( + 'ignore_issue', + $message, + [ + 'post_id' => $issue['postid'], + 'issue_id' => $ids[0], + 'rule_type' => $issue['ruletype'], + ] + );If individual tracking per issue is required for audit purposes, consider adding a batch ID or storing issue IDs as JSON in the message.
| } else { | ||
| $message = sprintf( | ||
| /* translators: %1$s: Rule name, %2$s: Post title, %3$d: Number of issues. */ | ||
| __( 'Unignored %1$s in "%2$s" (%3$d issues)', 'accessibility-checker' ), | ||
| $rule_name, | ||
| $post_title, | ||
| count( $ids ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Missing global unignore message variant.
The global ignore case (ignore_global === 1) only handles the enable action. When globally unignoring (disable with ignore_global === 1), the message still includes $post_title which may be misleading for a global operation.
🛠️ Proposed fix
} else {
- $message = sprintf(
- /* translators: %1$s: Rule name, %2$s: Post title, %3$d: Number of issues. */
- __( 'Unignored %1$s in "%2$s" (%3$d issues)', 'accessibility-checker' ),
- $rule_name,
- $post_title,
- count( $ids )
- );
+ if ( 1 === $ignore_global ) {
+ $message = sprintf(
+ /* translators: %1$s: Rule name, %2$d: Number of issues. */
+ __( 'Globally unignored %1$s (%2$d issues)', 'accessibility-checker' ),
+ $rule_name,
+ count( $ids )
+ );
+ } else {
+ $message = sprintf(
+ /* translators: %1$s: Rule name, %2$s: Post title, %3$d: Number of issues. */
+ __( 'Unignored %1$s in "%2$s" (%3$d issues)', 'accessibility-checker' ),
+ $rule_name,
+ $post_title,
+ count( $ids )
+ );
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else { | |
| $message = sprintf( | |
| /* translators: %1$s: Rule name, %2$s: Post title, %3$d: Number of issues. */ | |
| __( 'Unignored %1$s in "%2$s" (%3$d issues)', 'accessibility-checker' ), | |
| $rule_name, | |
| $post_title, | |
| count( $ids ) | |
| ); | |
| } | |
| } else { | |
| if ( 1 === $ignore_global ) { | |
| $message = sprintf( | |
| /* translators: %1$s: Rule name, %2$d: Number of issues. */ | |
| __( 'Globally unignored %1$s (%2$d issues)', 'accessibility-checker' ), | |
| $rule_name, | |
| count( $ids ) | |
| ); | |
| } else { | |
| $message = sprintf( | |
| /* translators: %1$s: Rule name, %2$s: Post title, %3$d: Number of issues. */ | |
| __( 'Unignored %1$s in "%2$s" (%3$d issues)', 'accessibility-checker' ), | |
| $rule_name, | |
| $post_title, | |
| count( $ids ) | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@includes/classes/ActivityLog/Activity_Log_Handler.php` around lines 196 -
204, The current message construction inside Activity_Log_Handler builds a
post-specific "Unignored %1$s in "%2$s" (%3$d issues)" even when ignore_global
=== 1 and the action is a global unignore; update the conditional that sets
$message to detect ignore_global === 1 for the disable/unignore path and use a
global variant like "Unignored %1$s globally (%3$d issues)" (omit $post_title)
while still using $rule_name and count($ids) so global operations don't
reference $post_title misleadingly.
| 'post_id' => isset( $metadata['post_id'] ) ? absint( $metadata['post_id'] ) : null, | ||
| 'issue_id' => isset( $metadata['issue_id'] ) ? absint( $metadata['issue_id'] ) : null, | ||
| 'rule_type' => isset( $metadata['rule_type'] ) ? sanitize_text_field( $metadata['rule_type'] ) : null, | ||
| ]; | ||
|
|
||
| $format = [ | ||
| '%d', // user_id. | ||
| '%s', // created. | ||
| '%s', // action. | ||
| '%s', // message. | ||
| '%d', // siteid. | ||
| '%d', // post_id. | ||
| '%d', // issue_id. | ||
| '%s', // rule_type. | ||
| ]; |
There was a problem hiding this comment.
Using %d format for nullable fields inserts 0 instead of NULL.
When post_id, issue_id, or rule_type are not provided, you set them to null, but then use %d (integer) or %s (string) formats which will convert null to 0 or empty string. This loses the semantic distinction between "not applicable" (NULL) and "zero/empty".
🛠️ Proposed fix: Conditionally include nullable fields
public static function log( string $action, string $message, array $metadata = [] ) {
global $wpdb;
$table_name = $wpdb->prefix . 'accessibility_checker_activity_log';
$user_id = get_current_user_id();
$siteid = get_current_blog_id();
// Prepare data for insertion.
$data = [
'user_id' => $user_id,
'created' => current_time( 'mysql', true ),
'action' => sanitize_text_field( $action ),
'message' => sanitize_text_field( $message ),
'siteid' => $siteid,
- 'post_id' => isset( $metadata['post_id'] ) ? absint( $metadata['post_id'] ) : null,
- 'issue_id' => isset( $metadata['issue_id'] ) ? absint( $metadata['issue_id'] ) : null,
- 'rule_type' => isset( $metadata['rule_type'] ) ? sanitize_text_field( $metadata['rule_type'] ) : null,
];
- $format = [
- '%d', // user_id.
- '%s', // created.
- '%s', // action.
- '%s', // message.
- '%d', // siteid.
- '%d', // post_id.
- '%d', // issue_id.
- '%s', // rule_type.
- ];
+ $format = [ '%d', '%s', '%s', '%s', '%d' ];
+
+ // Add optional fields only if provided.
+ if ( isset( $metadata['post_id'] ) ) {
+ $data['post_id'] = absint( $metadata['post_id'] );
+ $format[] = '%d';
+ }
+
+ if ( isset( $metadata['issue_id'] ) ) {
+ $data['issue_id'] = absint( $metadata['issue_id'] );
+ $format[] = '%d';
+ }
+
+ if ( isset( $metadata['rule_type'] ) ) {
+ $data['rule_type'] = sanitize_text_field( $metadata['rule_type'] );
+ $format[] = '%s';
+ }
// phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery -- Logging to custom table.
$result = $wpdb->insert( $table_name, $data, $format );🤖 Prompt for AI Agents
In `@includes/classes/ActivityLog/Logger.php` around lines 48 - 62, The code sets
nullable keys in $data to null but always appends fixed format placeholders in
$format, causing nulls to be coerced to 0/empty; update the logic that builds
$data and $format (the arrays named $data and $format in the ActivityLog\Logger
class) to only add 'post_id', 'issue_id', and 'rule_type' entries (and their
corresponding '%d'/'%s' formats) when the sanitized $metadata values are !==
null, so the final $wpdb->insert($table, $data, $format) call omits those
columns entirely instead of inserting 0/empty. Ensure you adjust any downstream
assumptions about array keys accordingly.
| $query = $wpdb->prepare( | ||
| // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table identifier is safe, WHERE clause is prepared. | ||
| "SELECT * FROM %i WHERE {$where_sql} ORDER BY created {$order} LIMIT %d OFFSET %d", | ||
| ...$prepare_args | ||
| ); |
There was a problem hiding this comment.
Ordering by created timestamp may produce inconsistent results for rapid inserts.
The query orders by created but tests assert ordering by id. If multiple entries are created within the same second, their order becomes undefined. Consider ordering by id instead, which is guaranteed to be sequential.
🛠️ Proposed fix
- "SELECT * FROM %i WHERE {$where_sql} ORDER BY created {$order} LIMIT %d OFFSET %d",
+ "SELECT * FROM %i WHERE {$where_sql} ORDER BY id {$order} LIMIT %d OFFSET %d",🤖 Prompt for AI Agents
In `@includes/classes/ActivityLog/Logger.php` around lines 141 - 145, The ORDER BY
clause in ActivityLog\Logger::$query construction uses the `created` timestamp
which can produce non-deterministic ordering for rapid inserts; change the ORDER
BY expression to use the sequential `id` column instead. Update the SQL string
in the $wpdb->prepare call that currently contains "ORDER BY created {$order}"
to "ORDER BY id {$order}" (keeping $where_sql, $order and ...$prepare_args
intact) so results are consistently ordered by id as expected by the tests.
| class LoggerTest extends WP_UnitTestCase { | ||
|
|
There was a problem hiding this comment.
Tests fail because the activity log table doesn't exist in the test environment.
The pipeline failures show Logger::log() returns false and get_logs() returns empty arrays. This indicates the accessibility_checker_activity_log table is not being created before tests run. You need a setUp() method to create the table schema.
🛠️ Proposed fix: Add setUp method to create the table
class LoggerTest extends WP_UnitTestCase {
+ /**
+ * Set up the test environment.
+ */
+ public function set_up(): void {
+ parent::set_up();
+ $this->create_activity_log_table();
+ }
+
+ /**
+ * Create the activity log table for testing.
+ */
+ private function create_activity_log_table(): void {
+ global $wpdb;
+
+ $table_name = $wpdb->prefix . 'accessibility_checker_activity_log';
+ $charset_collate = $wpdb->get_charset_collate();
+
+ $sql = "CREATE TABLE IF NOT EXISTS {$table_name} (
+ id bigint(20) unsigned NOT NULL AUTO_INCREMENT,
+ user_id bigint(20) unsigned NOT NULL,
+ created datetime NOT NULL,
+ action varchar(50) NOT NULL,
+ message text NOT NULL,
+ siteid bigint(20) unsigned NOT NULL,
+ post_id bigint(20) unsigned DEFAULT NULL,
+ issue_id bigint(20) unsigned DEFAULT NULL,
+ rule_type varchar(50) DEFAULT NULL,
+ PRIMARY KEY (id),
+ KEY siteid (siteid),
+ KEY action (action),
+ KEY post_id (post_id)
+ ) {$charset_collate};";
+
+ require_once ABSPATH . 'wp-admin/includes/upgrade.php';
+ dbDelta( $sql );
+ }
+
/**
* Test logging an activity.
*/🤖 Prompt for AI Agents
In `@tests/phpunit/includes/classes/ActivityLog/LoggerTest.php` around lines 16 -
17, The test failures are caused by the missing activity log table; add a
setUp() method to the LoggerTest class that creates the
accessibility_checker_activity_log table schema before each test runs so
Logger::log() and get_logs() operate against a real table; locate LoggerTest and
implement setUp() to use the WordPress testing DB functions or $wpdb to create
the table with the same columns/indexes the plugin expects (and optionally add
tearDown() to drop the table after each test).
![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 introduces a robust activity logging system, including new database tables, logger, and handler classes, along with comprehensive unit tests. The changes effectively integrate activity tracking for post scans, issue clearing, and issue ignoring. However, there are some inconsistencies in the parameters passed to action hooks and received by their respective handlers, which should be addressed to ensure robust and predictable behavior.
| * @param int $ignore_global Whether this is a global ignore. | ||
| * @param string $object The object/HTML that was ignored. | ||
| */ | ||
| do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global, $object ); |
There was a problem hiding this comment.
The edac_after_ignore_issue action is fired with four parameters ($ids, $action, $ignore_global, $object) when a large batch operation is performed. However, the Activity_Log_Handler::log_ignore_issue method, which hooks into this action, is only configured to receive three parameters. This inconsistency can lead to unexpected behavior or warnings if the handler is updated to expect the $object parameter in all cases, or if strict argument checking is enabled. Please ensure the action hook and its corresponding handler function have matching parameter counts and types.
do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global );| * @since 1.36.0 | ||
| */ | ||
| public function init() { | ||
| add_action( 'edac_after_post_scan', [ $this, 'log_post_scan' ], 10, 2 ); |
There was a problem hiding this comment.
The edac_after_post_scan action is registered to expect 2 arguments (10, 2), but the log_post_scan method only defines one parameter ($post_id). The second parameter, $post, which is passed from class-rest-api.php, will be ignored. Please update the log_post_scan method signature to accept the $post object if it's intended to be used, or adjust the add_action call if only $post_id is ever needed.
add_action( 'edac_after_post_scan', [ $this, 'log_post_scan' ], 10, 2 );| * | ||
| * @param int $post_id The post ID. | ||
| */ | ||
| public function log_post_scan( int $post_id ) { |
There was a problem hiding this comment.
The log_post_scan method is hooked to edac_after_post_scan which passes two arguments ($post_id, $post). However, this method only accepts $post_id. The $post object is currently ignored. If the $post object is not needed, please update the add_action call in the init method to reflect that only one argument is expected. If it is needed, please update the method signature to accept it.
public function log_post_scan( int $post_id, $post = null ) {| public function init() { | ||
| add_action( 'edac_after_post_scan', [ $this, 'log_post_scan' ], 10, 2 ); | ||
| add_action( 'edac_before_clear_issues', [ $this, 'log_clear_issues' ], 10, 1 ); | ||
| add_action( 'edac_after_ignore_issue', [ $this, 'log_ignore_issue' ], 10, 3 ); |
There was a problem hiding this comment.
The edac_after_ignore_issue action is registered to expect 3 arguments (10, 3). However, the batch operation of this action (in admin/class-ajax.php) passes 4 arguments. This mismatch can lead to unexpected behavior or warnings. Please ensure the add_action call correctly reflects the maximum number of arguments passed to the hook, or adjust the action calls to be consistent.
add_action( 'edac_after_ignore_issue', [ $this, 'log_ignore_issue' ], 10, 4 );| * @param string $action The action (enable or disable). | ||
| * @param int $ignore_global Whether this is a global ignore. | ||
| */ | ||
| public function log_ignore_issue( array $ids, string $action, int $ignore_global ) { |
There was a problem hiding this comment.
The log_ignore_issue method is hooked to edac_after_ignore_issue. While it expects 3 arguments, the batch operation of this action (in admin/class-ajax.php) passes 4 arguments, including an $object parameter. This method should be updated to accept the fourth parameter to avoid potential issues if the action is called with it. Even if not used, accepting it makes the handler more robust.
public function log_ignore_issue( array $ids, string $action, int $ignore_global, string $object = '' ) {| * @param int $post_id The post ID. | ||
| * @param object $post The post object. | ||
| */ | ||
| do_action( 'edac_after_post_scan', $post_id, $post ); |
There was a problem hiding this comment.
The edac_after_post_scan action is fired with two parameters: $post_id and $post. However, the Activity_Log_Handler::log_post_scan method, which hooks into this action, only accepts $post_id. The $post object is effectively ignored. This is a mismatch in the action hook signature and the handler's expected parameters. Please ensure the handler method is updated to accept the $post object if it's intended to be used, or adjust the add_action call if only $post_id is ever needed.
do_action( 'edac_after_post_scan', $post_id, $post );| * @param string $action The action (enable or disable). | ||
| * @param int $ignore_global Whether this is a global ignore. | ||
| */ | ||
| do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global ); |
There was a problem hiding this comment.
The edac_after_ignore_issue action is fired with three parameters ($ids, $action, $ignore_global) for single operations. This is inconsistent with the batch operation (lines 757-767) which passes an additional $object parameter. For better maintainability and predictability, it's recommended to have a consistent signature for the same action hook, even if some parameters might be null or empty in certain contexts. Alternatively, consider using two distinct action hooks if the context and parameters are fundamentally different.
do_action( 'edac_after_ignore_issue', $ids, $action, $ignore_global, null );| [ $limit, $offset ] | ||
| ); | ||
|
|
||
| // phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber -- Using spread operator for dynamic number of replacements. |
There was a problem hiding this comment.
The phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber comment is used here. While the spread operator (...$prepare_args) is a valid way to handle dynamic arguments, it's good practice to ensure that the number of placeholders in the SQL query string matches the number of arguments provided to wpdb->prepare(). In this case, the WHERE clause is dynamic, so the number of %d and %s placeholders can vary. This ignore comment is acceptable given the dynamic nature of the query construction, but it's worth noting that careful testing is required to prevent SQL injection vulnerabilities when constructing dynamic queries.
| // phpcs:ignore WordPress.DB.PreparedSQLPlaceholders.ReplacementsWrongNumber -- Using spread operator for dynamic number of replacements. | ||
| $query = $wpdb->prepare( | ||
| // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table identifier is safe, WHERE clause is prepared. | ||
| "SELECT * FROM %i WHERE {$where_sql} ORDER BY created {$order} LIMIT %d OFFSET %d", |
There was a problem hiding this comment.
The phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared comment is used for $table_name and $where_sql. While $table_name is generally safe as it's derived from $wpdb->prefix and a hardcoded string, $where_sql is constructed dynamically. Although the individual clauses are prepared, the implode operation itself doesn't re-prepare the entire string. This approach is generally safe if all components of $where_sql are already sanitized or prepared, but it's a point to be aware of for security and maintainability.
![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: 305262cf9d
ℹ️ 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".
| // Initialize activity log handler globally (works in all contexts). | ||
| $activity_log_handler = new Activity_Log_Handler(); | ||
| $activity_log_handler->init(); |
There was a problem hiding this comment.
Because the activity log handler is initialized for every request, Logger::log() can run in non-admin contexts (e.g., REST scans) before Update_Database::edac_update_database() has run on admin_init. On a fresh install or immediately after update, this means the accessibility_checker_activity_log table may not exist yet, so logging attempts will error and silently drop entries; consider creating the table earlier (e.g., on activation) or adding a table-existence guard before logging.
Useful? React with 👍 / 👎.
1 participant
This pull request introduces a comprehensive activity logging system to the Accessibility Checker plugin, enabling detailed tracking of key user actions such as scanning posts, clearing issues, and ignoring issues. The system includes database schema changes, new logging classes, integration with plugin actions, and automated tests to ensure reliability.
Activity Logging System Implementation:
Loggerclass for recording activities into a dedicatedaccessibility_checker_activity_logdatabase table, and for retrieving log entries with various filters.Activity_Log_Handlerclass that hooks into plugin actions (edac_after_post_scan,edac_before_clear_issues,edac_after_ignore_issue) to automatically log relevant events.Database and Action Hook Enhancements:
accessibility_checker_activity_log) for storing activity logs, with appropriate indices for efficient querying. The table is created during the database update routine. [1] [2]1.0.6to reflect the schema change.edac_after_post_scan,edac_before_clear_issues,edac_after_ignore_issue) at strategic points in the codebase to trigger activity logging after scans, before clearing issues, and after ignoring issues. [1] [2] [3]Testing:
Loggerclass to verify activity logging, retrieval, filtering, and ordering.Summary by CodeRabbit
Release Notes
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.