I've opened an issue on Commerce Stripe so that they can explain why they needed to display the PaymentElement at Review step instead of Payment step. 🐛 Displaying PaymentElement at Review step might not be the way to go Active
nicolas bouteille → created an issue.
I see... well Commerce Stripe has implemented Payment Element which requires that the pane where the Stripe PaymentElement JS object is shown to the customer at the Review step. So maybe indeed they should not have done so.
Hi Tom,
It's nice to see you've been working on it :)
Are you sure you wanna set the order balance as a fallback?
IntentHelper::getPrice($payment_intent) ?? $order->getBalance()
I say if we are not able to retrieve the amount from the PaymentIntent it means something bad has happened and we should stop here.
which would allow the customer to add/delete items from their cart
as mentioned here
🐛
Lock the order before stripe.confirmPayment() is called
Active
, the order needs to be locked before triggering the payment. As long as the order is locked, the order will not be visible anymore on /cart and adding new stuff to the cart will create a new cart and not affect this specific order.
If we place the order with the order total not being the paid amount, if the site is responsible for creating the invoice, it will be created with the wrong amount, which can cause legal problems because an invoice shall not be generated twice.
return page handling invokes onReturn() on the StripePaymentElement gateway. PaymentProcess should actually not even be encountered
I think you might be wrong here. From my understanding, PaymentCheckoutController::returnPage is the entry point. It calls first StripePaymentElement::onReturn() but only to store the Payment Method, and PaymentProcess is called right after (the next checkout step) in order so store the Commerce Payment regarding the successful Stripe payment.
And as I mentioned before, because of the delay for queued webhook processing, return url will be responsible for finalizing the order 99% of the time.
nicolas bouteille → created an issue.
Maybe "non draft" is too wide and should be replaced by "is completed or canceled". But the point is the order's items price should not be recalculated on an order which payment has already been initialized or processed. I suggest you try out the scenario I described:
As of right now, you can witness that expiring a promotion while waiting for the customer to confirm/finish an offsite payment will result in the order's total amount being modified and not corresponding to the amount that really has been paid anymore. This even though the order was locked.
Thank you for your feedback, unfortunately I am not able to fully understand your answer and your position so I am not able to further respond :/
nicolas bouteille → created an issue.
nicolas bouteille → created an issue.
nicolas bouteille → created an issue.
nicolas bouteille → created an issue.
Hello,
I totally agree with this. We are about to use Stripe Checkout on our site which means customers will leave our site right after cart validation and Stripe will handle billing information and payment steps.
We need to lock the order as soon as the customer leaves our site to make sure he cannot update the order once we've communicated the price to be paid to Stripe.
But for the reasons explained above, it can happen that the customer comes back on our website on the cart step.
As mentioned above, the default behavior is to hide locked carts so getting back to /cart will show an empty cart. From this point, adding something to his cart will create a new order while the first order still exists and can be paid.
We too did not like this behavior so here is what we did:
We patched CartProvider.php and commented out:
//if ($cart->isLocked()) {
//continue;
//}
Now when the cart is locked, it still shows up in /cart
This means customers can now interact on a locked cart and change the order, so we had to reinforce the code to make sure that never happens.
In CartManager.php, functions emptyCart(), addOrderItem(), removeOrderItem() and updateOrderItem() need to be updated to do nothing (return;) if the $cart->isLocked
For the record, for those who would like to do it on their site while this is not in core, this time, to make it easier to maintain, we did not create a patch, we created a custom class MyModuleCartManager.php that extends CartManager and that overrides only the functions mentioned above. For example:
public function emptyCart(OrderInterface $cart, $save_cart = TRUE) {
if ($cart->isLocked()) {
return;
}
parent::emptyCart($cart, $save_cart);
}
For this custom class to replace CartManager, we created a custom ServiceProvider that extends ServiceProviderBase as explained here:
https://www.drupal.org/docs/drupal-apis/services-and-dependency-injectio... →
Be aware to name it exactly in PascalCase MyModuleNameServiceProvider and place it in my_module/src/.
This is what the code looks like:
class MyModuleNameServiceProvider extends ServiceProviderBase {
public function alter(ContainerBuilder $container) {
$cart_manager = $container->getDefinition('commerce_cart.cart_manager');
$cart_manager->setClass(MyModuleCartManager::class);
}
}
We also made sure to visually remove the buttons on the cart form that would allow to remove cart items or update quantities...
We also had to do the same for Coupons.
In CouponRedemption.php buildInlineForm() needs to hide the coupon form if the order is locked and validateInlineForm() must stop and show an error in case a coupon is submitted but the order has been locked since.
For this, we also created a custom class MyModuleCouponRedemption.php but this time we used hook_commerce_inline_form_info_alter to replace CouponRedemption with our class:
function my_module_commerce_inline_form_info_alter(&$definitions, $b = null, $c = null, $d = null) {
if (isset($definitions['coupon_redemption'])) {
$definitions['coupon_redemption']['class'] = MyModuleCouponRedemption::class;
}
}
So now the customer can see his locked cart, but cannot interact with it anymore. We needed to explain to him what was happening.
When the customer is at /cart and his cart is locked, we now display a warning message that says the order is locked and cannot be updated anymore because its price needs to remain what's been communicated to the payment partner.
We also give the opportunity to the customer to definitely cancel this order. So we added a custom button that will cancel the Stripe PaymentIntent and the order. Then the customer sees an empty cart.
The custom code here is added in views-view--commerce_cart_form.html.twig based on new variables passed through hook_preprocess_views_view__commerce_cart_form
I understand that this Cancel order button is probably where and why Commerce Core is going to have some trouble implementing all this. Because in OffsitePaymentGatewayInterface there is no function cancelRemotePayment yet. So Commerce Core probably does not have a way to cancel the offsite payment of an order. So if you cannot make sure to properly cancel the order with the remote payment intent as well, you'd better hide the locked cart and allow the customer to start a new cart.
Maybe this cancel button should be visible only under some conditions like the PaymentGateway is not Offsite or the function cancelRemotePayment is implemented…
I've just asked chatGPT a few questions. Here is what I can report: the way Stripe and Commerce Stripe are working right now, there can only be one successful PaymentIntent per order. Indeed, no matter what kind of payment method Stripe lets the customer choose from, even for payments splitted over time like Klarna or Afterpay, there will be only one PaymentIntent for one transaction. Stripe handles the multiple payments internally and should transfer us the total amount right away. There might be multiple "charges" inside one PaymentIntent in some scenari, but always only one PaymentIntent. This actually makes sense because we are actually the ones technically creating the PaymentIntent in the first place with the order balance as amount.
Even for people using Stripe Checkout, thus only creating the Checkout Session and letting Stripe being responsible for creating the PaymentIntent, Stripe guarantees it will only create one PaymentIntent for one given Checkout Session.
All that being said, the problem I raised still remains. When the customer comes back to our site after a successful payment, we cannot create the commerce_payment with the order balance as the amount and assume this is the amount that has just been paid. Because as I myself have been able to witness, it is technically possible that by the time the customer returns to the site, the order balance has changed and is not the same as the amount that has truly been paid! Which is bad and we should stop completing the order right there and raise an error.
There are two scenari:
1/ webhook handling:
In StripePaymentElement->processWebhook I think we should simply stop setting the order balance inside the commerce_payment and directly set the PaymentIntent's amount instead.
Btw, this requires to create a Price object from the PaymentIntent's amount and currency. And convert the PaymentIntent's amount format from 2499 into 24.99.
We could also still verify that the PaymentIntent's amount matches the order balance and raise an error if it's not the case but the most important is to make sure what we write in the database is the reality.
I also still believe calling $order->getState()->applyTransitionById('place'); shall not be done manually here and that, as I mentioned earlier above, let OrderStorage->doOrderPreSave() check if the order has been fully paid before placing it as completed.
2/ return page handling:
The returnPage callback leeds us to the generic PaymentProcess->createPayment() that creates the commerce_payment with the order balance. This code belongs to Commerce Payment (core). It does not depend on Stripe at all so there is no way to set the PaymentIntent's amount here. Furthermore, this code is also called when creating the commerce_payment before the remote payment has been processed, which is actually why we set the order balance. So obviously no issue shall be created on Commerce Payment's side.
StripePaymentElement->createPayment() is called right after, and this is where we can verify that the amount of the commerce_payment matches the PaymentIntent's amount before we mark the commerce_payment completed.
nicolas bouteille → created an issue.
Well for now we're using the code I shared above, but first we're checking if we are currently rebuilding caches and our blocking code is only executed if we are indeed rebuilding caches. Because we are sure the rebuilding caches process will also rebuild search api indexes, it is ok to block regular pages from trying to do the exact same thing. It is actually necessary to block them during that specific period of time.
The way we are doing it so far is we check whether our index still exists or has been deleted from the cache.
$our_index_in_cache = \Drupal::cache('config')->get('search_api.index.our_index');
if( ! $our_index_in_cache) {
// if our index is not in cache it means we are currently rebuilding cache
// so any regular page call in maintenance mode that could trigger this hook will simply return []
// only drush cr or admin_toolbar_tools.flush will be allowed to let the rest of the code execute
}
So far this seems to be working well. We did not have errors during our last production deployment. But I am no expert and this is experimental stuff. I was hoping for some help and more robust code from maintainers, but it seems they don't share my conclusions so far.
This problem only occurs for high traffic websites with lots of data. It only happens because many people are visiting your website while you put it in maintenance mode. I don't know why but hook_views_data gets triggered by simple page calls when the maintenance mode is on. So if your rebuilding cache while in maintenance mode with many people visiting your site, you're gonna get lock conflicts because of loadMultiple. Also the more data needs to be written in your index cache, the more time the lock stays on, the more chances to have a lock conflict. It works both ways btw, the more data Drupal needs to write inside cache_config when rebuilding caches, the more chances search_api_views_data has be blocked by a lock on cache_config if it's called while caches are rebuilding.
In the same way, at the end of StripePaymentElement->processWebhook, we call $this->placeOrder, which calls $order->getState()->applyTransitionById('place');
So here we assume that there can only be one webhook / PaymentIntent for a given order and because this payment was successful, we can assume the order is complete. Are we sure about that?
I have noticed OrderStorage->doOrderPreSave() checks if the order has been fully paid and if so, it dispatches the OrderEvents::ORDER_PAID event. OrderPaidSubscriber listens to this event and calls $order->getState()->applyTransitionById('place') for OffsitePaymentGatewayInterface.
Shouldn't we rely on this instead, allowing us to have multiple payments / PaymentIntents / webhooks on one same order before it is fully paid and only then can we complete it.
Ok funny enough (or not), because of many sales that happened on our website during Black Friday and Cyber Monday, I have had for the first time, 3 use cases where the commerce_payment has been created with the order balance as its amount BUT the PaymentIntent's amount was actually not the same as the order balance!
So I definitely think we should assign the amount that has really been paid by the PaymentIntent when we create the commerce_payment. And if this amount is not equal to the order's balance, we should raise an error and not complete it automatically.
So what happened to us, well, we had a coupon code that expired at 23:59:59 and a customer initiated the payment at 23:59 and it finished at 00:00. Because of another bug that I will have to investigate on later, the product price was re-updated after the payment was successful, so the order total was changed. So when creating the commerce_payment, the order balance was not the correct price anymore.
The second case was pretty much the same, someone bought a product while the market team manually udpated a coupon code.
The last case was different but I won't speak of it for now because it might be an important security breach so I need to reproduce it first and I will contact maintainers in private about it, but in the end the PaymentIntent's amount was not the order's expected total amount anymore.
BTW, since the problem resides in both StripePaymentElement->processWebhook and PaymentProcess->createPayment, I suggest we also create an issue in Commerce Core (payment).
nicolas bouteille → created an issue.
ok thanks!
Thanks for the quick reply!
Ok then if I am not mistaken, the view config is not imported.
I noticed commerce_stripe_webhook_event_update_9101 is responsible to import advancedqueue.advancedqueue_queue.commerce_stripe_webhook_event.yml
but their does not seem to be a code importing views.view.commerce_stripe_webhook_event.yml
BTW, I noticed you created both a view (available in views.view.commerce_stripe_webhook_event.yml) and a custom route with the overview callback. I figured you created the view first but then switched to the custom route as a replacement but never deleted the optional config file. Is that right?
nicolas bouteille → created an issue.
In the end, here is the solution I'm probably going to go for instead of using Advanced Queue:
// first, I store the webhook data in the database, then I trigger it's processing but through a cURL post request that will work asynchronously, allowing me to respond to Stripe after 1 second of waiting max, or less if processing is even faster.
$http_client = \Drupal::httpClient();
$domain = \Drupal::request()->getSchemeAndHttpHost();
try {
$http_client->post($domain . '/process-webhook-async', ['timeout' => 1]);
} catch (ConnectException $e) {
// timeout exception will be thrown if webhook processing takes more than 1 sec, but it's ok, we do nothing and let the rest of the code execute so that Stripe gets the 200 code response
// webhook processing will not be affected by the 1 second limit
}
return new Response('', 200);
this would replace the Queue for me, allowing me to process the webhook ASAP but still return a 200 code to Stripe ASAP as well and even if the webhook processing would fail. And I would still be able to reprocess a failed webhook based on what's stored in the database.
What do you think? Do you see a problem with my solution? Do you still see any advantage to use AdvancedQueue ?
Hi guys,
I am considering using Stripe webhooks in order to finalize orders as recommended by Stripe. Stripe also recommend to respond with a 200 code ASAP before even dealing with the webhook itself. They also recommend to use an asynchronous queue so that we can deal with multiple events being sent simultaneously.
I figured this is why you decided to give the option to queue events and use Advanced Queue instead of processing webhooks right away.
Now, I discovered that queues need to be processed either by drush or by cron.
My server is currently set to run Drupal's cron every 5 minutes. So this would mean that in the worst case scenario, if I rely only on webhooks to finalize the order, it could take up to 5 minutes before the order actually completes!?
Am I correct?
I noticed that even when webhooks are enabled, you still also rely on the code inside onReturn() to finalize the order. It is not disabled. Am I right as well?
Did you do that precisely because relying only on queued webhooks could take too long so you prefer to rely on the return url to finalize the order and webhooks are only here as a safe guard in case something goes wrong with the return url?
I still believe that 5 minutes could be too long even for the safe guard, and the webhook should be processed ASAP right after the 200 response is sent to Stripe.
ChatGPT tells me I could manually trigger the queue like this
\Drupal::service('advancedqueue.worker_manager')->processQueue('my_custom_queue');
what do you think of this? why didn't you do it?
Chat also told me about continuous workers like Supervisord and Systemd. Is that a good idea? Is it easy to use? Do you use them?
Thank you in advanced, I'm a bit lost, I thought webhooks was the way to go, I thought queueing them was the way to go, and now I don't know what to think anymore…
Closing this because Stripe Webhooks have actually been added in version 1.2 in a submodule that makes use of Avanced Queue.
This is actually a great implementation and queueing webhooks instead of calling processWebhook immediately ensures that these Stripe's recommandations are followed:
1/ respond to Stripe with a 200 code ASAP.
2/ makes us more inclined to receive lots of events simultaneously on our endpoint. For example all subscription renewals triggered at the same time.
3/ makes it possible to process a webhook again that could have failed with no need to ask Stripe to send it again
Thank you for you great work on this guys!
From what I understand, when webhooks are enabled, Commerce and thus Commerce Stripe as well, are relying on both the return page and webhooks to mark the order as "paid" and complete. Is this the reason why 2 payments were mistakenly created here?
From what I read, when relying on webhooks to finalize the order, the return page should not try to finalize it as well anymore. Do you know why the choice has been made to keep using both these mechanisms side by side? Also, can you explain how you fixed this problem? I tried to understand the code behind the merge request 131 but did not understand it. To prevent this from happening, do we need to use "automatic_async" capture_method so that the webhooks are called faster hoping they get called before the return page?
Thank you for your enlightenments
Ok it seems there might be some confusion about my suggestion here.
I am not saying that search_api_views_data should not put a lock on cache_config when it needs to write inside that table.
My main point is that, when you know for sure that someone very important is about to write stuff really important inside a book, you don't want to take away the book and prevent them to do it just so that you can write down something they were gonna write anyway.
So, when we now that cache are being rebuilt, and that a lot of important stuff is about to be written in cache_config + the cache rebuilding process is about to call search_api_views_data anyway, we should not allow any other process to trigger search_api_views_data as well at the same time, thus blocking any writing inside cache_config, especially not a visitor requesting a simple page, especially while in maintenance mode. Which is what we are facing right now.
The problem here is not a silently broken site, it is a broken site that you cannot repair, because even in maintenance mode drush cr always fails because of the locks put on cache_config by search_api_views_data triggered by visitors requesting pages. The more visitors we have, the more risks we have of never being able to complete a drush cr on a production site.
So I agree that my code would need an additional condition that would make sure caches are being rebuilt before preventing any other processes than drush cr to go through simultaneously.
If we do that, won’t Views just cache that empty array as the new Views data for the Search API, breaking all search views?
The most important IMHO is to prevent search_api_views_data to put a lock on cache_config that would prevent drush cr from correctly rebuilding caches. Knowing that drush cr also calls search_api_views_data anyway, this is why I suggested this. But I have to confess that for my suggestion, I assumed that search_api_views_data would be called only when caches are being rebuilt. This is because when testing, the only way I could make a classic page request trigger search_api_views_data was when I emptied caches tables. If there is even a slight chance that hook_views_data are called outside of a cache rebuild situation, what you predict would indeed happen, the empty array could override the search data…
In that case, we should first try to detect if the caches are actually being rebuilt and only block the execution triggered by classic page requests when it is the case.
Also, I’m pretty sure other code is loading config entities during their hook_views_data() implementation as well
I had the exact same thought, but the other way around... I wondered: how come search API is the only one of our module to ever cause deadlock on cache_config in their hook_views_data implementation? So that's why I started to question the implementation and the use of Index::loadMultiple in that context.
I think this context is pretty tricky indeed. The fact that on a live website with many visitors, even with the site under maintenance hook_views_data can be called many times simultaneously is problematic.
More generally speaking though, I think maybe ideally the code responsible for rebuilding Search API data available for Views should not be triggered by visitors page call ever. Should all hook_views_data implementations never get triggered by visitors? Or should Search API rely on something else than this hook to rebuild its data for views...?
I would like to suggest one more thing: if Index::loadMultiple is indeed the code responsible for writing in cache_config when the indexes are not in cache (because search_api_index are configuration entities and not content ones…) Then I think maybe we'd better not use loadMultiple to retrieve index data inside search_api_views_data() which is a hook that will be called when caches are rebuilt. More direct database requests would avoid lock conflicts here on cache_config.
That being said, I still believe our best bet here is to prevent the execution of search_api_views_data() unless it is being triggered by drush cr or admin/flush because even if the lock conflicts are avoided for cache_config, there could be new lock conflicts when multiple simultaneous calls to search_api_views_data() try to write the same data at the same place.
For the record, I now understand better why we are having these lock conflicts on our production website when we thought rebuilding cache under maintenance mode would prevent them from happening. We have light ajax calls triggered on our pages every 20s to store our student progress. Normally these ajax calls never trigger search_api_views_data() even if all cache tables are emptied.
However, I noticed that when the site is under maintenance and all caches are emptied, calling one of these ajax urls will trigger search_api_views_data() this time. Normally, search_api_views_data() should be triggered only once... but if multiple ajax calls are requested quite simultaneously on a production website with lots of visitors, they will all trigger search_api_views_data and generate multiple deadlock errors in the logs. This is what we faced during our last deployment. The code I suggested above should protect us from that in the future I hope.
I would like to suggest one more thing: if Index::loadMultiple is indeed the code responsible for writing in cache_config when the indexes are not in cache (because search_api_index are configuration entities and not content ones…) Then I think maybe we'd better not use loadMultiple to retrieve index data inside search_api_views_data() which is a hook that will be called when caches are rebuilt. More direct database requests would avoid lock conflicts here on cache_config.
That being said, I still believe our best bet here is to prevent the execution of search_api_views_data() unless it is being triggered by drush cr or admin/flush because even if the lock conflicts are avoided for cache_config, there could be new lock conflicts when multiple simultaneous calls to search_api_views_data() try to write the same data at the same place.
For the record, I now understand better why we are having these lock conflicts on our production website when we thought rebuilding cache under maintenance mode would prevent them from happening. We have light ajax calls triggered on our pages every 20s to store our student progress. Normally these ajax calls never trigger search_api_views_data() even if all cache tables are emptied.
However, I noticed that when the site is under maintenance and all caches are emptied, calling one of these ajax urls will trigger search_api_views_data() this time. Normally, search_api_views_data() should be triggered only once... but if multiple ajax calls are requested quite simultaneously on a production website with lots of visitors, they will all trigger search_api_views_data and generate multiple deadlock errors in the logs. This is what we faced during our last deployment. The code I suggested above should protect us from that in the future I hope.
After digging a bit more:
It seems that Index::loadMultiple() in search_api_views_data() is responsible for writing into cache_config if the indexes are no longer in cache.
Also, it seems that search_api_views_data() (like all hook_views_data implementations) will only be called if both views_data:[language] and views_data:views:[language] entries are missing from table cache_default.
When the caches are being emptied, any visitor accessing a page will thus trigger search_api_views_data(), which will then try to write into cache_config because the indexes are missing from cache.
Drush cr will also call search_api_views_data() in its process.
Most probable scenario: drush cr gets here first, puts the lock on cache_config leading to the deadlock found error when search_api_views_data() gets executed by visitors requesting pages. This is no so bad since drush cr will eventually call search_api_views_data() anyway, so we could indeed not throw the exception.
Worst case scenario : a visitor triggers search_api_views_data() and puts a lock on cache_config preventing drush cr from rebuilding correctly the caches. This is bad and should be avoided.
Suggested solution:
I think we should only allow the execution of search_api_views_data() if triggered by drush cr or /admin/flush and stop its execution if triggered by a regular page request.
Here is the custom code I will be adding right now to my project in order to do so. What do you think?
<?php
if (php_sapi_name() === 'cli' && in_array('./vendor/bin/drush', $_SERVER['argv'])) {
// case drush cr: do not block execution
}
elseif (\Drupal::routeMatch()->getRouteName() === 'admin_toolbar_tools.flush') {
// case /admin/flush: do not block execution
}
else {
// triggered by visitor requesting a page: stop execution to make sure no lock gets in the way of cache rebuilding
return [];
}
?>
After digging a bit more:
It seems that Index::loadMultiple() in search_api_views_data() is responsible for writing into cache_config if the indexes are no longer in cache.
Also, it seems that search_api_views_data() (like all hook_views_data implementations) will only be called if both views_data:[language] and views_data:views:[language] entries are missing from table cache_default.
When the caches are being emptied, any visitor accessing a page will thus trigger search_api_views_data(), which will then try to write into cache_config because the indexes are missing from cache.
Drush cr will also call search_api_views_data() in its process.
Most probable scenario: drush cr gets here first, puts the lock on cache_config leading to the deadlock found error when search_api_views_data() gets executed by visitors requesting pages. This is no so bad since drush cr will eventually call search_api_views_data() anyway, so we could indeed not throw the exception.
Worst case scenario : a visitor triggers search_api_views_data() and puts a lock on cache_config preventing drush cr from rebuilding correctly the caches. This is bad and should be avoided.
Suggested solution:
I think we should only allow the execution of search_api_views_data() if triggered by drush cr or /admin/flush and stop its execution if triggered by a regular page request.
Here is the custom code I will be adding right now to my project in order to do so. What do you think?
<?php
if (php_sapi_name() === 'cli' && in_array('./vendor/bin/drush', $_SERVER['argv'])) {
// case drush cr: do not block execution
}
elseif (\Drupal::routeMatch()->getRouteName() === 'admin_toolbar_tools.flush') {
// case /admin/flush: do not block execution
}
else {
// triggered by visitor requesting a page: stop execution to make sure no lock gets in the way of cache rebuilding
return [];
}
?>
Thank you the patch applies cleanly on Drupal 10.3.2 and fixes the problem of the HTML of the front page being sent in response of the ajax call when the user is logged out. Now we immediately have the simple 503 service unavailable error which is what we want.
Hello,
I faced this problem today.
We have routes that are called in ajax, some of them are called every 20 seconds on some lesson page in order to save the time spent by the student on the lesson.
In the middle of a deployment procedure, with the site under maintenance, an error was caused by this ajax route when it shouldn't have. Because there is no way the code of this ajax route could possibly cause this specific error.
After debugging I discovered that the first time the ajax route is called, when the student is still authenticated, the homepage was returned in HTML by the ajax call. Any call after that would return a nice 503 service unavailable response. So it was the building of the homepage that was responsible for that error when calling the ajax route the first time.
I debugged a little further and discovered the MaintenanceModeSubscriber.php of the user module that forces the log out and forces the redirect to the homepage.
I am happy to discover that this issue exists, but I'm skeptical to see that no final decision could be made so far :/
Personally I also believe that this redirect to the front page should not be made, and that displaying the maintenance page is enough and better.
Even though I understand the point of "a user on a public computer that could not log out...", I also find it very frustrating to all our students that they get logged out every time we put the site under maintenance mode. So I would be in favor not to log them out anymore.
I actually think this could be introduced as a config checkbox on the maintenance mode page : "automatically log out unprivileged users who try to access the site while in maintenance mode" (can be safer for users who logged in on a public computer and cannot log out while site is in maintenance)
In the mean time, I think I'm going to try the patch that gets rid of the forced log out and the forced redirect to front page.
Nicolas
Hi,
I'd like to reopen this issue in order not to pollute
🐛
Consider to remove exception handling from search_api_views_data
Needs review
We're still facing this issue when rebuilding cache + someone accesses a page that triggers a call to search_api_views_data().
Today we even faced it with the site under maintenance which makes it really problematic for us.
I'd like to find out how I can prevent this from happening.
From what I understand, the error message "while computing Views data for index" can only be thrown by search_api_views_data().
I also know from the system logs that the error thrown by search_api_views_data() happens when "someone accesses a page" because I have the uri and referer.
In order to prevent this from happening, there are two things I need to understand better:
1/ in which scenario can search_api_views_data() be called? I have tried to debug this locally but for a given url, search_api_views_data() is never called unless the caches are being rebuilt.
2/ how come search_api_views_data() tries to write in cache_config table? Shouldn't the cache_config of search API be written only when we rebuild cache? This would solve the problem…
Thank you for your enlightenment.
Nope I went for a fully custom form that extends FormBase and with a custom route.
In the mean time, I believe the code you wrote for the add payment method form using SetupIntent could be provided as a form that can be manually used by anyone wishing to allow customers to add payment methods from their personal account.
This is something I also implemented earlier on my website and I guess my code is pretty similar to yours and having your code as an existing form inside Commerce Stripe would have been great so I guess it could definitely help others until we have a definitive solution...
Tom that's a very good point.
I did not know that some of Stripe's payment types did not support SetupIntent.
It seems that is THE main reason why this workflow HAD to be implemented for Payment Element…
Andy, I just had a look at the MR and to my surprise you did not modify StripePaymentElementInterface to extend OnsitePaymentGatewayInterface, it still extends OffsitePaymentGatewayInterface.
So your changes are not as big as I feared. I now understand the new workflow you suggest and that it behaves just like with already existing payment methods as soon as we reach review step. Still using the return url and stuff…
It has an advantage over the current workflow: the Commerce Payment Method is created and stored on Drupal's end before the payment is processed. In the current workflow the Commerce Payment Method is created after a successful payment intent, when Stripe calls our return url. This leads to one big problem: if an error occurs when creating the Commerce Payment Method, the code stops and the order never gets completed but the money has actually been paid! I've been facing several situations like that since I started using Payment Element. With your workflow, if an error occurs at Payment Method creation, the customer cannot go further and no money has been involved yet. Which is safer.
I believe there might be one disadvantage though, customers might need to validate their payment method twice. First when they add it on Payment Info step (SetupIntent) and a second time at Review step when the payment is processed (PaymentIntent) especially if their card requires Strong Customer Authentication all the time. Have you faced such a situation when testing?
Also, as I said earlier, I believe some customers (like me) don't want websites to store their credit card details for later. So I'm afraid if the Payment details form is displayed at Payment Info step instead of Review step, some customers might stop right there. But maybe this can be fixed with a checkbox "Save my payment method for later" that would be unchecked by default, and we would make sure not to save the payment method.
There is one important thing we shall not neglect though : we must not break or make weird changes on already working sites. Some sites are already using Payment Element form at review step right now. If the Payment Element form suddenly appears at Payment Info step after upgrading Commerce Stripe, this could be bad. Maybe they added custom code based on the fact that the payment form is on review step and this will suddenly break… so I don't think we can just change the workflow like that.
Sorry I won't have time to test your MR for now.
I definitely think such a rollback in the architecture cannot be pushed just like that and potentially break all sites using Payment Element as an Offsite PG so far. This needs to be discussed IMHO.
Using this commit replaces the review stage payment processing that the payment gateway handled prior to my merge request. Everything still works, but the payment form is now loaded on the payment page rather than the review page. I'm not sure why it functioned that way before, but this approach aligns with the other payment flows (like the base Stripe plugin in this module).
I'm a little skeptical reading this…
A choice has been made to implement Payment Element as an Offsite Payment Gateway with the form loading at review step.
I opened this issue so that the people that made this choice could explain why. Which is why the title of the issue used to be
"Why Payment Element an Offsite instead of Onsite PaymentGateway? How to provide an add-payment-method form?"
No one ever explained this new approach and I had to adapt our website in order to use Payment Element as an Offsite PG. So we now have a production website that uses Payment Element form at review step.
Do I understand correctly that your new code would revert this decision and make Payment Element an Onsite Payment Gateway instead? Which means Payment Element form cannot be used at review step any more, that the user does not have a choice but to add and save a payment method first (with SetupIntent) and can no longer make a one time payment without saving his payment method?
If this gets released, I'm afraid this would break sites like mine that have already used Payment Element as an Offsite payment gateway at review step…
I think a discussion should be held before making breaking changes.
I would like for this issue title to get back to "Why Payment Element an Offsite instead of Onsite PaymentGateway? How to provide an add-payment-method form?"
I'd like for the people who decided to make Payment Element an Offsite PG to explain why they did so. Did they do it on purpose? Does it have advantages? How do they suggest that we provide a way to add a new payment method from the user account? Is switching back to an Onsite PG the only way to do so? What consequences will it have? Should/can we offer the admins to chose between Offsite and Onsite PG with Payment Element?
Thank you Tom for your time and work!
TomTech → credited Nicolas Bouteille → .
it seems patch needs reroll for 1.8
Ok thanks for the help, indeed, looking for accordion in the config helped us find out where we actually used it!
All good then now we can decide what we do!
I just read in 3.5 release notes that "Accordion is now a separate deprecated module to notify people that this shouldn't be used anymore" so definitely don't feel like adding jquery_ui_accordion in composer if not using it...
We're pretty sure we don't use accordion either with field_group.
We too don't feel happy about having to require jquery_ui_accordion in our composer.json.
Any chance update_8303 does not correctly checks whether accordion type is used?
Staying on 3.4.0 until you guys can sort this out.
Nicolas Bouteille → created an issue.
thank you for your work, I can confirm that I had the error with beta2 and upgrading to beta3 fixed it :)
This morning again I got a case where the payment_method had not been correctly associated to the order, leading later on to the error "The "stripe_payment_element" plugin did not specify a "offsite-payment" form class"...
The payment_method had been created, but the payment_method column was null for the order.
You did not tell me what you think about adding
$order->save()
after
$this->createPaymentMethod($payment_method, $payment_details);
$order->set('payment_method', $payment_method);
in StripePaymentElement->onReturn()
any reason it is not done?
Hello,
I am experiencing this as well.
What I don't really understand is why "not catching the exceptions" would prevent this from happening…?
From what I understand, this error happens because of simultaneous calls to search_api_views_data.
In my case as well, this happens when a cache rebuild is in progress while someone, at the same time is trying to access the page of an indexed entity. search_api_views_data seems to be triggered in both situations, leading to simultaneous inserts into cache_config table, leading to deadlock exception and incomplete definition into the cache system, that can be resolved with a new cache rebuild (provided there is no new conflict this time).
I don't understand the solution suggested to not catch exceptions… this will not prevent the deadlock from happening nor will it prevent the data from being incomplete in the cache system?
What do I miss…?
Personaly, I am happy to receive exceptions that let me know something bad happened.
What I would want though, is to understand how I could prevent this from happening.
I don't master yet what search_api_views_data does and when / how often it can be called…
I sometimes make a full cache rebuild on our production website without putting the whole site into maintenance mode. Until recently, I never had this type of error even with Search API in use. But this new deadlock problem with Search API is quite problematic until one can rebuild caches again so I'd like to understand what I should do or not do to prevent this.
Thank you!
Hello,
This error is pretty new to us, but we've experienced it three times in the last month.
Drupal\Core\Database\DatabaseExceptionWrapper while computing Views data for index Leçon: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO "cache_config"
It happened during our last two deployment process during the final cache rebuild, and it also happened this morning during our weekly full cache rebuild.
So I agree this definitely seems to be due to cache rebuilding at the same time than Search API is "computing Views data for index".
What I don't understand is why Search API when "computing Views data for index" would try to insert data into cache_config table, especially data regarding paragraphs fields…
I can also confirm that the error was triggered by DatabaseBackend->doSetMultiple()
I will provide the full stack trace below.
For the record, the search_api_cron is scheduled to run every 15 minutes on our website. This morning the cron ran at 7:30 which is the exact same time the full cache rebuild was triggered. So at first I figured the error could happen when search_api_cron runs while a cache rebuild is in progress.
However, our server is responsible for triggering cron runs every 5 minutes, and we disable drush cron during the whole deployment process. So search_api_cron should not have ran while our final cache rebuild but right after because the very next move is to unlock cron runs. So maybe the problem can happen when search_api_cron is called in the few milliseconds right after the cache rebuild is done…
But there is another clue that makes me think that the error is not necessarily caused by search_api_cron : the request_uri that logged the error is https://www.MY_WEBSITE.com/formation/3d/cinema-4d/cinema-4d-2023-les-mat... which is a basic lesson page
If I am not mistaken, if the error had happened during cron run, the request_uri would have been different isn't it?
I need to spend more time to understand what "computing Views data for index" means, and what action this relates to, so in which situation this error message can happen… I don't know yet if it means Search API is reindexing the index, or simply fetching data in order to deliver it to views…
But the biggest and scariest question that remains is: what's the link between Search API "computing Views data for index" and the insertion of paragraph field data inside cache_config…
For the record, after this Deadlock found error in cache_config happened, we started to have a lot of the following error:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 139 of web/core/lib/Drupal/Core/Entity/EntityTypeManager.php).
Which sounded like the cache_config was not correctly filled. So we rebuilt the cache again and the error was gone.
So it seems Search API process stops the cache rebuild process, and we end up with a bad cache_config table.
Last but not least, it is fair to say that in the last month, we've considerably increased the data stored inside each node "Lesson". The only index we've created with Search API is out Lesson index. It has around 50 000 lessons. However, these new fields and data that we've recently added to a lot of our Lessons, we have not added it yet to the indexed fields...
Stack trace below:
Drupal\Core\Database\DatabaseExceptionWrapper while computing Views data for index Leçon: SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO "cache_config" ("cid", "expire", "created", "tags", "checksum", "data", "serialized") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6), (:db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13), (:db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20), (:db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23, :db_insert_placeholder_24, :db_insert_placeholder_25, :db_insert_placeholder_26, :db_insert_placeholder_27), (:db_insert_placeholder_28, :db_insert_placeholder_29, :db_insert_placeholder_30, :db_insert_placeholder_31, :db_insert_placeholder_32, :db_insert_placeholder_33, :db_insert_placeholder_34), (:db_insert_placeholder_35, :db_insert_placeholder_36, :db_insert_placeholder_37, :db_insert_placeholder_38, :db_insert_placeholder_39, :db_insert_placeholder_40, :db_insert_placeholder_41), (:db_insert_placeholder_42, :db_insert_placeholder_43, :db_insert_placeholder_44, :db_insert_placeholder_45, :db_insert_placeholder_46, :db_insert_placeholder_47, :db_insert_placeholder_48), (:db_insert_placeholder_49, :db_insert_placeholder_50, :db_insert_placeholder_51, :db_insert_placeholder_52, :db_insert_placeholder_53, :db_insert_placeholder_54, :db_insert_placeholder_55), (:db_insert_placeholder_56, :db_insert_placeholder_57, :db_insert_placeholder_58, :db_insert_placeholder_59, :db_insert_placeholder_60, :db_insert_placeholder_61, :db_insert_placeholder_62), (:db_insert_placeholder_63, :db_insert_placeholder_64, :db_insert_placeholder_65, :db_insert_placeholder_66, :db_insert_placeholder_67, :db_insert_placeholder_68, :db_insert_placeholder_69), (:db_insert_placeholder_70, :db_insert_placeholder_71, :db_insert_placeholder_72, :db_insert_placeholder_73, :db_insert_placeholder_74, :db_insert_placeholder_75, :db_insert_placeholder_76), (:db_insert_placeholder_77, :db_insert_placeholder_78, :db_insert_placeholder_79, :db_insert_placeholder_80, :db_insert_placeholder_81, :db_insert_placeholder_82, :db_insert_placeholder_83), (:db_insert_placeholder_84, :db_insert_placeholder_85, :db_insert_placeholder_86, :db_insert_placeholder_87, :db_insert_placeholder_88, :db_insert_placeholder_89, :db_insert_placeholder_90), (:db_insert_placeholder_91, :db_insert_placeholder_92, :db_insert_placeholder_93, :db_insert_placeholder_94, :db_insert_placeholder_95, :db_insert_placeholder_96, :db_insert_placeholder_97) ON DUPLICATE KEY UPDATE "cid" = VALUES("cid"), "expire" = VALUES("expire"), "created" = VALUES("created"), "tags" = VALUES("tags"), "checksum" = VALUES("checksum"), "data" = VALUES("data"), "serialized" = VALUES("serialized"); Array ( [:db_insert_placeholder_0] => core.base_field_override.paragraph.discover_catalog.id [:db_insert_placeholder_1] => -1 [:db_insert_placeholder_2] => 1719811809.205 [:db_insert_placeholder_3] => [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => b:0; [:db_insert_placeholder_6] => 1 [:db_insert_placeholder_7] => core.base_field_override.paragraph.discover_catalog.uuid [:db_insert_placeholder_8] => -1 [:db_insert_placeholder_9] => 1719811809.205 [:db_insert_placeholder_10] => [:db_insert_placeholder_11] => 0 [:db_insert_placeholder_12] => b:0; [:db_insert_placeholder_13] => 1 [:db_insert_placeholder_14] => core.base_field_override.paragraph.discover_catalog.revision_id [:db_insert_placeholder_15] => -1 [:db_insert_placeholder_16] => 1719811809.205 [:db_insert_placeholder_17] => [:db_insert_placeholder_18] => 0 [:db_insert_placeholder_19] => b:0; [:db_insert_placeholder_20] => 1 [:db_insert_placeholder_21] => core.base_field_override.paragraph.discover_catalog.langcode [:db_insert_placeholder_22] => -1 [:db_insert_placeholder_23] => 1719811809.205 [:db_insert_placeholder_24] => [:db_insert_placeholder_25] => 0 [:db_insert_placeholder_26] => b:0; [:db_insert_placeholder_27] => 1 [:db_insert_placeholder_28] => core.base_field_override.paragraph.discover_catalog.type [:db_insert_placeholder_29] => -1 [:db_insert_placeholder_30] => 1719811809.205 [:db_insert_placeholder_31] => [:db_insert_placeholder_32] => 0 [:db_insert_placeholder_33] => b:0; [:db_insert_placeholder_34] => 1 [:db_insert_placeholder_35] => core.base_field_override.paragraph.discover_catalog.status [:db_insert_placeholder_36] => -1 [:db_insert_placeholder_37] => 1719811809.205 [:db_insert_placeholder_38] => [:db_insert_placeholder_39] => 0 [:db_insert_placeholder_40] => b:0; [:db_insert_placeholder_41] => 1 [:db_insert_placeholder_42] => core.base_field_override.paragraph.discover_catalog.created [:db_insert_placeholder_43] => -1 [:db_insert_placeholder_44] => 1719811809.205 [:db_insert_placeholder_45] => [:db_insert_placeholder_46] => 0 [:db_insert_placeholder_47] => b:0; [:db_insert_placeholder_48] => 1 [:db_insert_placeholder_49] => core.base_field_override.paragraph.discover_catalog.parent_id [:db_insert_placeholder_50] => -1 [:db_insert_placeholder_51] => 1719811809.205 [:db_insert_placeholder_52] => [:db_insert_placeholder_53] => 0 [:db_insert_placeholder_54] => b:0; [:db_insert_placeholder_55] => 1 [:db_insert_placeholder_56] => core.base_field_override.paragraph.discover_catalog.parent_type [:db_insert_placeholder_57] => -1 [:db_insert_placeholder_58] => 1719811809.205 [:db_insert_placeholder_59] => [:db_insert_placeholder_60] => 0 [:db_insert_placeholder_61] => b:0; [:db_insert_placeholder_62] => 1 [:db_insert_placeholder_63] => core.base_field_override.paragraph.discover_catalog.parent_field_name [:db_insert_placeholder_64] => -1 [:db_insert_placeholder_65] => 1719811809.205 [:db_insert_placeholder_66] => [:db_insert_placeholder_67] => 0 [:db_insert_placeholder_68] => b:0; [:db_insert_placeholder_69] => 1 [:db_insert_placeholder_70] => core.base_field_override.paragraph.discover_catalog.behavior_settings [:db_insert_placeholder_71] => -1 [:db_insert_placeholder_72] => 1719811809.205 [:db_insert_placeholder_73] => [:db_insert_placeholder_74] => 0 [:db_insert_placeholder_75] => b:0; [:db_insert_placeholder_76] => 1 [:db_insert_placeholder_77] => core.base_field_override.paragraph.discover_catalog.default_langcode [:db_insert_placeholder_78] => -1 [:db_insert_placeholder_79] => 1719811809.205 [:db_insert_placeholder_80] => [:db_insert_placeholder_81] => 0 [:db_insert_placeholder_82] => b:0; [:db_insert_placeholder_83] => 1 [:db_insert_placeholder_84] => core.base_field_override.paragraph.discover_catalog.revision_default [:db_insert_placeholder_85] => -1 [:db_insert_placeholder_86] => 1719811809.205 [:db_insert_placeholder_87] => [:db_insert_placeholder_88] => 0 [:db_insert_placeholder_89] => b:0; [:db_insert_placeholder_90] => 1 [:db_insert_placeholder_91] => core.base_field_override.paragraph.discover_catalog.revision_translation_affected [:db_insert_placeholder_92] => -1 [:db_insert_placeholder_93] => 1719811809.205 [:db_insert_placeholder_94] => [:db_insert_placeholder_95] => 0 [:db_insert_placeholder_96] => b:0; [:db_insert_placeholder_97] => 1 ) in Drupal\Core\Cache\DatabaseBackend->doSetMultiple() (line 283 of /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php).
Détails
Array
(
[%type] => Drupal\Core\Database\DatabaseExceptionWrapper
[@message] => SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO "cache_config" ("cid", "expire", "created", "tags", "checksum", "data", "serialized") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6), (:db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13), (:db_insert_placeholder_14, :db_insert_placeholder_15, :db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20), (:db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23, :db_insert_placeholder_24, :db_insert_placeholder_25, :db_insert_placeholder_26, :db_insert_placeholder_27), (:db_insert_placeholder_28, :db_insert_placeholder_29, :db_insert_placeholder_30, :db_insert_placeholder_31, :db_insert_placeholder_32, :db_insert_placeholder_33, :db_insert_placeholder_34), (:db_insert_placeholder_35, :db_insert_placeholder_36, :db_insert_placeholder_37, :db_insert_placeholder_38, :db_insert_placeholder_39, :db_insert_placeholder_40, :db_insert_placeholder_41), (:db_insert_placeholder_42, :db_insert_placeholder_43, :db_insert_placeholder_44, :db_insert_placeholder_45, :db_insert_placeholder_46, :db_insert_placeholder_47, :db_insert_placeholder_48), (:db_insert_placeholder_49, :db_insert_placeholder_50, :db_insert_placeholder_51, :db_insert_placeholder_52, :db_insert_placeholder_53, :db_insert_placeholder_54, :db_insert_placeholder_55), (:db_insert_placeholder_56, :db_insert_placeholder_57, :db_insert_placeholder_58, :db_insert_placeholder_59, :db_insert_placeholder_60, :db_insert_placeholder_61, :db_insert_placeholder_62), (:db_insert_placeholder_63, :db_insert_placeholder_64, :db_insert_placeholder_65, :db_insert_placeholder_66, :db_insert_placeholder_67, :db_insert_placeholder_68, :db_insert_placeholder_69), (:db_insert_placeholder_70, :db_insert_placeholder_71, :db_insert_placeholder_72, :db_insert_placeholder_73, :db_insert_placeholder_74, :db_insert_placeholder_75, :db_insert_placeholder_76), (:db_insert_placeholder_77, :db_insert_placeholder_78, :db_insert_placeholder_79, :db_insert_placeholder_80, :db_insert_placeholder_81, :db_insert_placeholder_82, :db_insert_placeholder_83), (:db_insert_placeholder_84, :db_insert_placeholder_85, :db_insert_placeholder_86, :db_insert_placeholder_87, :db_insert_placeholder_88, :db_insert_placeholder_89, :db_insert_placeholder_90), (:db_insert_placeholder_91, :db_insert_placeholder_92, :db_insert_placeholder_93, :db_insert_placeholder_94, :db_insert_placeholder_95, :db_insert_placeholder_96, :db_insert_placeholder_97) ON DUPLICATE KEY UPDATE "cid" = VALUES("cid"), "expire" = VALUES("expire"), "created" = VALUES("created"), "tags" = VALUES("tags"), "checksum" = VALUES("checksum"), "data" = VALUES("data"), "serialized" = VALUES("serialized"); Array
(
[:db_insert_placeholder_0] => core.base_field_override.paragraph.discover_catalog.id
[:db_insert_placeholder_1] => -1
[:db_insert_placeholder_2] => 1719811809.205
[:db_insert_placeholder_3] =>
[:db_insert_placeholder_4] => 0
[:db_insert_placeholder_5] => b:0;
[:db_insert_placeholder_6] => 1
[:db_insert_placeholder_7] => core.base_field_override.paragraph.discover_catalog.uuid
[:db_insert_placeholder_8] => -1
[:db_insert_placeholder_9] => 1719811809.205
[:db_insert_placeholder_10] =>
[:db_insert_placeholder_11] => 0
[:db_insert_placeholder_12] => b:0;
[:db_insert_placeholder_13] => 1
[:db_insert_placeholder_14] => core.base_field_override.paragraph.discover_catalog.revision_id
[:db_insert_placeholder_15] => -1
[:db_insert_placeholder_16] => 1719811809.205
[:db_insert_placeholder_17] =>
[:db_insert_placeholder_18] => 0
[:db_insert_placeholder_19] => b:0;
[:db_insert_placeholder_20] => 1
[:db_insert_placeholder_21] => core.base_field_override.paragraph.discover_catalog.langcode
[:db_insert_placeholder_22] => -1
[:db_insert_placeholder_23] => 1719811809.205
[:db_insert_placeholder_24] =>
[:db_insert_placeholder_25] => 0
[:db_insert_placeholder_26] => b:0;
[:db_insert_placeholder_27] => 1
[:db_insert_placeholder_28] => core.base_field_override.paragraph.discover_catalog.type
[:db_insert_placeholder_29] => -1
[:db_insert_placeholder_30] => 1719811809.205
[:db_insert_placeholder_31] =>
[:db_insert_placeholder_32] => 0
[:db_insert_placeholder_33] => b:0;
[:db_insert_placeholder_34] => 1
[:db_insert_placeholder_35] => core.base_field_override.paragraph.discover_catalog.status
[:db_insert_placeholder_36] => -1
[:db_insert_placeholder_37] => 1719811809.205
[:db_insert_placeholder_38] =>
[:db_insert_placeholder_39] => 0
[:db_insert_placeholder_40] => b:0;
[:db_insert_placeholder_41] => 1
[:db_insert_placeholder_42] => core.base_field_override.paragraph.discover_catalog.created
[:db_insert_placeholder_43] => -1
[:db_insert_placeholder_44] => 1719811809.205
[:db_insert_placeholder_45] =>
[:db_insert_placeholder_46] => 0
[:db_insert_placeholder_47] => b:0;
[:db_insert_placeholder_48] => 1
[:db_insert_placeholder_49] => core.base_field_override.paragraph.discover_catalog.parent_id
[:db_insert_placeholder_50] => -1
[:db_insert_placeholder_51] => 1719811809.205
[:db_insert_placeholder_52] =>
[:db_insert_placeholder_53] => 0
[:db_insert_placeholder_54] => b:0;
[:db_insert_placeholder_55] => 1
[:db_insert_placeholder_56] => core.base_field_override.paragraph.discover_catalog.parent_type
[:db_insert_placeholder_57] => -1
[:db_insert_placeholder_58] => 1719811809.205
[:db_insert_placeholder_59] =>
[:db_insert_placeholder_60] => 0
[:db_insert_placeholder_61] => b:0;
[:db_insert_placeholder_62] => 1
[:db_insert_placeholder_63] => core.base_field_override.paragraph.discover_catalog.parent_field_name
[:db_insert_placeholder_64] => -1
[:db_insert_placeholder_65] => 1719811809.205
[:db_insert_placeholder_66] =>
[:db_insert_placeholder_67] => 0
[:db_insert_placeholder_68] => b:0;
[:db_insert_placeholder_69] => 1
[:db_insert_placeholder_70] => core.base_field_override.paragraph.discover_catalog.behavior_settings
[:db_insert_placeholder_71] => -1
[:db_insert_placeholder_72] => 1719811809.205
[:db_insert_placeholder_73] =>
[:db_insert_placeholder_74] => 0
[:db_insert_placeholder_75] => b:0;
[:db_insert_placeholder_76] => 1
[:db_insert_placeholder_77] => core.base_field_override.paragraph.discover_catalog.default_langcode
[:db_insert_placeholder_78] => -1
[:db_insert_placeholder_79] => 1719811809.205
[:db_insert_placeholder_80] =>
[:db_insert_placeholder_81] => 0
[:db_insert_placeholder_82] => b:0;
[:db_insert_placeholder_83] => 1
[:db_insert_placeholder_84] => core.base_field_override.paragraph.discover_catalog.revision_default
[:db_insert_placeholder_85] => -1
[:db_insert_placeholder_86] => 1719811809.205
[:db_insert_placeholder_87] =>
[:db_insert_placeholder_88] => 0
[:db_insert_placeholder_89] => b:0;
[:db_insert_placeholder_90] => 1
[:db_insert_placeholder_91] => core.base_field_override.paragraph.discover_catalog.revision_translation_affected
[:db_insert_placeholder_92] => -1
[:db_insert_placeholder_93] => 1719811809.205
[:db_insert_placeholder_94] =>
[:db_insert_placeholder_95] => 0
[:db_insert_placeholder_96] => b:0;
[:db_insert_placeholder_97] => 1
)
[%function] => Drupal\Core\Cache\DatabaseBackend->doSetMultiple()
[%file] => /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php
[%line] => 283
[severity_level] => 3
[@backtrace_string] => #0 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Database/Query/Upsert.php(119): Drupal\mysql\Driver\Database\mysql\ExceptionHandler->handleExecutionException()
#1 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php(283): Drupal\Core\Database\Query\Upsert->execute()
#2 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Cache/DatabaseBackend.php(198): Drupal\Core\Cache\DatabaseBackend->doSetMultiple()
#3 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Config/CachedStorage.php(105): Drupal\Core\Cache\DatabaseBackend->setMultiple()
#4 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Config/ConfigFactory.php(165): Drupal\Core\Config\CachedStorage->readMultiple()
#5 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Config/ConfigFactory.php(136): Drupal\Core\Config\ConfigFactory->doLoadMultiple()
#6 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php(181): Drupal\Core\Config\ConfigFactory->loadMultiple()
#7 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(312): Drupal\Core\Config\Entity\ConfigEntityStorage->doLoadMultiple()
#8 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityFieldManager.php(395): Drupal\Core\Entity\EntityStorageBase->loadMultiple()
#9 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityFieldManager.php(353): Drupal\Core\Entity\EntityFieldManager->buildBundleFieldDefinitions()
#10 /PATH_TO_PROJECT/web/modules/contrib/search_api/src/Plugin/search_api/processor/ReverseEntityReferences.php(383): Drupal\Core\Entity\EntityFieldManager->getFieldDefinitions()
#11 /PATH_TO_PROJECT/web/modules/contrib/search_api/src/Plugin/search_api/processor/ReverseEntityReferences.php(235): Drupal\search_api\Plugin\search_api\processor\ReverseEntityReferences->getEntityReferences()
#12 /PATH_TO_PROJECT/web/modules/contrib/search_api/src/Entity/Index.php(845): Drupal\search_api\Plugin\search_api\processor\ReverseEntityReferences->getPropertyDefinitions()
#13 /PATH_TO_PROJECT/web/modules/contrib/search_api/src/Item/Field.php(475): Drupal\search_api\Entity\Index->getPropertyDefinitions()
#14 /PATH_TO_PROJECT/web/modules/contrib/search_api/search_api.views.inc(206): Drupal\search_api\Item\Field->getDataDefinition()
#15 /PATH_TO_PROJECT/web/modules/contrib/search_api/search_api.views.inc(63): _search_api_views_get_handlers()
#16 /PATH_TO_PROJECT/web/core/modules/views/src/ViewsData.php(228): search_api_views_data()
#17 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(388): Drupal\views\ViewsData->Drupal\views\{closure}()
#18 /PATH_TO_PROJECT/web/core/modules/views/src/ViewsData.php(236): Drupal\Core\Extension\ModuleHandler->invokeAllWith()
#19 /PATH_TO_PROJECT/web/core/modules/views/src/ViewsData.php(151): Drupal\views\ViewsData->getData()
#20 /PATH_TO_PROJECT/web/core/modules/views/src/Plugin/Derivative/ViewsEntityRow.php(94): Drupal\views\ViewsData->get()
#21 /PATH_TO_PROJECT/web/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(101): Drupal\views\Plugin\Derivative\ViewsEntityRow->getDerivativeDefinitions()
#22 /PATH_TO_PROJECT/web/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.php(87): Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDerivatives()
#23 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(323): Drupal\Component\Plugin\Discovery\DerivativeDiscoveryDecorator->getDefinitions()
#24 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(205): Drupal\Core\Plugin\DefaultPluginManager->findDefinitions()
#25 /PATH_TO_PROJECT/web/core/modules/views/views.module(146): Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
#26 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Theme/Registry.php(479): views_theme()
#27 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Theme/Registry.php(372): Drupal\Core\Theme\Registry->processExtension()
#28 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(388): Drupal\Core\Theme\Registry->Drupal\Core\Theme\{closure}()
#29 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Theme/Registry.php(373): Drupal\Core\Extension\ModuleHandler->invokeAllWith()
#30 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Theme/Registry.php(255): Drupal\Core\Theme\Registry->build()
#31 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Utility/ThemeRegistry.php(88): Drupal\Core\Theme\Registry->get()
#32 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Utility/ThemeRegistry.php(69): Drupal\Core\Utility\ThemeRegistry->initializeRegistry()
#33 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Theme/Registry.php(291): Drupal\Core\Utility\ThemeRegistry->__construct()
#34 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(193): Drupal\Core\Theme\Registry->getRuntime()
#35 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(157): Drupal\Core\Entity\EntityViewBuilder->getBuildDefaults()
#36 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/EntityViewBuilder.php(123): Drupal\Core\Entity\EntityViewBuilder->viewMultiple()
#37 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php(134): Drupal\Core\Entity\EntityViewBuilder->view()
#38 [internal function]: Drupal\Core\Entity\Controller\EntityViewController->view()
#39 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array()
#40 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/Render/Renderer.php(627): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#41 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext()
#42 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext()
#43 /PATH_TO_PROJECT/vendor/symfony/http-kernel/HttpKernel.php(181): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#44 /PATH_TO_PROJECT/vendor/symfony/http-kernel/HttpKernel.php(76): Symfony\Component\HttpKernel\HttpKernel->handleRaw()
#45 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle()
#46 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle()
#47 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/ContentLength.php(28): Drupal\Core\StackMiddleware\KernelPreHandle->handle()
#48 /PATH_TO_PROJECT/web/core/modules/big_pipe/src/StackMiddleware/ContentLength.php(32): Drupal\Core\StackMiddleware\ContentLength->handle()
#49 /PATH_TO_PROJECT/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(191): Drupal\big_pipe\StackMiddleware\ContentLength->handle()
#50 /PATH_TO_PROJECT/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(128): Drupal\page_cache\StackMiddleware\PageCache->fetch()
#51 /PATH_TO_PROJECT/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(82): Drupal\page_cache\StackMiddleware\PageCache->lookup()
#52 /PATH_TO_PROJECT/web/core/modules/ban/src/BanMiddleware.php(50): Drupal\page_cache\StackMiddleware\PageCache->handle()
#53 /PATH_TO_PROJECT/web/modules/custom/MY_CUSTOM_MODULE/src/BanMiddleware.php(97): Drupal\ban\BanMiddleware->handle()
#54 /PATH_TO_PROJECT/web/modules/contrib/shield/src/ShieldMiddleware.php(270): Drupal\MY_CUSTOM_MODULE\BanMiddleware->handle()
#55 /PATH_TO_PROJECT/web/modules/contrib/shield/src/ShieldMiddleware.php(137): Drupal\shield\ShieldMiddleware->bypass()
#56 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Drupal\shield\ShieldMiddleware->handle()
#57 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle()
#58 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/AjaxPageState.php(36): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle()
#59 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/StackMiddleware/StackedHttpKernel.php(51): Drupal\Core\StackMiddleware\AjaxPageState->handle()
#60 /PATH_TO_PROJECT/web/core/lib/Drupal/Core/DrupalKernel.php(704): Drupal\Core\StackMiddleware\StackedHttpKernel->handle()
#61 /PATH_TO_PROJECT/web/index.php(19): Drupal\Core\DrupalKernel->handle()
#62 {main}
[%index] => Leçon
[channel] => search_api
[link] =>
[uid] => 0
[request_uri] => https://www.MY_WEBSITE.com/formation/3d/cinema-4d/cinema-4d-2023-les-materiaux
[referer] =>
[ip] => 40.77.167.254
[timestamp] => 1719811809
)
thanks for the update!
UPDATE : in the end the number of errors that appear in the logs seems to clearly decrease, so this clearly seems to be due to Drupal 9 pages being kept open on an old tab that requests CSS and JS files the old way not compatible with D10. So not much to do here, the errors should fade away progressively...
Thank you the patch works great for us on Commerce 2.38.
RTBC!
After some digging it looks like adding (and expecting) parameters theme, delta or even language in the aggregated file url was not in place in Drupal 9.
From what I could figure out, creating the aggregated file url is done in core/lib/Drupal/Core/Asset/CssCollectionOptimizer->optimize() for D9 and now in core/lib/Drupal/Core/Asset/CssCollectionOptimizerLazy->optimize() for D10
Only the D10 version adds these parameters.
Checking for parameters theme, delta and language is done in core/modules/system/src/Controller/AssetControllerBase->deliver() in D10 and no similar code can be found for D9
It seems this was delt in D9 by core/lib/Drupal/Core/Asset/AssetResolver->getCssAssets() which first line is actually automatically retrieving the active theme…
$theme_info = $this->themeManager->getActiveTheme();
So it seems normal that users visiting pages generated by D9 in the past trigger these errors in the new D10 code.
But what's weird is it seems to me that these errors are triggered by users who have been visiting the site multiple times since we've upgraded to D10 so I would definitely expect these errors to progressively disappear… yet they don't seem to... but it's hard to be certain...
But when I visit the referer myself I cannot reproduce the error
Could have been you're totally right, but these are files requested by logged in users with legit IP and JS files loaded from a legit referer url :/
We've had this error showing in our logs since we upgraded from D9 to D10 last week.
I thought it was due to old users having old pages opened and requesting old aggregated CSS / JS… and that it would progressively disappear. But it seems that it could be increasing actually..
I see this error for aggregated CSS and JS files, but I also have it a lot for the following requested files:
js/lazy.commerceui-wc.utils.js
js/lazy.commerceui-wc.themes.js
js/lazy.commerceui-wc-buying-options.js
js/lazy.commerceui-react.price-history.js
js/lazy.commerceui-react.price-activity.js
js/lazy.commerceui-react.theme.js
js/lazy.commerceui-react.layout.js
js/470.js
js/273.js
js/763.js
js/256.js
I have no idea what these are about. We have Drupal Commerce installed, but I can't find any occurence of these words anywhere in the source code… this is really weird.
I have two new information about this:
First, Stripe Payment Element not loading/displaying can be because of js.stripe.com/v3 being blocked.
In our case OneTrust cookie consent tool was blocking Stripe JS until we set this cookie as Strictly Necessary.
Second, when we face the error
The "stripe_payment_element" plugin did not specify a "offsite-payment" form class
we can conclude that we've reached the line
$pane_form['offsite_payment'] = $inline_form->buildInlineForm($pane_form['offsite_payment'], $form_state);
in PaymentProcess.php
However, we are not supposed to reach this line with PaymentElement as a Payment Gateway inside PaymentProcess.php
we are supposed to enter the preceding if statement:
if ($payment_gateway_plugin instanceof SupportsStoredPaymentMethodsInterface && !$this->order->get('payment_method')->isEmpty()) {
$payment->payment_method = $this->order->get('payment_method')->entity;
$payment_gateway_plugin->createPayment($payment, $this->configuration['capture']);
$this->checkoutFlow->redirectToStep($next_step_id);
}
Which directly redirects to the next step, so the rest of the code should not be executed...
So chances are we did not enter it because $this->order->get('payment_method')->isEmpty()
So you can start digging why the order does not have a payment_method at that moment... which is not supposed to happen
BTW, I have noticed in StripePaymentElement->onReturn()
there is no $order->save() after
$this->createPaymentMethod($payment_method, $payment_details);
$order->set('payment_method', $payment_method);
if nothing goes wrong the order eventually gets saved later on with the payment_method associated, but in our case it seems something went wrong (it seems to user got logged out...), so the payment method was actually created but never associated with the order, so we were facing the error The "stripe_payment_element" plugin did not specify a "offsite-payment" form class because the order did not have any payment_method attached when reaching "payment" step.
I had to manually unlock the order + set the payment_method ID in the payment_method column of the order directly in the database and delete the order entry in cache_entity to unblock the situation...
Ok I have spotted that $storage->load() has been overridden in ConfigPagesStorage to actually call ConfigPages::config($id); unless the $id is numeric...
for that reason, calling $storage->load() inside ConfigPages::config would create an infinite loop
also I thought calling $storage->load('my_config_page') directly would be more efficient than using ConfigPagesLoaderService or ConfigPages::config because they use loadByProperties, but in the end all those three methods call loadByProperties in the end...
Hello,
In the code of the PR 34, I see the $cache variable lives in the config() function.
Am I right to assume that the config page entity will be "cached" only during a single session ? Or maybe even a single request ? And that it only relieves the site when too many loads are done for a single request, but has no effect if multiple visitors load the config page in a short time?
In my website, I would like to check a config page's value on every single request made to the website.
The config page's value will not change frequently at all. So loading the config page entity on every page request or every session is overkill.
Which is why I am considering caching the data I need and removing it from cache whenever the config page is updated.
But I was wondering… do I really need to do this myself or config page / Drupal is already caching stuff?
So I stumbled upon this issue and it looks like no caching is done...
As mentioned above, no cache is made with loadByProperties() unlike load().
So as suggested above, shouldn't we use load() when context is null? Or is it because when context is null, we still need to fetch and specify the default context that we cannot do that?
BTW, on this documentation page :
https://www.drupal.org/docs/contributed-modules/config-pages/usage-of-co... →
The third option is to get a config page via storage manager:
$storage = \Drupal::entityTypeManager()->getStorage('config_pages');
$entity = $storage->load($config_page_machine_name);
this does mention the context... so is the context not that important, or is the documentation incomplete?
If we cannot use load() instead of loadByProperties(), what do you think of caching the config page entity using cache_set() and remove it when it is updated? Or is it too heavy and we'd better only store simple values that we need to access frequently?
I had to uninstall module so that composer accepts that I make it to Drupal 10 and now I cannot install again. Any chance we can have a new release compatible with D10?
thank you :)
Hello, patch #3 applies correctly, makes Upgrade Status happy, and module seems to be working well.
patch #10 however did not apply correctly for me on 8.x-1.3-alpha4
it works thanks :)
thanks a lot! will get back to you when I have done so.
thank you for the tip, it works much better indeed!
Starting my D10 upgrade right now, and using this module. Any updates?
I am also interested in having this information on my website: how many users/visitors are currently online. I can't find a solution anywhere. Any idea?
Yes it happens when PaymentElement JS does not load correctly and customers can submit the Review step form without actually paying. Read above for more details. I have not been able to understand how / when this happens though.
Nicolas Bouteille → created an issue.
In this piece of code, we also retrieve the order's payment_method
$payment_method = $this->order->get('payment_method')->entity;
but we forget to check if the $payment_method is not null before using it
$payment_method->getRemoteId();
the only thing we check is if
!$this->order->get('payment_method')->isEmpty()
but the entity_reference field can have a target_id that references the id of a payment_method that has been deleted since. Leading to $payment_method object being null.
if I'm not mistaken
assert($payment_method instanceof PaymentMethodInterface);
is only here to help for code autocomplete, but does not stop the code execution.
So $payment_method->getRemoteId(); throws an Exception or a PHP error "trying to call ->getRemoteId() on null..."
Here is what the if statement looks like on our website now to solve this :
if (
!$this->order->get('payment_method')->isEmpty()
&& $payment_method = $this->order->get('payment_method')->entity
) {Hi,
I discovered a use case when this can happen. A client sent me an screenshot of the review step on her iPad. PaymentElement did not load correctly so it was missing. (could not reproduce bug on 2 iPads btw)
There was only two checkboxes of CGVs that we enforce users to check before they can submit, and the "Pay and complete purchase" button.
Because an error had occured while mounting PaymentElement, the JS code that is supposed to override the form submit (prevent default etc) never got executed.
So when she submitted the form, she went the regular Commerce way of submitting the Review step and then going to the PaymentProcess step. Leading to this specific error.
In order to prevent this from happening in the future :
1/ we disable the submit button in case error happen that prevent our code from overriding submit in JS
here
if (!drupalSettings.commerceStripePaymentElement || !drupalSettings.commerceStripePaymentElement.publishableKey) {
myCustomLogErrorCode() // custom code to log the error and send us an alert mail
Drupal.commerceStripe.displayError(drupalSettings.utils.CUSTOM_ERROR_MESSAGE);
$("form.my-custom-checkout-flow-form .form-actions input.form-submit").prop("disabled", true);
return;
}and here we actually added a try catch :
try {
// Create an instance of Stripe Elements.
var elements = stripe.elements(settings.createElementsOptions);
var paymentElement = elements.create('payment', settings.paymentElementOptions);
paymentElement.mount('#' + settings.elementId);
} catch (e) {
myCustomLogErrorCode() // custom code to log the error and send us an alert mail
Drupal.commerceStripe.displayError(drupalSettings.utils.CUSTOM_ERROR_MESSAGE);
$("form.my-custom-checkout-flow-form .form-actions input.form-submit").prop("disabled", true);
return;
}
What we also did, because in our case even when confirming payment with an existing payment method we always use the return_url and never submit the form in JS, we also added a custom check in validateForm of our CheckoutFlow plugin that prevents the review step from ever being submitted. The only way to pass this step in our case is throught the return_url passed to stripe.confirmPayment()
public function validateForm(array &$form, FormStateInterface $form_state) {
parent::validateForm($form, $form_state);
if ($form['#step_id'] === 'review') {
myCustomLogErrorCode() // custom code to log the error and send us an alert mail
$form_state->setError($form, CustomUtils::CUSTOM_ERROR_MESSAGE);
}
}Updating the description to make it clear that using Paypal was done via custom code.
Your right. Let me update the description to clarify the situation.
Well, I know this work has already started. Some might already be available on the dev branch you should go check. I think there are other open issues dealing with this more generic need. This one's goal is only to not throw a payment error once we know the payment was successful.
Yes indeed, the job of onReturn right now is only to store the Commerce Payment Method.
But even if you skip that part, you still need to let PaymentCheckoutController->ReturnPage redirect to PaymentProcess step so that the Commerce Payment is created and the order completed.
$payment_gateway_plugin->onReturn($order, $request);
$redirect_step_id = $checkout_flow_plugin->getNextStepId($step_id);As a quick workaround, I would personnally disable any payment type that you have not tested yet. You can do that by replacing in StripePaymentElement.php
'automatic_payment_methods' => [
'enabled' => TRUE,
],with something like:
'payment_method_types' => ['card', 'paypal'],
Then you can take the time to test thoroughly any payment type you want to add. Be carefull, some payment types such as bank wire for example could be different than the others. I think they might have a "processing" status at first and will only have a "succeeded" status later on when the bank transfert has been received… so you should definitely take the time to test any payment type before you enable them.
PaymentElement is a great tool to ease the possibility to use multiple payment types, but I don't think it's plug and play. We need to be careful to a lot a details.
Thank you for your feedback.
Indeed, the only moment it would have taken 20 minutes would have been when updating your module. Because at that moment the node_access table is empty and 68k node permission need to be created. But I did not want to have our website down for that long so I uninstalled.
Hello everyone,
My website currently still uses the REPEATABLE READ transaction isolation level.
I would like to switch to READ COMMITED.
I read this documentation :
https://www.drupal.org/docs/getting-started/system-requirements/setting-... →
From what I understand, for best performance "The preferred way to change the transaction isolation level" is to set the Global value on the server side with
SET GLOBAL transaction_isolation='READ-COMMITTED';
However, if I'm not mistaken, this global variable is shared between all databases of the server. Plus updating this variable needs to be done with a super user.
Where I work, we have multiple databases of different websites on the same server. Plus my database user cannot update this variable. So if I decided to update this variable globally, I would need to coordinate with all other websites and ask IT to do it for me.
For that reason, updating it in settings.php would be easier for me.
In the documentation, the syntax presented in settings.php is the following:
'init_commands' => [
'isolation_level' => 'SET SESSION tx_isolation=\'READ-COMMITTED\'',
],BUT there's a warning: "Adding the setting of the transaction isolation level to the init commands in the settings.php file has the disadvantage that on every page request the transaction isolation level is set. That is an extra database call for every page request!"
Since my goal here is to improve performances, after reading this, configuring this in settings.php was not an option for me anymore.
But then I realized that I actually read here that "The transaction isolation level can be set in the database settings array in setttings.php" and that "Drupal now sets the READ COMMITTED transaction isolation level by default for new installs on MySQL."
So I wondered… if setting the isolation level inside settings.php is bad for performance because it redefines the level for each db request, how come Drupal now sets it by default inside settings.php??
I actually created a brand new Drupal website recently, and it uses indeed READ COMMITED. So I went to the settings.php but to my surprise, the way the isolation level is defined is not
'init_commands' => [
'isolation_level' => 'SET SESSION tx_isolation=\'READ-COMMITTED\'',
],
but simply :
'isolation_level' => 'READ COMMITTED',
Here are my many questions:
Is the documentation outdated?
About this new syntax ('isolation_level' => 'READ COMMITTED',): does it have better performance than the 'init_commands' SET SESSION tx_isolation=\'READ-COMMITTED\ syntax? Or is it just a simpler way to write it but in the end it does the exact same thing and has the same drawback of setting the isolation level for each session?
How bad is it to actually set the transaction isolation level on every page request?
Should I do all I can to set the transaction isolation level globally on my database server and avoid to set it in settings.php whenever possible? Or I should not care about this because I won't see a difference...?
In settings.php, is the parameter 'isolation_level' => 'READ COMMITTED', added to every new Drupal site? Or is there a check that is made to the server at site creation and 'isolation_level' => 'READ COMMITTED' is added to settings.php only if the global isolation level of the server is not already READ COMMITTED?
What happens if the parameter 'isolation_level' => 'READ COMMITTED' is still present on settings.php but the DB serveur global isolation level is updated to READ COMMITED later on. Will the isolation level still be redefined for each session no matter what, for nothing? In other words, once we set READ COMMITED globally on server side, should we remove the 'isolation_level' => 'READ COMMITTED' line from settings.php to improve performances?
Thank you in advance for your feedback :)
Nicolas Bouteille → created an issue.
Hi,
We have Payment Element enabled for 3 days now in production.
A lot of payments have successfully passed.
I just saw this same error pass right now for a customer
IntegrationError: Invalid value for stripe.confirmPayment(): elements should have a mounted Payment Element or Express Checkout Element.
At review step.
This error came along with another error
The "stripe_payment_element" plugin did not specify a "offsite-payment" form class
Coming from
commerce/modules/payment/src/Plugin/Commerce/InlineForm/PaymentGatewayForm.php(103): Drupal\Core\Plugin\PluginFormFactory->createInstance()
commerce/modules/payment/src/Plugin/Commerce/CheckoutPane/PaymentProcess.php(218)
$pane_form['offsite_payment'] = $inline_form->buildInlineForm($pane_form['offsite_payment'], $form_state);
seems to be the line that triggers the error
I have no clue what the customer did or faced to generate this error.
I am surprised the customer could access the "payment process" step since the PaymentIntent was never confirmed...
Did he try to hack the checkout flow and submit the pane manually in js?
I think on our website I am actually going to completely remove the code that tries to attach the billing address to the payment method after a successful payment. Now that I know that this can actually cause an error inside doCreatePaymentMethod() which will prevent the payment method creation inside createPaymentMethod(), I think I'd better not even try to attach billing details to the PM at all.
After all, the client could have used a credit card which is not under her name... so trying to attach her address to a credit card under someone else's name could cause an error. Also at this step, the payment is already successful and the payment method already configured for off_session future payments. So what good could bring us to attach the address to the PM? I don't see any positive side, only a potential risk of error.
What do you think?
Hi,
We now have Payment Element live in production! Yeah!
Today we got an error when StripePaymentElement->doCreatePaymentMethod() tries to send the billing details to the payment method
PaymentMethod::update($stripe_payment_method_id, ['billing_details' => $payment_method_data]);
the request generated this error:
card_declined
The card returned a decline code of invalid_account.The card, or account the card is connected to, is invalid.
The customer may need to contact their card issuer to check that the card is working correctly.
To learn more about why some payments fail and what you can do to decrease your decline rate visit our guide on understanding declines and failed payments: https://stripe.com/docs/declines
Because of this error that was converted to a DeclineException by ErrorHelper->handleException, the order could not be placed.
Just because we could not attach the billing details to the payment method although the payment was successful...
Thanks to my custom try catch, the customer did not receive the error message from PaymentCheckoutController->returnPage
"Payment failed at the payment server. Please review your information and try again."
Instead, she saw a message letting her know that an error had occurred after the payment might have been successful so she should wait for us to contact her. Which we did.
Now I do believe failing to attach the billing details to the payment method after a successful payment should not prevent the correct finalization of the order afterwards. So I am keeping my try catch, but now I don't even re-throw a generic exception anymore, I only log the error with full stack trace so that I am alerted and I can resolve the situation, but I don't stop the order to be place no more and don't show an error message to the customer just because the billing address could not be attached to the PM.
I think any error during the whole "saving the payment method locally" step should not block the finalization of the order.
With Card Element, we did not have such a situation because the PM was always created before the payment was confirmed... I think we should think about this carefully for Payment Element that creates PM after successful payment only...
What do you think?
I actually don't know how to reproduce, plus I know use StripePaymentElement on my website...
So I won't be able to RTBC sorry
Hi Vitaliy,
I think this comment of yours is the situation you were thinking about:
https://www.drupal.org/project/commerce_stripe/issues/3392413#comment-15...
✨
Add the option to not setup payment methods for future usage
Fixed
However, I believe that the problem I am raising here should not just be fixed because there is a bug right now somewhere. I don't think you need to waste time verifying if you find a way to produce a PaymentGatewayException there or not. The fact that it actually can or could happen is enough IMHO. Right now it can happen because in doCreatePaymentMethod we call $stripe_payment_method->attach(['customer' => $customer_id]); and because doCreatePaymentMethod transfers ApiErrorExceptions to ErrorHelper::handleException($e);
But even if this code was not here today, it could be tomorrow...
The protection layer I am suggesting we add should be added not matter what IMHO in case a PaymentGatewayException is someday thrown after we know the payment was successful on Stripe's end.
So if tomorrow someone adds some code that generates a PaymentGatewayException after the payment was successful, we'll still be able to let the customer know that something went wrong but they should not try again right now because the payment might actually have passed successfully...
Nicolas Bouteille → created an issue.
Thanks I did not know about this module. I'll go check it out.
With such a module, I would agree Drupal Commerce does not need to make sure they handle errors better than now.
Maybe we could just make a recommendation on the Commerce's project page to invite developers starting to use Commerce to better accompany the customers in case an error occurs instead of a blank page.
However, I still believe that an error happening after the customer has entered his/her payment details and is waiting for payment confirmation is a very very very critical moment that deserves a specific reassuring message. So I will probably still handle this specific use case with special care as far as I'm concerned.
Nicolas Bouteille → created an issue.
Here is how I've been able to solve the problems I just told you about:
The code below is added to StripeReview->buildPaneForm() after line 200 after we check wether the PaymentIntent already exists or not. Indeed, I believe we should update setup_future_usage every time we come back to StripeReview even if PaymentIntent already exists, because the client may have gone back and switched from using an existing payment method to using a new payment method and vice versa.
if ($this->order->get('payment_method')->isEmpty()) { // new payment method
// Set the setup_future_usage parameter for the Stripe Payment Element.
if ($stripe_plugin instanceof StripePaymentElementInterface) {
$intent = PaymentIntent::update($intent->id, [
'setup_future_usage' => $stripe_plugin->getPaymentMethodUsage(),
]);
}
}
else {
// For existing payment methods, we don't set setup_future_usage at all (NULL) so we don't create multiple mandates
if ($intent->setup_future_usage === "off_session") { // if the client previously chose to add new payment method
// it is not possible to update PaymentIntent that already has setup_future_usage set to off_session back to NULL
// only solution that seems possible is to cancel current PaymentIntent and create a new one without setup_future_usage
$intent->cancel();
$payment_process_pane = $this->checkoutFlow->getPane('payment_process');
assert($payment_process_pane instanceof CheckoutPaneInterface);
$intent_attributes = [
'capture_method' => $payment_process_pane->getConfiguration()['capture'] ? 'automatic' : 'manual',
];
$intent = $stripe_plugin->createPaymentIntent($this->order, $intent_attributes);
}
}
Obviously, the code before line 200 that sets setup_future_usage for PaymentElement only when the PaymentIntent did not exist yet, should be deleted...
What do you guys think?
Nicolas Bouteille → created an issue.
Nicolas Bouteille → created an issue.
Just wanted to share that with PaymentElement, the checkout flow is different and we don't need this patch. PaymentProcess->createPayment is not responsible for confirming the PaymentIntent anymore. The PaymentIntent confirmation is handled in JS in commerce_stripe.payment_element.js and PaymentProcess->createPayment is now only called after the payment confirmation attempt has been made, and is only responsible for creating the Payment entity on Drupal's end. Now when 3DS authentication fails, the promise resolves with an error. The error message is displayed to the user telling them that the authentication failed and they need to choose a new payment method a try again. The payment method is no longer deleted so this patch is not necessary with PaymentElement.
Rarer scenario: the client chooses to use a new payment method (which creates the PaymentIntent with setup_future_usage to off_session) but changes his mind, goes back, chooses an already existing payment method. Now in StripeReview we need to update the PaymentIntent and set setup_future_usage to null again.
Because of the code above, a new specific case also needs to be dealt with: after selecting an existing payment method, the client changes his mind, comes back and finally chooses to use a new payment method (that will be added with Payment Element form at review step).
As of now, setup_future_usage is only set in StripeReview when the PaymentIntent does not exist yet.
The PaymentIntent is created the first time we arrive at the Review step.
Because the client selected an already existing payment method at the Payment Information pane, with the code above, we did not set setup_future_usage (null) so that no new mandate would be created.
Now he's gone back and selected "use a new payment method".
So when StripeReview->buildPaneForm() is called for the second time, the PaymentIntent already exist so we never reach the code that is supposed to set setup_future_usage.
Hi there,
I'd like to add some info to the mix that needs to be taken into account about setup_future_usage:
📌
Do not set setup_future_usage off_session again for already stored payment methods to avoid multiple mandates creation
Active
Nicolas Bouteille → created an issue.
Hi,
I got some update:
On my previous comments, I mentioned that the person from Stripe told me I did not need to do anything client-side, that I could confirm payments server-side only... and require client-side JS action of the customer only if 3DS is required. This is because they thought I wanted to charge the person in off-session mode. But we also need to deal with the client reusing its payment method in on-session mode.
My readings made me believe that stripe.confirmPayment() could be used to confirm any kind of payment client-side, thus removing the need to implement every individual case with stripe.confirmCardPayment for credit cards, stripe.confirmPayPalPayment for Paypal, stripe.confirmSepaDebitPayment for SEPA Debit, etc.
My tests confirm this.
I've just been able to finalize / confirm a payment with a previously stored payment method of type Paypal by simply replacing in commerce_stripe.payment_element.js
stripe.confirmCardPayment(
settings.clientSecret,
{
payment_method: settings.paymentMethod
}
).then(function (result) {
with
stripe.confirmPayment({
clientSecret: settings.clientSecret,
confirmParams: {
return_url: settings.returnUrl,
// payment_method: settings.paymentMethod // not even needed since it's already been added to the PaymentIntent
}
}).then(function (result) {
using the same piece of code, I was able to confirm a payment with a stored credit card as well.
So I can confirm that stripe.confirmPayment() seems indeed generic and allows to confirm all types of payment methods with no need to confirm the payment server-side.
Only difference is, like when using PaymentElement form when adding new payment methods, the Promise never resolves and Stripe redirects us to the return_url. NB: this can be overridden with the 'redirect' parameter when possible if necessary...
So we have to adapt StripePaymentElement->onReturn to not try to save a payment method twice.
BTW as you may have guessed, I have been able to update StripePaymentElement->onReturn so that it can save payment methods other than credit cards. This does not require too much effort. I will probably create another issue to share my code when I'm done.
Because this is issue is too long, I'll just keep it a support request and we could create a cleaner, shorter, separate feature request...