🇮🇱Israel @jsacksick

The first comment mentions SEO which is not really a concern for checkout pages which are typically not indexed and shouldn't be.

Discussed this with Ryan on Slack, perhaps we should mimic what we were doing in Commerce 1.x which is:
Checkout: [step label]. (We probably should wrap that in a t().

$this->t('Checkout: @step_label'); Thoughts?

jsacksick → created an issue.

Actually, both are incorrect, the checkout flow plugin could be different and the identifier could be different.
This selector "#commerce-checkout-flow-multistep-default" assumes the "Multistep default" plugin is used, so we need to fix this.

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

Added D11 support & committed. Thanks everyone!

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

Pushed an update. The payment type is now mandatory and we run through the parent create method for all the set up.

Where did you see that the payment type was now mandatory? It is not, plenty of modules aren't specifying it?

I just tested this. the "Not in" filter requires a state to be chosen? Whenever I submitted the filter without choosing a state I got an error, but this would essentially be a useless filter, so choosing a state is required in order for this to work...

See the screenshot:

jsacksick → created an issue.

I actually decided to rewrite returnPage() as well for consistency.
Plugins using the route match rather than the passed order should be rewritten. But I don't believe this is the case, I think the OrderVersionMismatchException was thrown due to the checkout step redirection login from the checkout flow plugin which was always using the order from the route match. Using the setter, the order is set afresh after loading it via loadForUpdate().

The change from the commit linked in #8 introduced PHPSTAN issues. Slightly adapted the change, See: https://git.drupalcode.org/project/commerce/-/merge_requests/485/diffs?c...

I did implement the change proposed, See https://git.drupalcode.org/project/commerce/-/merge_requests/485/diffs?c....

Since we're making this change in cancelPage(), I wonder if we should do the same from returnPage() (i.e. set the order on the checkout flow plugin rather than messing with the route match parameters but a bit more hesitant to change this).

Ok so 8.x-2.x is no longer maintained, we just need an MR for 3.x.
Additionally, the checkout flow plugin now has a setOrder() method that we could use here without the need to interfere with the route match I believe, unless some other code is relying on it?

So we could update the code to look like the following:

$checkout_flow_plugin = $checkout_flow->getPlugin();
$checkout_flow_plugin->setOrder($order);

Technically the payment plugins shouldn't rely on the route match parameters from the onCancel() method as the order is passed there?

Attaching a patch for use with composer.

Merged, thanks!

@majmunbog: So did we just use the same color than core?

Added a CurrentCurrency condition plugin.
Not sure if we should add a "negate" checkbox. We skipped it for UX reasons in the past, which is why I skipped it here. Was wondering about copying the "Matching strategy" from the CurrentUserRole but it doesn't make sense here as the current currency cannot match all the currencies selected...

So it's either "any" of the currency selected, or none of the selectec currencies.

See the screenshot below:

Assigning to Ryan for review :).

jsacksick → created an issue.

jsacksick → created an issue. See original summary → .

One thing that we're missing that could be eventually addressed as followup is a CurrentCurrency condition plugin. This would be super helpful with promotions & shipping for example.

I'm not particularly in favor of making the flag permanent, as you'd never be able to send the email from the UI. Plus you might need to use the order receipt resend form to send the order to another email (once that feature lands, See ✨ Allow specifying an email to send the order receipt to Active ).

If you're explicitly opening the form to resend a receipt, we shouldn't prevent the user from doing that. By MailerHelper are you referring to the OrderReceiptMail service?

Also if we make the flag a non boolean and it can support more than 2 states, we should probably make it an enum.

But maybe we'd need a more global flag that'd be respected by commerce_shipping too for the shipment confirmation (instead of having a separate flag for this).

So in this case, we'd be more looking at a flag that can be set for programmatically placed orders I suppose, rather than what we're trying to solve here (i.e. the place transition applied from the UI);

jsacksick → created an issue.

Reviving this, let's create an MR, we also need tests.

Let's unset the data flag to make it more like a per request flag instead of a global flag that wouldn't allow resending the order receipt manually.
We can unset the flag from the order receipt subscriber.
I'm also wondering if we should update the order type method to check for the order flag as well, but the method doesn't accept an order, so that would be a breaking change.

We have a built-in method for that. See Order::getCalculationDate().

Hi, you should be able to manage your payment methods from your Dashboard from now on, there was a seting that had to be turned on to allow that.
Please let me know if the issue is fixed.

Weird, this only happens with D11, the attached patch should do the trick.
Also, perhaps we should address ✨ Add a setting to suppress the credentials message after Stripe Connect Active from this MR too... Not sure anyone would ever set the settings flag though... So maybe we should not output the credentials at all? But I'm thinking this is necessary for people used to overridding credentials from settings.php (or via variables overrides from the different hosting providers).

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

