🇺🇦Ukraine @marchuk.vitaliy

vmarchuk → created an issue.

vmarchuk → created an issue.

tomtech → credited vmarchuk → .

@megachriz I agree with your suggestions.

@megachriz

Yeah, this is "Constructor property promotion". Since this module is not that big, it makes sense to refactor everything possible to support all PHP 8 features.

I'm not sure if the failed tests are related to the changes, but I've added changes to the query to use the 2.x commerce way.

vmarchuk → created an issue.

vmarchuk → created an issue.

A new branch "3481620-add-use-of" has been opened with fixes for PHP 8 features, but I don't think it's finished yet. Need to spend time on additional fixes and testing.

vmarchuk → created an issue.

@megachriz
OK, thanks for the reply. I will create at least a few separate issues to address all the issues from the comment ✨ Use queue for sending emails Needs review .

vmarchuk → created an issue.

Hello @megachriz,

I eventually got a fix for options 1-3 from a comment ✨ Use queue for sending emails Needs review . I don't think 4-5 are really needed now and can be done later if possible.
Do you think I can open a new branch in this issue and add a fix to it, or is it better to create a new issue?

Thanks.

vmarchuk → created an issue.

vmarchuk → created an issue.

@jsacksick: The phpstan / phpcs issues are fixed and the issue can be merged.

Added a patch compatible with commerce 2.X.

@megachriz

The module looks much better with this patch (it was like the D7 module before the changes)!
But at the same time, several things are missing:

