Skip to content

Release version 0.1.3.1#499

Merged
randhirinsta merged 12 commits into
mainfrom
develop
Apr 22, 2026
Merged

Release version 0.1.3.1#499
randhirinsta merged 12 commits into
mainfrom
develop

Conversation

@randhirinsta
Copy link
Copy Markdown
Collaborator

  • Improved: Local push site creation and restore now handle task errors gracefully.
  • Improved: Local push now uses current site WP and PHP versions for staging site.
  • Improved: Local push now creates a reserved site for better reliability.
  • Improved: Migration cleanup will no longer remove the InstaWP Connect plugin if the site is connected and managed via InstaWP.
  • Fixed: Serve URL 404 error on /serve-instawp/ WordPress-routed migration fallback.

dev5 and others added 12 commits April 1, 2026 10:37
- Handle task error status in site creation and restore polling loops
- Use current site WP and PHP versions instead of hardcoded values
- Create reserved site for local push for better reliability
- Sanitize error messages in WP_Error responses

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: improve local push error handling and site creation
… connected

Prevents the InstaWP Connect plugin from being removed post-migration when the site
has an active connect_id, ensuring managed sites retain their connection to InstaWP.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: skip connect plugin deletion if site is connected
The existing pre-filter only skipped auto-appending the plugin slug when
connect_id was set. It did not cover entries added via post_uninstalls
(e.g. from white-label payloads). Add an array_search-based guard right
before the delete loop that removes instawp-connect from $plugins_to_delete
whenever the site has an active connect_id, instawp_is_staging flag, or
instawp_sync_connect_id parent reference — regardless of how the slug got
into the list.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: hard-guard instawp-connect from deletion in post-migration cleanup
Keeps the method on the migration REST class (no move to InstaWP_Tools)
but exposes it for external callers. Returns {success, modified, message}
at every exit point so HestiaCP scripts can log a human-readable status
after invoking it via 'wp eval' on pure pull migrations, which do not go
through handle_post_migration_cleanup.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
refactor: make sanitize_wp_config public and return status array
handle_post_migration_cleanup was calling instawp_reset_running_migration('hard', true)
unconditionally when clear_connect was set. The hard reset deletes instawp_api_options,
instawp_is_staging, and other critical options — leaving connected staging sites unable
to authenticate or heartbeat to the dashboard.

Now check instawp_get_connect_id() first: if a valid connect_id exists the site is
actively connected, so only run a soft reset (cleans migration temp data). Disconnected
sites still get the full hard reset + server-side disconnect as before.

Also moves $connect_id declaration above its first use (the reset block) so all
downstream references (plugin-delete guard, hard guard) share a single call.

Adds Variable Declaration Before Use and Self-Review principles to CLAUDE.md.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: use soft reset for connected sites in post-migration cleanup
The `handle_migration_through_wp()` handler still referenced the old
`serve.php` path after it was moved to `iwp-serve/index.php` in Feb 2025,
causing the WP-routed migration fallback to always 404 with "File not
found" and breaking pulls on sites where the direct serve URL is blocked.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fix: route /serve-instawp/ to iwp-serve/index.php after rename
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 22, 2026

Claude finished @randhirinsta's task —— View job


PR Review for InstaWP Connect v0.1.3.1 ✅

Context loaded:

  • CLAUDE.md (project guidelines)
  • Developer preferences (proceeding without git config access)

Review Checklist:

  • Analyze all changed files for architectural compliance
  • Check for code quality and WordPress standards
  • Review security considerations
  • Assess performance implications
  • Verify backward compatibility
  • Look for common anti-patterns
  • Provide comprehensive feedback

Summary

This PR introduces improvements to local push migration handling, site creation reliability, and migration cleanup logic. The changes are generally well-implemented but there are several architectural and security concerns that should be addressed.

🏗️ Architectural Review