Change looks legit, committed to 2.x, thanks!

I'm fine committing this as is, but giving @zaporylie an opportunity to review this since he worked on this (even though that was years ago).

jsacksick → created an issue.

@valic: Just wondering about keeping / reintroducing getResolvers() to expose the resolvers to the outside world.
If this is later needed, that means introducing a breaking change but at the same time, it may not be needed :p.

@vmarchuk: Reviewing the code once again, and regarding your feedback:

@jsacksick
When an anonymous user is on the cart page (not going deep into the checkout process), we don't have a "checkout_flow" value, so we can't check if we can use the "guest_new_account"/"guest_order_assign" settings. I don't want to duplicate the logic, but otherwise, we need to "force" site owners to use "guest_new_account"/"guest_order_assign" settings. Maybe we should add an "Allow anonymous checkout" setting to the payment gateway that will only be available when "guest_new_account"/"guest_order_assign" is checked (we can specify this in the field description). But this will affect the entire checkout process.
Another option is to leave everything as is and just document it (as you said).

The duplicated code runs on CheckoutEvents::COMPLETION, is Stripe firing that event manually? Maybe from StripePaymentElement::placeOrder()?
Then maybe we should resolve a checkout flow? Though that makes it hard for merchants to figure out the right checkout flow to configure for the guest account behavior...

@tomtech: thoughts?

Also pasting here the feedbacks from Tom regarding the JS file:

We got rid of jQuery in the paymentintent js file. We are regressing by re-introducing it in this new js file.
closest() is a native call since ~2015.
Same for forEach() (as a replacement for jQuery each())
$.ajax is replaceable by either Drupal.ajax or just js native fetch().

I decided to skip defining a service collector but instead went with the AutowireIterator attribute.

The ChainCurrencyResolverInterface could almost be skipped... We no longer need an addResolver() and we don't really need th getResolvers() method.

We could keep the interface to be able to distinguish between the chain currency resolver and resolvers themselves, but not 100% sure we need to expose the resolvers to the outside world... Kept it in case there is a usecase for accessing the resolvers? But we aren't even calling the getResolvers() method ourselves...

Also implemented return types and defined a service alias for autowiring.

I'm generally not against the idea but it feels odd to me to define a new API Commerce itself isn't really leveraging? Reviewed the MR and left a few comments:

1. We define a new API, but besides defining a currency resolver, we aren't actually relying on the chain currency resolver? Maybe this is fine as this means no risk of introducing a performance regression I suppose.
2. I wonder if we should expand the Context object to be aware of the currency? Though it's always instantiated manually so perhaps not a good idea...
3. Perhaps we need a test price resolver that uses the new chain currency resolver as an example? Would have made sense to have it available from the Context I guess. At the same time... The order probably already has a currency when price resolvers are invoked for a given order item, so currency could be set using the order total currency code instead.
4. Perhaps we could extract the ServiceCollection from this MR: https://git.drupalcode.org/project/commerce/-/merge_requests/438/diffs#8... so we collect services the "new" way :).

Back to the last point, perhaps ServiceCollection should be renamed ServiceCollector and getCollection() to just getServices()? Or getCollectedServices()...

Authorize.net has been compatible with Commerce 3 for almost a year?

See https://git.drupalcode.org/project/commerce_authnet/-/blob/8.x-1.11/comp...

hm... Just realized a much simpler alternative was possible... By overridding the "_entity_create_access" route requirement as done for Commerce pricelist as done in 🐛 The add price action is missing on D11 Active .

But it turns out the Wishlist usecase is quite different as we're checking a custom permission (similar to product variations: "manage {$bundle_name} {$entity_type_id}").

jsacksick → created an issue.

Not sure what was the initial reasoning but I don't think we could support more than one purchasable entity type properly without passing the type from the url...

Also, since there is no wishlist item type setting for example at the wishlist type level, not sure how we could properly determine the type to use?

Or maybe we'd need a deriver defining local actions for each purchasable entity type defined, but that would result in a weird experience...

"Add product variation item"?

The wishlist code is used. This is the url for wishlists:

/user/{user}/wishlist/{code}

In theory a user can have multiple wishlists, so we can't use just "wishlist".

@vladimiraus: You're right, somehow missed the opened issue. However since this one went in, it probably makes more sense to mark the other one as outdated or duplicate... Since this one made its way into 2.18.

Sorry I missed this one, fixed this as part of 🐛 Fix phpcs & phpstan issues Active .

@larowlan: I didn't mean to be offensive. This probably came out wrong. I was just genuinely trying to understand what is it am I missing here.

