I've pushed a commit to fix an inconsistency in the config schema. A few more commits to follow to address a couple other issues.
@newaytech,
Thanks for providing this.
Some observations:
1. The StripePaymentElement line numbers indicated in the stack trace don't seem to line up with commerce_stripe 2.0.1. Can you confirm you are using 2.0.1 with no patches/modifications?
2. The order workflow shouldn't be relevant here. The checkout flow could be if you have a custom checkout flow.
3. While the stack trace is here in full, there is usually one more line that reports the error in the error log. It appears the the "Call to a member function getState() on null" is occurring when isVisible() is called on a particular checkout pane. Identifying that pane, and the line this occurs on would be helpful in determining this issue. This is possibly an issue with that particular checkout pane.
@newaytech,
Thanks for the report.
Receiving the webhook before the return is a valid and expected scenario.
The most important scenario, is if the customer completes the payment, the payment is processed on stripe, but the onReturn does NOT occur at all. e.g. If the customer loses their internet connection before onReturn is triggered from their browser.
The payment completed on the stripe side, and the webhook is there to complete the placement of the order, even if onReturn does not. Otherwise, we would have a stripe payment, but the order is not marked as placed/paid.
We have tested this extensively and built the module to handle race conditions between onReturn and onNotify(webhook). No matter which occurs first, both should work, and duplicate payment entities should not be created.
Sounds like you have encountered a scenario we are not accounting for, or something new causing an issue.
Can you provide more details on the "Call to a member function getState() on null" error you encountered?
There should hopefully be a full stack trace or at least a more detailed error message in the log messages so we can look into this.
(Queueing Webhook events is primarily helpful when your site is under load and can't process all the messages in real time. e.g. a Black FridayGiving Tuesday type of event, or a ticket drop for a large concert. It also provides other benefits, but queuing should solely be relied on for handling the race condition, as a slow internet connection could still cause onReturn to occur after onNotify.)
MR created that implements two field formatters for adjustments.
One is separator based. The other displays as a table.
Configuration options include specifying the adjustment type(s) to include, whether to display the label(s) of the adjustments, and currency options.
We can consider the collectAdjustments as a followup enhancement.
Hi all,
Appreciate the feedback, but this issue was fixed 7 years ago.
If there is a regression or new problem, a new issue should be created, and optionally reference this one, if needed.
Based on the comments and patches, I suspect everyone encountering this issue now is using Drupal Core 10.4.6 or 11.1.6?
There was an issue in those releases that caused a problem with aggregate fields. (See: 🐛 Decimal separator and decimals settings ignored when aggregating decimal fields Needs work )
This issue was reverted in the following releases, 10.4.7 and 11.1.7. Upgrading to those version will likely resolve this regression.
If this doesn't resolve it, please open a new issue referencing this one.
This is now fixed on 2.x and back ported to 1.x.
While we're here, we should rename the getStore() method. In commerce, that generally returns a (commerce) StoreInterface. Going to make this getTempStore() to differentiate that.
Hmm...if we are going to make this method public, we should add it to the interface.
We are working to land this ASAP.
Appreciate the testing and feedback!
PayPal and Link are NOT supported at this time, as we have not landed Payment Method Types for them. (This needs to land with PaymentElement in order for Express Checkout to be able to support it.)
(If you are able to enable either of these, then that is something that needs to be addressed in this MR.)
After we land the Payment Method Types, we can then enable them in Express Checkout. (PayPal has a proposed MR that is under consideration. Link needs additional work, as it it not only needs a PaymentMethodType, but also some js/UX changes for support. Specifically, Link works with a very simple PaymentMethodType for new payments, but does not work for saved payment methods.)
@jonathanshaw in makes sense, if we are supporting the next step not being "complete", that we defer the checkout completion event and the place status transition to the implementer's implementation.
We do need this to be consistent, though, between the webhook and the checkout flow. It is necessary to invoke the checkout completion event, as that typically is what sends the order email. If the customer completed payment, but was not able to return to the site, or made it through the onReturn, but lost the network connection before getting to the complete page, we need to fire those events so that they can still get the order email, among other things that occur from the place transition and/or checkout completion event.
Hopefully, this handles the most common cases, and, as you inferred, the placeOrder method can be overridden, if needed, for custom implementations. (Though I'm now considering we should rename that method, given that it isn't actually placing the order now if the workflow has steps between "payment" and "complete".)
To be clear, we should NOT have created the 2.0.x branch initially.
We should have just created the 2.x branch. Unfortunately, it isn't possible to delete branches once they have been created.
2.x is the default branch, and all new issues should target this branch.
There is no way to "force" 2.x on the project page. Once we tag a 2.1.0 release, though, that should resolve that issue.
(And for completeness, we might have needed to create a 2.0.x branch in the future, in a specific scenario...but don't expect that to be the norm.)
Hi all!
Thanks for the reports, and sorry you encountered this issue.
The js file has been further modernized and refined, removing jQuery related references.
@willardb, you were actually closer in your initial assessment. :)
We should actually never submit the form. Stripe will redirect the browser when the intent is confirmed successfully. (The redirect url actually is a different destination than what would happen with a submit. It is actually important to NOT submit the form.)
The submit event listeners should only:
1. disable the submit button(to prevent multiple submits),
2. invoke the Stripe confirm method
3. if an error occurs, display the error, and re-enable the submit button
The two methods are now nearly identical, other than their initial argument. (We might refactor this later into one method.)
Looks like we've got some javascript functional tests failing. This are not related to this issue.
I'll be getting this merged, and the tests fixed.
Stripe Payment Element defaults to "auto" support for languages/locales based on the browser's indicated locale.
See list here: https://docs.stripe.com/js/appendix/supported_locales
See also here: https://support.stripe.com/questions/payment-element-faq
Are you visiting the site using a browser configured with one of the supported locales/languages? If so, this would seem to be a Stripe issue in their browser detection.
If you are visiting the site with the browser configured to one language, but the site configured to another, this is a different scenario.
If you are doing testing, then you would likely just need to update your browser settings to properly test this.
If you are specifically wanting a site that handles different configurable languages, regardless of the browser language configuration,
this is a more complex scenario, and not certain we want to take over Stripe's auto-detection to support this. (If we did, we would need to, at a minimum: 1) ensure the locale names map 1:1 with Stripe's. More than likely, we would need a mapping Enum to handle this. 2) determine what to do for languages Stripe doesn't support, but Drupal does.)
Another simplistic, brute force approach, would be for commerce_combine_carts to clear any messages after it has invoked the cart unifier in its hook_user_login implementation. (I say the hook, rather than in the cart unifier, because theoretically, the cart unifier could be used in a non-user_login context.)
The downside to this for the OOTB commerce (core) message, is that it is a generic "Success" message, so, again, not an easy way to target just the add to cart message in commerce.
It would be possible, though, for commerce_combined_carts to invoke the clear() method on the CartConfirmationManager in this module, to clear the messages.
(Note: you could also do this in your custom code as a current solution.)
@anybody,
Appreciate your feedback on this!
You did conveniently skip over the fact that this issue exists with commerce_combined_carts and commerce (core), without this module involved. :)
(Though that message is the basic Drupal message.)
Adding a context doesn't seem to be a trivial matter. Most likely, this would involve:
1. In commerce, adding a "context" to the cart Manager, when invoking addOrderItem.
2. In commerce, adding this passed context to the CartEntityAddEvent event.
3. in commerce_combine_carts, passing this context when invoking the cartUnifier. (e.g. from hook_user_login)
4. In commerce_combine_carts, in the cartUnifier, passing the context when invoking addOrderItem.
5. In commerce, checking this context, if it is provided in the CartEntityAddEvent event, and suppressing the message if it is present.
6. In commerce_add_to_cart_confirmation, checking this context, if it is provided in the CartEntityAddEvent event, and suppressing the message if it is present.
That would be a bit of effort, and this module is at the tail end of it.
Hi, thanks for the report, but this seems to be erroneous report in whatever tool you are using to analyze the code.
This class is an enum class, which was not introduced until php8.1, and is using appropriate syntax for a self-describing label.
@grevil, not sure what you tested, but I just did a clean install of commerce + commerce_combine_carts, and the default "[product] added to your cart" message is displayed.
This does NOT appear to be specific to this module, but a result of this (or commerce core) event subscribers adding a message.
You mention "commerce_add_to_cart_confirmation_page_attachments"...this isn't really relevant. The critical part is this module (as does commerce core) subscribes to the CartEvents::CART_ENTITY_ADD event, and reacts when it is fired to display the message.
There is no context of an item being added via a 3rd party module, e.g. commerce_combined_cart, to indicate that the message display should suppressed.
Again, this is true for both this module and commerce core.
In both cases, both module's events are doing as intended...displaying an add to cart message, when something is added to the cart.
Now, the question arises as to whether this really should be suppressed when the user signs in. If:
1. the user had a previous cart with item A in it.
2. They return to the site anonymously, (possibly days later), and add item B to the cart. (And get an add to cart message.)
3. They sign in to the site. (Thinking they have a single item in their cart.)
4. Combine carts then does what it does, and combines the two carts.
What should happen? With this module (or with the default commerce core cart module), they will get a message about an item added to their cart.
While the message might not be perfect, it seems reasonable that the customer receive a message about another item in their cart. Otherwise, they might actually be confused as to where this mystery item came from.
An ideal message would probably be something like "We've added your new item(s) into a previous cart you had while browsing the site!" or something to that effect.
But, that really would be something more appropriate for commerce_combined_carts to do, rather than this or the core functionality.
Silencing the message, though, doesn't seem like the best UX, though, tbh.
Are you using commerce_combine_carts by chance?
If so, it does "add" items from one cart to another, which would trigger this.
I presume this would also occur with the "standard" add to cart message in commerce core, if that is not disabled.
(If not, is this consistently reproducible? Or possibly a transient anomaly immediately after the upgrade?)
@gillmead, Thanks for the report!
In hindsight, maybe we shouldn't be putting the DOB in the label, even if it is the only field returned with the Klarna payment method.
I've modified the display label to return "Klarna ([createTime])", with the creation time of the Klarna payment.
I've merged a small commit that ensures the transition is allowed before invoking it.
While certainly not complete, see [#3515814--16059987] for some of the details behind handling the Stripe Payment Element in this fashion.
tomtech → changed the visibility of the branch 3513042-onreturn-method-of to hidden.
@jonathanshaw, thanks again for the report and MR.
The MR has been merged.
A few things to note:
1. It is necessary for the payment step to be bypassed.
The nature of the PaymentProcess Pane is to be "visible" and create a payment. When that occurs, it will not succeed, an exception would be generated, and the customer redirected back to a previous step...leaving the checkout flow and order in an incongruent state, given the payment was actually successful.
While it might seem that the PaymentProcess would be "invisible" if the order is paid, this only occurs if the payment is "complete". If the payment was only an authorization, (what Stripe calls "manual") the order would still have a balance, until the authorized payment is "captured".
Additionally, if the payment was less than the total, it would also cause the PaymentProcess pane to be "visible". This can happen if the review page with the Stripe payment component was reached with a discount applied, and/or a specific tax rate in effect...but the payment is not completed until later. e.g. Payment started at 11:55 pm December 31st, with a 10% discount applied, and a 7% tax rate, but the customer completes the order at 12:05am an January 1st, when the discount has expired, and the tax rate has changed to 8%.
2. It is possible that the customer completed the payment with Stripe, but the onReturn is NOT invoked. (This can happen when the customer's network connection drops, battery dies, etc...) In this case, the webhook necessarily handles creating the payment and placing the order. In the OOTB handling, the order will move the checkout to the complete step. If you are adding additional steps between payment and complete, be sure to consider this possibility.
Thanks for the report and. the MR, @jonathanshaw!
When the payment is saved, the order is queued for update and this occurs on request destruct, so the total paid and balance do get updated on the same request.
But, as you've observed, they are not yet available when the place transition is invoked after the payment is saved.
While this
Invoking the updateOrder() directly between the payment save and the updateOrder() is appropriate.
@anybody,
This is already being used on production sites.
We are in final testing before committing. (We generally try to test in different use cases/scenarios before committing, especially a larger feature add like this.)
Found some minor issues during recent testing that @vmarchuk has since addressed.
If you have any issues/feedback, please provide! (Note that, depending on the issue, we will likely still commit and tag a release, then follow up in a future commit/release.)
@jonathanshaw,
While ApplePay and GooglePay appear to be distinct payment method types in Stripe, this is only true to some degree. They actually return as the card payment method type, with a wallet.type of apple_pay or google_pay. (I know...confusing, isn't it?)
The initial reason for the implementation of the PaymentElement was to add support for ApplePay and GooglePay. (CardElement does not support them.) We've been adding additional payment method types over time since then.
@aaronbauman and @jonathanshaw,
Thanks for the report and comments on this. (And the other 2 items mentioned.)
Just got back from DrupalCon Atlanta, but will look to get some of this resolved soon.
Note, though, that there are reasons for the implementation, and I'll articulate them for your understanding and consideration. (Makes sense to get it into the documentation and/or a change record.)
Would also appreciate understanding your scenarios, as well, so that the best end result can be achieved.
@aaronbauman, you mention that the order is already marked complete at this point in your site. Given that this is done immediately after the payment is created, would it be reasonable to presume you are doing so when the payment is created? Can you share thought on this?
zaporylie → credited tomtech → .
The reverted database update selects all order ids, then uses it in the delete statement.
That would work with a nominal number of orders, but on sites with hundreds of thousands, or millions of orders, this would not be performant, presuming it completed at all.
The proposed singular not exists delete statement in #3363849-13: commerce_log entries are not removed on source entity delete → is much more performant, and could handle millions of rows.
For some sites that do have a larger number of orders, they might prefer to handle this manually in a separate task than during database update execution. This is where the flag that skips the execution is useful.
The preferred approach would have the not exists statement wrapped in the execution flag.
Hi and thanks for the report!
A few things to note here:
1. This has been the default behavior for ~12+ years, going back to D7. It is really a sample, though. You aren't locked into the view or the template.
2. This can get really complicated to try and handle OOTB, depending on the merchant requirements. Some examples:
If there is a max quantity of 10 for an item, there are currently 8 items in the cart, and the customer attempts to add 4 more, your site might only allow an increase of 2, not the 4 that was attempted.
If you are displaying price, and the price changes when quantity is increased, which price should be displayed? The base price from the product? Or the calculated price on the order item?
A simple way to avoid many of these issues, is to simply display the product that was added, and not display the price or quantity in the message. (Allow the cart to handle displaying the complexities of price/quantity/extended price/adjustments, etc...)
3. Fortunately, the dialog renders a view in a template. Customizations can be achieved by modifying the view. Advanced changes can be achieved by overriding the template in your theme.
Hi!
Thanks for the report!
Both width and maxWidth were set in the js when the dialog was declared. This provided a reasonable desktop width, along with the max width for mobile.
When the jQuery dialog was replaced by the Drupal core dialog, it appears support for the maxWidth property was not carried over.
Setting the width to 90% might work on mobile, but is way too wide for desktop, especially a wide one.
With the changes in theming in a recent MR, #3511910: Cleanup Theming → , the width is still set to 745 for desktop/laptop. max-width is now set in the css to handle this. (You can, of course, style to your desire in your theme!)
Hi @megachriz!
Thanks for the issue and the MR!
Confirmed and merged!
Looks like the tests started failing on a prior commit.
That will need to get resolved before any more merges can occur.
API version has been reworked. There is now a getter (getApiVersion()) for it, that is used for both the SDK and the js initialization.
api_version has been added to the configuration schema, and can be set.
NOTE: While the api_version is now part of the config schema and can be set, it has NOT been added to the gateway UX at this time. Changing API version is an advanced change. Providing an invalid API version will cause exceptions. Even when a valid API version is provided, payloads returned may be different which may cause unexpected behavior, or exceptions. (This MR addresses one such case.)
tomtech → changed the visibility of the branch 3489133-custom-display-label-translatable to hidden.
Hi sorabh.v6!
Thanks for the report!
This is not the appropriate fix, however.
The label in question is not currently showing in the translate tab, as the schema does not currently indicate it is a translatable property.
We need to update the schema to address this.
@grimreaper,
Thanks for the report!
Took a minute to setup to circle on this one, as it has been a while since I've used a prefix. :)
Direct DB queries do support prefix substitution.
It wasn't possible to write this query using the abstraction layer, because of the subqueries.
As observed in your MR, the table names were properly wrapped with curly braces to handle prefix substitution, but for the subquery joins, the table.column needed to also be wrapped as {table}.column. (When using an alias, this isn't necessary, but for some SQL statements, aliases can't be used. For those, the full table name must be used.)
I've reproduced the issue, and the proposed MR resolves it.
Merging.
Thanks!
Hi @mhawwari,
The PaymentElement actually supports using multiple different Payment Methods.
There are currently 8-10 implemented.
Currently SEPA is not, but only requires implementing a PaymentMethodType to be supported.
Hi @brenm!
It sounds like either:
1. The Strip Review Pane is not present on the Review Checkout Step
2. If it is present, then best place to investigate is the browser console log. It is possible that some other javascript on the site is conflicting/breaking with the stripe js.
Stripe Card Element, as its name implies, only supports cards. (Additionally, it does not support ApplePay or GooglePay.)
As of this writing, this module's Stripe Payment Element currently supports the following Payment Methods:
Card (including ApplePay and GooglePay)
ACH (US Bank Accounts)
Affirm
Alipay
AmazonPay
CashApp
Klarna
WeChatPay
This list is growing!
The initial implementation only supported Card.
The others were added the past year.
You might find MRs/patches for some in the issue queue.
This was originally a D7 issue, which is no longer supported.
ApplePay is already supported in the current version using the Stripe Payment Element. (Stripe does NOT support ApplePay with Stripe Card Element.)
ApplePay will also be supported with the upcoming Express Checkout implementation. ✨ Stripe Express Checkout Element Integration Active