✅ Positive Aspects

  1. Data Layer Abstraction: Code properly works with storage layer (options table) via Option::get_option() and Option::update_option()
  2. SoC: Good separation between API handling, migration logic, and CLI commands
  3. Error Handling: Comprehensive error handling with meaningful messages throughout

⚠️ Architectural Concerns

includes/apis/class-instawp-rest-api-migration.php:

  1. Code Duplication (DRY violation) - Lines 332, 335:

    error_log( 'sso_url_response: ' . wp_json_encode( $login_url_response ) );
    error_log( esc_html__( 'sso_url_class_not_found: This class...', 'instawp-connect' ) );

    Issue: Direct error_log() usage violates project guidelines
    Fix: Should use Helper::add_error_log() as specified in CLAUDE.md

  2. Variable Scope Issue - Line 442:

    'response' => $response,

    Issue: $response variable may not be defined in catch block scope
    Fix: Initialize $response before try block or remove from error array

includes/class-instawp-hooks.php:

  1. WordPress Route Fallback - Lines 54-73:
    The serve-instawp route handling is well-implemented with proper fallback and error logging.

🔒 Security Review

✅ Security Strengths

  1. REST API Security: Proper use of validate_api_request() for bearer token validation
  2. Input Sanitization: Consistent use of sanitize_text_field() for user inputs
  3. Plugin Deletion Protection: Excellent safeguards in migration cleanup (lines 395-411)

⚠️ Security Concerns

  1. CLI Command Authorization (includes/class-instawp-cli.php):
    // Line 57: Uses $_POST without nonce verification
    $migrate_settings = InstaWP_Tools::get_migrate_settings( $_POST );
    Issue: WP CLI context, but still concerning pattern
    Recommendation: Add explicit comment explaining CLI context justifies this

🚀 Performance & Optimization

✅ Good Practices

  1. Reserved Site Creation: Using reserved sites improves reliability (CLI local push)
  2. Migration State Management: Proper cleanup and soft reset for connected sites
  3. API Optimization: Current site WP/PHP versions sent to reduce server-side detection

🔧 Performance Suggestions

  1. Database Queries: The new migration conflict detection adds API calls but seems necessary for reliability
  2. Memory Usage: File operations in class-instawp-tools.php appear optimized for large migrations

📝 Code Quality Issues

WordPress Coding Standards Violations

  1. Error Logging (multiple files):

    error_log( 'migration clean up exception: ' . json_encode( $response ) );

    Fix: Replace with Helper::add_error_log()

  2. Variable Declaration (class-instawp-tools.php:150):
    The code appears to have proper variable scoping, but should verify all declarations precede usage.

🔄 Backward Compatibility

No Breaking Changes Detected

  • All new features are additive
  • Existing APIs maintain compatibility
  • Database schema changes appear safe

🧪 Testing Recommendations

  1. Migration Conflict Testing: Verify the new migration blocking logic works correctly
  2. Plugin Deletion Protection: Test that connected sites retain plugin after cleanup
  3. CLI Local Push: Test error handling in site creation and restore processes
  4. Serve URL Fallback: Test WordPress routing fallback for /serve-instawp/

📋 Required Fixes

High Priority

  1. Fix error logging violations - Replace all error_log() calls with Helper::add_error_log()
  2. Fix variable scope issue in catch block (line 442 of migration API)

Medium Priority

  1. Add CLI context comments for $_POST usage in CLI commands
  2. Consider extraction of migration conflict detection logic to reduce API class complexity

✅ Approved Changes

  • Migration cleanup plugin protection logic (excellent implementation)
  • Reserved site creation for local push
  • Serve URL WordPress routing fallback
  • Version information passing to staging sites

Overall Assessment: APPROVE with Minor Fixes

The PR implements valuable improvements to migration reliability and handles edge cases well. The architectural violations are minor and easily fixable. The security enhancements (especially plugin deletion protection) are excellent.

Recommendation: Fix the error logging issues and variable scope problem, then this is ready to merge.


@randhirinsta randhirinsta merged commit 177f6f8 into main Apr 22, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant