Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed priority of handle_returns and maybe_redirect to fix notification issue. #181

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

knit-pay
Copy link
Contributor

@knit-pay knit-pay commented Jun 4, 2024

Some plugins that perform tasks after payment confirmation (Eg WPNotif), get loaded late, because of which notification does not get triggered after payment confirmation if we confirm the payment sooner than they get loaded. So, I have increased the priority of handle_returns and maybe_redirect to max possible value, to make sure, notification plugins or any other dependent plugin gets loaded before payment gets confirmed.
My client reported this issue in which SMS and Whatsapp message was not getting triggered by WPNotif in case of Knit Pay whereas WPNotif was working fine for other payment gateway plugins.

@remcotolsma
Copy link
Member

I'm not a fan of using PHP_INT_MAX in such situations. It makes it difficult or impossible to hook functions in after our action functions. It also feels like a somewhat drastic measure to solve this problem. With what priority does WPNotif hook into the wp_loaded action?

This hook is fired once WP, all plugins, and the theme are fully loaded and instantiated.

https://developer.wordpress.org/reference/hooks/wp_loaded/

Maybe it is a problem that actually needs to be solved in WPNotif. Is it this plugin: https://wpnotif.unitedover.com/? You mentioned 'WPNotif' and 'WPNotify'. The question might be why isn't this plugin fully loaded when wp_loaded is executed?

@knit-pay
Copy link
Contributor Author

knit-pay commented Jun 4, 2024

Yes, the Plugin name is WPNotif, I miss-typed it to WPNotify at one place. I have corrected it.
https://codecanyon.net/item/wpnotif-wordpress-sms-whatsapp-notifications/24045791

WPNotif is using default priority (ie 10) for hooking into the wp_loaded action.

Here is the code of WPNotif ( from file /wpnotif/includes/functions.php )
add_action('wp_loaded', array($this, 'woocommerce_loaded'));

    public function woocommerce_loaded()
    {
        if (class_exists('WooCommerce')) {
            $statuses = wc_get_order_statuses();
            foreach ($statuses as $key => $status) {
                $key = str_replace('wc-', '', $key);
                add_action('woocommerce_order_status_' . $key, array($this, 'wc_order_status_change'), 20);
            }
            add_action('wpn_wc_notify_order', array($this, 'notify_order'), 20);

            add_action('woocommerce_new_order', array($this, 'wc_new_order'), 20, 2);
        }
    }

I understand, here WPNotif should use plugins_loaded hook instead of wp_loaded hook. Even if they are using the wp_loaded hook, they should be using a lower priority than 10. The client also tried contacting the WPNotif support team, but for other default and third-party payment gateways of WooCommerce, WPNotif is working fine, that's why they are not taking pain to fix their code. Because of this, the client is suffering. I was thinking some other notification plugins might also be doing something similar which might conflict with our code, It would be better if we could increase our priority.

If not PHP_INT_MAX, can we increase it a little bit, something like 20? What do you suggest?

@remcotolsma
Copy link
Member

If not PHP_INT_MAX, can we increase it a little bit, something like 20? What do you suggest?

A priority of 20 or 100 seems fine to me, with this information it is sufficiently clear why we use a higher priority. Do you adjust the PR so we can merge it in?

Updated priority to 100 as suggested by @remcotolsma
@knit-pay
Copy link
Contributor Author

knit-pay commented Jun 4, 2024

@remcotolsma

I have updated the priority to 100 in the PR.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 21.89%. Comparing base (6bb8284) to head (4819307).
Report is 23 commits behind head on main.

Files Patch % Lines
src/Plugin.php 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #181      +/-   ##
============================================
- Coverage     21.89%   21.89%   -0.01%     
  Complexity     2530     2530              
============================================
  Files           107      107              
  Lines         10380    10382       +2     
============================================
  Hits           2273     2273              
- Misses         8107     8109       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@remcotolsma remcotolsma merged commit b0a6a19 into pronamic:main Jun 4, 2024
14 of 15 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.

3 participants