1. Support for PHP 8 features (it would be great to add it now because the entire module is almost the commerce_abandoned_carts_cron() function)
2. The mail_abandoned_cart log template was introduced but never used
3. In getAbandonedOrders(), it would be better to use "o.type <> recurring" to detect commerce_recurring, so we don't need to join the commerce_order_item table
4. In getAbandonedOrders() to determine if an order is still a cart, it is better to use "o.cart = 1"(it's more commerce 2.x way) instead of condition('o.state ', 'draft')
5. We must consider the commerce_cart[cart_expiration][number] setting if set, i.e. history_limit must be less than commerce_cart[cart_expiration][number] otherwise all carts can be deleted before sending emails (don't forget about multiple order types?)
6. Probably consider using the advancedqueue module (maybe at least when it's installed?). It supports allow_duplicates and max_retries, seems to be what we need for this module (we added core queue and advancedqueue support to this module https://www.drupal.org/project/commerce_email → )

Also, the patch from MR works fine!

The minimum amount for Stripe is 0.5 https://docs.stripe.com/currencies#minimum-and-maximum-charge-amounts, so it is not possible to use the Stripe Payment Element that uses PaymentIntent. Instead, you need to use SetupIntent to collect payment information during checkout. See related issue https://www.drupal.org/project/commerce_stripe/issues/3406817 💬 Create add-payment-method for Stripe Payment Element Needs work

The attached patch in comment #10 works fine.
+1 for RTBC

TomTech → credited vmarchuk → .

@gilmord

I see no reason not to enable the "Block if CVC verification fails" and "Block if postal code verification fails" rules for the project. I just checked the Radar settings on our side for the projects and we have both options enabled, but not for all projects (it seems like it should be enabled for all) so we can catch errors in the iframe.

Can you send the credit cards you use and I'll run tests on my side with the Radar rules disabled.

This request is out of the scope of this module. The custom tokens can be added to a custom module.

Committed!

vmarchuk → created an issue.

Updated to lcobucci/jwt:5.3.0 and tested locally (composer require 'lcobucci/jwt:5.3.0 as 4.3.0') and it works great. The lcobucci/clock library has been replaced by psr/clock.
Opened MR with the fixes.
There seems to be a problem running the tests:

Problem 1
    - lcobucci/jwt[5.0.0, ..., 5.4.x-dev] require ext-sodium * -> it is missing from your system. Install or enable PHP's sodium extension.
    - Root composer.json requires lcobucci/jwt ^5 -> satisfiable by lcobucci/jwt[5.0.0, ..., 5.4.x-dev].
To enable extensions, verify that they are enabled in your .ini files:
    - /usr/local/etc/php/php-cli.ini

So that should be fixed as well.

vmarchuk → created an issue.

After applying the patch, the order bundle template is selected commerce-order-receipt--entity-print--quote.html.twig.

vmarchuk → created an issue.

We probably need to add more conditions to check if commerce_shipping is enabled, but here is a patch that fixes the problem.

vmarchuk → created an issue.

Added a patch compatible with the latest dev version.

vmarchuk → made their first commit to this issue’s fork.

The patch from #42 works fine.

Because "quantity" is on the order_item level. What you want to do can be done by going through all the order items and summing the 'quantity' field. So it must be a custom token.

vmarchuk → made their first commit to this issue’s fork.

vmarchuk → made their first commit to this issue’s fork.

vmarchuk → created an issue.

Results here https://git.drupalcode.org/issue/commerce_wishlist-3432043/-/pipelines/1...

Once the issues reported by gitlab-ci are resolved, we'll need to update the .gitlab-ci.yml file with something like this (see example):

#
# Linting jobs are passing so any issue that breaks them should fix them.
#
cspell:
  allow_failure: false
eslint:
  allow_failure: true
phpcs:
  allow_failure: false
phpstan:
  allow_failure: false
stylelint:
  allow_failure: false

vmarchuk → created an issue.

Added a small fix for Drupal coding standards and opened MR.
+1 to RTBC

vmarchuk → made their first commit to this issue’s fork.

I can also confirm that the patch in comment #5 fixes the issue. What is suggested in comment #8 also works fine, but the solution in comment #5 is simpler to understand what is going on there.
+1 to RTBC

@TomTech Thank you for pointing out any issues that may occur after the update.

From the documentation https://docs.stripe.com/upgrades#2022-11-15

Charge no longer auto-expands refunds by default. You can expand the list but for performance reasons we recommended against doing so unless needed.

Added a fix for this.

If in our case it is due to using PaymentIntent::retrieve($intent_id) without a try-catch.

vmarchuk → created an issue.

@TomTech

Thanks for the hook_update() related fix. I tested the patch locally, it seems to work fine.
The only thing I'm worried about is that after placing the order (and processing the queue) I see the payment_intent.succeeded event succeed. But it's a success because all the conditions we have in case Event::PAYMENT_INTENT_SUCCEEDED were skipped and the webhook did nothing.
Don't you think we should add a new condition and skip it?

Committed!

vmarchuk → created an issue.

@LeDucDuBleuet

Definitely possible. This could be hook_update() with a batch where we can go through all Stripe users and update them accordingly.
Please create a separate issue for this.

Update will be done in this issue https://www.drupal.org/project/commerce_stripe/issues/3403745 💬 Why is stripe-php still stuck on version 7 and Stripe API on 2019-12-03? What does it imply? Active

Update will be done in this issue https://www.drupal.org/project/commerce_stripe/issues/3403745 💬 Why is stripe-php still stuck on version 7 and Stripe API on 2019-12-03? What does it imply? Active

@TomTech

I wanted to test the patch locally, but I can't even apply the code changes:

➜  kickstart ddev drush updb -y
 ------------------------------- ----------- --------------- ------------------------------------------------- 
  Module                          Update ID   Type            Description                                      
 ------------------------------- ----------- --------------- ------------------------------------------------- 
  commerce_stripe_webhook_event   9101        hook_update_n   9101 - Install the advanced queue, if possible.  
 ------------------------------- ----------- --------------- ------------------------------------------------- 


 // Do you wish to run the specified pending updates?: yes.                                                             

>  [notice] Update started: commerce_stripe_webhook_event_update_9101
>  [error]  Drupal\Core\Entity\EntityStorageBase::create(): Argument #1 ($values) must be of type array, bool given, called in /var/www/html/web/modules/contrib/commerce_stripe/modules/commerce_stripe_webhook_event/commerce_stripe_webhook_event.install on line 159 
>  [error]  Update failed: commerce_stripe_webhook_event_update_9101 
 [error]  Update aborted by: commerce_stripe_webhook_event_update_9101 
 [error]  Finished performing updates. 
Failed to run drush updb -y: exit status 1

@TomTech @rszrama

Yes, that's why defer/async wasn't added earlier.
I think we need to update the JS to wait for the "Stripe" object to load, as done here (see waitForSdk) https://git.drupalcode.org/project/commerce_square/-/blob/8.x-1.x/js/com...

The patch is ready https://git.drupalcode.org/project/commerce_stripe/-/merge_requests/90.diff
Who can help with testing? Basically, there are not many changes and everything works fine.

vmarchuk → made their first commit to this issue’s fork.

Link to MR https://git.drupalcode.org/project/commerce_stripe/-/merge_requests/88/d...
@TomTech I left a few questions for MR, could you please check them? After that, I will test the patch locally.

vmarchuk → changed the visibility of the branch 3424966-processing-webhooks-in to active.

There is nothing to fix. This has already been fixed and merged. See the latest pipelines https://git.drupalcode.org/project/commerce_stripe/-/pipelines/110553

Committed!

vmarchuk → made their first commit to this issue’s fork.

vmarchuk → created an issue.

I can also confirm that this issue appears in 4.2.6, but I don't see it in 4.2.5.
The fix from MR fixes this, not sure if this fix is ​"good" but can be used as a temporary solution.

Given my experience with other payment gateways, we might introduce another problem, as soon as we process payment both in onReturn() and onNotify(), because the overlapping requests can cause double-placements and related problems. It's hard to find the perfect solution for this problem then. I've tried different approaches, which all work quite good for us. The latest one was to process payment info in onReturn() directly, and in onNotify() create an advancedqueue job item, which will be processed on cron then. This way, you get rid of 99% of conurrent request problems, can send an immediate response to the payment gateway, which is also important. And in most of the cases, there's no need to do any processing anymore, as the order is already placed in Drupal, when cron runs. But for the small amount of use cases, where the user never returned, the order is placed on cron then

This is correct and we planned to work on it in the future. There is no other way but to queue the webhook to be processed by a cron job.
From the file StripePaymentElement.php

        $queue_webhook = FALSE;
        if ($queue_webhook) {
          // Soon, we will support queueing webhook processing.
          return new Response('', 200);
        }

Yeah, a new version https://packagist.org/packages/enshrined/svg-sanitize#0.17.0 has been released. And the dependency ezyang/htmlpurifier: ^4.16 has been removed.

@agoradesign
This version contains the latest fixes/features https://www.drupal.org/project/commerce_stripe/releases/8.x-1.2-rc1 →

FYI: Google Pay and Apple Pay should work (we have already integrated these payment methods), we tested it on several LIVE projects and everything is fine.

@ramlev Can this issue be closed?

@hcksharp
In version 1.3 → , we removed the dependency on commerce_log. So if it is enabled you will be able to use logging, otherwise it will not be offered.

@ramlev

So in production or whatever, the queue won't be processed by cron (but how then)? If you only have trouble testing this locally (by running the drush command manually), then you need to use the ways it will work in production. As I told you before, it won't work when you run drush advancedqueue:queue:process myqueue manually, and that's expected.

@ramlev
I see this too, it's because $events[KernelEvents::REQUEST][] = ['onRequest', 900]; doesn't fire when you run the drush command manually. Can you try setting a cron job to run every 5 minutes (with ultimate_cron or with something similar) and let it process your queue? In the case of a cron job, onRequest should run and process your queue. Let me know the results of this test, in theory it should work.

@ramlev
Set a breakpoint in the EmailSubscriber::onRequest() and see if it runs.

Committed!

vmarchuk → created an issue.

vmarchuk → made their first commit to this issue’s fork.

Committed!

Yeah, I just reproduced it too. In the getReplyTo() method we need to use the default value and that's it.
MR updated with fixes.

vmarchuk → made their first commit to this issue’s fork.

vmarchuk → created an issue.

All phpcs issues fixed.
Committed!

vmarchuk → created an issue.

Committed!

Committed!

Committed!

vmarchuk → created an issue.

@jsacksick
Opened MR with fixes and + added DI for the \Drupal::service('module_handler')

vmarchuk → made their first commit to this issue’s fork.

Committed!

@Anybody

I've just added a description to the "Reply to" field.
Unfortunately, we don't have tests for this module yet, but we will probably think about it in the future.

Committed!