pronamic / wp-pay-core

Core components for the WordPress payment processing library. This library is used in the WordPress plugin Pronamic Pay: https://www.pronamicpay.com/, but also allows other plugin developers to set up a payment plugin.
https://www.wp-pay.org/
GNU General Public License v3.0
27 stars 3 forks source link

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

Closed knit-pay closed 3 months ago

knit-pay commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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?

knit-pay commented 3 months ago

@remcotolsma

I have updated the priority to 100 in the PR.

codecov-commenter commented 3 months ago

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 :warning:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.