@vladimiraus: Did you not read the comment #10?

How is this API not available for Drupal core 10.2 and earlier?

renderRoot() has been around for 10 years? What am I missing?

See https://git.drupalcode.org/project/drupal/-/blob/9.4.0/core/lib/Drupal/C...

It even existed in Drupal 8: See https://git.drupalcode.org/project/drupal/-/blob/8.0.x/core/lib/Drupal/C...

hm... Actually I managed to reproduce this with D11... The wishlist item add page returns a 404.

jsacksick → created an issue.

jsacksick → changed the visibility of the branch 8.x-2.x to hidden.

Created an issue to fix those (See 🐛 Fix phpcs & phpstan issues Active ).

hm... This introduced phpstan errors, would have been nice to fix them before merging the MR.

jsacksick → created an issue.

I don't see why a new branch is required... If all we do is calling renderRoot which kinda always existed (it was added 10 years ago)... So I think we can just merge this?

Just closed ✨ Allow the PaymentProcess pane to work even without PaymentInformation Active as a duplicate.

Several comments / thoughts:

1. Regarding the getErrorStepId() method, perhaps we could simply fallback to the previous step ID in case the payment information pane is missing?
2. Regarding isVisible(), well maybe we should not tie the visibility logic to the presence of the payment information pane? If the payment process pane is added to the checkout flow, then perhaps we can/should assume that a payment should be processed? If no payment should be processed, then I think the right thing to do would be to remove the pane from the checkout flow?
3. One option that wasn't considered either could be to make this configurable via a setting that would default to "payment_information"? Though this is likely not something that makes sense to be exposed in the UI.

Wasn't sure which one to keep, but since the other issue has more comments, let's keep the older one ✨ Allow the checkout flow to determine it's payment information pane Needs work .

Ok, went with the label for consistency. We might need a followup to call collectAdjustments() as it looks kinda odd to only display order level adjustments (which normally only contains shipping, shipping taxes and eventually shipping promotions). Other than that, all other adjustments are added to order items.
But I'm fine with doing this as a followup, since this is already a big improvement as we don't have formatters for anything atm :).

Thank you Tom for addressing this, retitling the issue.

Tested this manually, works great! Only one minor comment regarding the Adjustment types setting. Just wondering whether we should use the label simply? Or if we stick with the plural label, perhaps we should uppercase the first character using ucfirst?

I think everywhere else (e.g: when adding an adjustment from the order edit form) we simply use the label (which is singular).

Yes, you need to turn on the commerce_stripe_webhook_event module. And we support processing the jobs with advancedqueue as well if it's installed.

jsacksick → created an issue.

@grevil: Could you elaborate on what needs to be adjusted? Not too familiar with the other issue linked... Also the links from your comment #58 aren't working for me (I'm getting a "Couldn't fetch the linked file" error).

This is probably a Commerce Stripe issue and I don't think it is still relevant as the webhook processing is queued? Are you running the latest version?

Ok the change shouldn't be disruptive as I finally skipped return types, will go ahead and merge this.

Actually, more in favor of increasing the scope and doing this once for all... This change is long overdue and should have been part of 3.0.0

jsacksick → created an issue.

There’s no visibility of how many Drupal 9-10.3 installations still out there

There is: https://www.drupal.org/project/usage/drupal →

So I believe with my changes, the BuyXGetY auto add feature won't work when the price calculated formatter invokes the processsor, at the same time, I don't think it makes sense for the formatter to trigger the creation of a new order item that is then orphaned and saved into the DB permanently.

hm.. Maybe not the right fix...as we have $get_order_items[$order_item->id()] = $order_item; later on... Maybe the whole auto add logic should be bypassed in case an unsaved order is passed?

Any chance you could test the patch from the attached MR? This is a completely blind fix.

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

I tried merging from the issue to give credits to multiple people but that didn't work. Merged from the MR but that didn't edit the commit message... Anyway... Thanks for this!

Updated the target branch to be 2.x (and not 2.0.x).

Regarding the subscriber; if we have no other option fine. I'm just wondering if we should add a setting for this behavior? Was creating an account part of the requirements?

Also, regarding applyRate(), does it work without? I don't see where the rate is applied otherwise? All we do is setting a shipping method?
I also merged 2.x changes to fix the test failures.

The code was already not relying on JQuery, but the library stiill depends on it, I think we need to add a dependency on core/once too. Can you make those changes please?

Is is possible to open a merge request with the changes?

This was actually fixed today in this commit: https://git.drupalcode.org/project/commerce_paypal/-/commit/81394b3ced0e....

@alex.bukach: Can you explain what we're changing exactly? The validation is now skipped if the condition is unchecked... or? Not fully clear by just looking at the code change.