Fix: Theme ZIP install debug warnings in MainWP Child Reports / Child#17
Open
Fix: Theme ZIP install debug warnings in MainWP Child Reports / Child#17
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughNormalize and skip null meta values before WPDB meta insertion; add theme-slug normalization and safer log-arg construction in the Connector Installer, updating theme-slug extraction sites to use the normalizer and removing empty Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
connectors/class-connector-installer.php (2)
662-668: Pre-existing issue: undefined$successand$errorvariables.The
compact()call at line 664 references$successand$error, but these variables are not defined inautomatic_updates_complete_plugin_theme(). This triggers PHP warnings in 7.3+. Since the PR is refactoring this method, consider fixing this as well.🔧 Suggested fix to define missing variables
if ( ! empty( $type ) ) { $context = $type . 's'; $name = isset( $log['name'] ) ? $log['name'] : null; $version = isset( $log['version'] ) ? $log['version'] : null; $slug = isset( $log['slug'] ) ? $log['slug'] : null; $old_version = isset( $log['old_version'] ) ? $log['old_version'] : null; $message = isset( $log['message'] ) ? $log['message'] : null; $action = isset( $log['action'] ) ? $log['action'] : null; + $success = true; // Auto-updates in this method are already validated as successful + $error = null; $this->log( $message,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/class-connector-installer.php` around lines 662 - 668, The compact() call passed to $this->log references undefined $success and $error in automatic_updates_complete_plugin_theme(), causing PHP warnings; either initialize those variables (e.g., set $success = false; $error = null; or populate them from the function's update result/error variables used earlier) before the $this->log(...) call, or remove 'success' and 'error' from the compact() list so only defined variables (type, name, version, slug, old_version) are passed.
565-581: Consider adding a guard for emptyold_version.If
callback_pre_auto_updatefails to execute for any reason,$old_versionwould be an empty string, resulting in a log message like "WordPress auto-updated from to 6.5". While WordPress core should always firepre_auto_updatebeforeautomatic_updates_complete, a defensive check could improve robustness.🔧 Optional defensive check
$old_version = $this->current_wordpress_version; $new_version = $info->item->version; $auto_updated = true; - $message = esc_html__( 'WordPress auto-updated from %1$s to %2$s', 'mainwp-child-reports' ); + if ( ! empty( $old_version ) ) { + $message = esc_html__( 'WordPress auto-updated from %1$s to %2$s', 'mainwp-child-reports' ); + } else { + $message = esc_html__( 'WordPress auto-updated to %1$s', 'mainwp-child-reports' ); + $old_version = $new_version; // Use new_version as placeholder for %1$s + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/class-connector-installer.php` around lines 565 - 581, The log can show an empty "from" when $this->current_wordpress_version was never set; before creating $old_version (and before calling $this->log), add a guard that checks $this->current_wordpress_version and either (a) set $old_version to a sensible placeholder like 'unknown' when empty or (b) skip/abort the log entirely if old version is missing. Update the code paths around $old_version, $new_version and the $this->log call (referencing $this->current_wordpress_version, $old_version and the log invocation) so the resulting message never reads "from to X" and consistently conveys a valid source version or omits the log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@connectors/class-connector-installer.php`:
- Around line 662-668: The compact() call passed to $this->log references
undefined $success and $error in automatic_updates_complete_plugin_theme(),
causing PHP warnings; either initialize those variables (e.g., set $success =
false; $error = null; or populate them from the function's update result/error
variables used earlier) before the $this->log(...) call, or remove 'success' and
'error' from the compact() list so only defined variables (type, name, version,
slug, old_version) are passed.
- Around line 565-581: The log can show an empty "from" when
$this->current_wordpress_version was never set; before creating $old_version
(and before calling $this->log), add a guard that checks
$this->current_wordpress_version and either (a) set $old_version to a sensible
placeholder like 'unknown' when empty or (b) skip/abort the log entirely if old
version is missing. Update the code paths around $old_version, $new_version and
the $this->log call (referencing $this->current_wordpress_version, $old_version
and the log invocation) so the resulting message never reads "from to X" and
consistently conveys a valid source version or omits the log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 273dbe00-458b-419e-b47f-69c51b5c091a
📒 Files selected for processing (1)
connectors/class-connector-installer.php
phuochoit
force-pushed
the
phuochoit/mwp-1477
branch
from
cab3b5a to
364efcb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
connectors/class-connector-installer.php (1)
91-112: Normalize whitespace-only theme slugs tonull.Line 107 only rejects
''. A whitespace-only value can still pass and later cause invalid theme resolution.Proposed fix
private function normalize_theme_slug( $theme ) { if ( $theme instanceof \WP_Theme ) { $theme = $theme->get_stylesheet(); } if ( is_scalar( $theme ) ) { - $theme = (string) $theme; + $theme = trim( (string) $theme ); } if ( ! is_string( $theme ) || '' === $theme ) { return null; } return $theme; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@connectors/class-connector-installer.php` around lines 91 - 112, The normalize_theme_slug method currently only rejects the empty string but accepts whitespace-only slugs; update normalize_theme_slug to trim whitespace after casting scalars to string (e.g., $theme = trim($theme)); if the trimmed string is empty return null; otherwise return the trimmed slug. Ensure this change is applied in the private function normalize_theme_slug (handle the WP_Theme branch first, then cast and trim the scalar).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@connectors/class-connector-installer.php`:
- Around line 734-735: The code indexes $args['slug'] directly before passing it
to normalize_theme_slug which can raise "undefined array key" notices; guard the
access by checking for the key first (e.g. use isset($args['slug']) or
array_key_exists('slug', $args)) and handle the missing case (early return,
default value, or set $slug = null) before calling
$this->normalize_theme_slug($args['slug']) so normalize_theme_slug is never
called with an undefined index.
---
Nitpick comments:
In `@connectors/class-connector-installer.php`:
- Around line 91-112: The normalize_theme_slug method currently only rejects the
empty string but accepts whitespace-only slugs; update normalize_theme_slug to
trim whitespace after casting scalars to string (e.g., $theme = trim($theme));
if the trimmed string is empty return null; otherwise return the trimmed slug.
Ensure this change is applied in the private function normalize_theme_slug
(handle the WP_Theme branch first, then cast and trim the scalar).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 290664ce-83b5-4e33-b525-92a756350388
📒 Files selected for processing (2)
classes/class-db-driver-wpdb.phpconnectors/class-connector-installer.php
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit