Skip to content

fix: plugin deactivation error#13

Merged
alaminfirdows merged 4 commits into
developfrom
fix/deactivation-error
Apr 18, 2025
Merged

fix: plugin deactivation error#13
alaminfirdows merged 4 commits into
developfrom
fix/deactivation-error

Conversation

@alaminfirdows
Copy link
Copy Markdown
Contributor

@alaminfirdows alaminfirdows commented Apr 18, 2025

Previously, the deactivation hooks were dependent on FlyWP\Api\UpdatesData, which had an internal dependency on route initialization that caused an error.

This PR simplifies the plugin deactivation hooks and removes the dependency.

Summary by CodeRabbit

  • Refactor
    • Improved handling of scheduled background tasks during plugin deactivation for better reliability.
  • Chores
    • Updated internal constants to be more accessible for future enhancements.

@coderabbitai

This comment was marked as off-topic.

@alaminfirdows alaminfirdows self-assigned this Apr 18, 2025
@alaminfirdows alaminfirdows requested a review from tareq1988 April 18, 2025 07:01
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
flywp.php (1)

93-97: Good refactoring to simplify the deactivation process.

The deactivation logic has been improved by directly unscheduling the cron event without instantiating the UpdatesData class, which aligns with the PR objective to fix plugin deactivation errors.

However, there's a code style issue flagged by static analysis.

Consider separating the assignment from the condition to improve readability and address the static analysis warning:

-    public function deactivate() {
-        if ( $timestamp = wp_next_scheduled( FlyWP\Api\UpdatesData::CRON_HOOK ) ) {
-            wp_unschedule_event( $timestamp, FlyWP\Api\UpdatesData::CRON_HOOK );
-        }
-    }
+    public function deactivate() {
+        $timestamp = wp_next_scheduled( FlyWP\Api\UpdatesData::CRON_HOOK );
+        if ( $timestamp ) {
+            wp_unschedule_event( $timestamp, FlyWP\Api\UpdatesData::CRON_HOOK );
+        }
+    }
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection

[failure] 94-94:
Assignments must be the first block of code on a line


[warning] 94-94:
Variable assignment found within a condition. Did you mean to do a comparison?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 11cb9b6 and cef8514.

📒 Files selected for processing (2)
  • flywp.php (1 hunks)
  • includes/Api/UpdatesData.php (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Inspections
includes/Api/UpdatesData.php

[warning] 1-1: Variable assignment found within a condition. Did you mean to do a comparison?


[error] 1-1: Assignments must be the first block of code on a line.

🪛 GitHub Check: Run PHPCS inspection
flywp.php

[failure] 94-94:
Assignments must be the first block of code on a line


[warning] 94-94:
Variable assignment found within a condition. Did you mean to do a comparison?

🔇 Additional comments (1)
includes/Api/UpdatesData.php (1)

6-7: Appropriate visibility change for constants to support refactoring.

Making the cron constants public provides the necessary access for the plugin's deactivation process without requiring instantiation of the UpdatesData class.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
flywp.php (2)

95-95: Remove trailing whitespace.

There's unnecessary whitespace at the end of this line that should be removed to maintain code quality standards.

-        $timestamp = wp_next_scheduled( FlyWP\Api\UpdatesData::CRON_HOOK );
-    
+        $timestamp = wp_next_scheduled( FlyWP\Api\UpdatesData::CRON_HOOK );
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection

[failure] 95-95:
Whitespace found at end of line


88-93: Update the PHPDoc description.

The method PHPDoc comment incorrectly states "Plugin activation hook" when it should say "Plugin deactivation hook".

-     * Plugin activation hook.
+     * Plugin deactivation hook.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cef8514 and 3d70b15.

📒 Files selected for processing (1)
  • flywp.php (1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
flywp.php

[failure] 95-95:
Whitespace found at end of line

🪛 GitHub Actions: Inspections
flywp.php

[error] 1-1: Whitespace found at end of line

🔇 Additional comments (2)
flywp.php (2)

94-98: Improved deactivation method by removing UpdatesData class dependency.

The changes correctly refactor the deactivation logic to directly check and unschedule the cron event using WordPress core functions, which effectively resolves the plugin deactivation error by eliminating the dependency on the UpdatesData class's deactivate method. This is a cleaner approach that makes the deactivation process more reliable.

🧰 Tools
🪛 GitHub Check: Run PHPCS inspection

[failure] 95-95:
Whitespace found at end of line


94-94:

Details

✅ Verification successful

Verify the constant visibility in UpdatesData class.

Make sure that the CRON_HOOK constant in the UpdatesData class is now defined as public to enable direct access from this file.


🏁 Script executed:

#!/bin/bash
# Search for the CRON_HOOK constant definition in the UpdatesData class
grep -n "CRON_HOOK" --include="*.php" --include="*.inc" -r ./includes/

Length of output: 474


CRON_HOOK visibility confirmed
The CRON_HOOK constant in includes/Api/UpdatesData.php (line 6) is already declared as public, so it can be accessed directly. No further changes are needed.

@alaminfirdows alaminfirdows merged commit 088ebfe into develop Apr 18, 2025
2 checks passed
@alaminfirdows alaminfirdows deleted the fix/deactivation-error branch July 9, 2025 14:17
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.

2 participants