🇩🇪Germany @Grevil

Hey @tomtech, just wanted to ask how soon we can expect to get this reviewed / merged?

Also, I couldn't find an issue regarding the missing PayPal / Link PaymentMethodTypes, so I created one here: 🐛 Implement PayPal and Link "PaymentMethodType"s Active . Not sure if you guys plan to implement that yourself? Otherwise, we could also chime in to help implement the PayPal integration, as the "commerce_paypal" Express Checkout Buttons are currently no option for us (see 🐛 Anonymous user can't finish checkout using PayPal Express Checkout, when shipping rates exist Active ).

Thanks for the feedback! 🙂👍

grevil → created an issue.

Ok we could safely reproduce #10 now.

It definitely happens because of the missing shipping rate price in the total. Note, that this is NOT reproducible using sandbox mode.

Interestingly enough, the commerce stripe express checkout handles this correctly (see ✨ Stripe Express Checkout Element Integration Active ). But this might be a stripe thing. Stripe (through AmazonPay for example) communicates back and forth with the Server, so once AmazonPay has the address, they pass the address back to the server and the server can calculate the shipping rates based off the address given and send the available rates to stripe / amazon pay.

Please finish this @LRWebks

grevil → created an issue.

LGTM! Great work! 🎉🌈

grevil → created an issue.

Ok, back to needs work. Newest developments in 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active , show, that we need to properly handle the details and especially the links in a loop instead of resetting to the first issue / link available.

Ok, I gathered a few more information on this problem and I think I found the problem.

The problem is, that our shop overcharges the user above the amount he agreed to pay for, which is not allowed and against the "Overcapture requirements (PSD2)", on which PayPal implemented a way to reconfirm the new updated price through the "payer-action" link returned from the "PAYER_ACTION_REQUIRED" error response.

For more information, please take a look at:
https://www.paypal.com/ae/cshelp/article/overcapture-requirements-psd2-h...
https://developer.paypal.com/docs/multiparty/checkout/standard/customize...

This is easily reproducible using the express checkout buttons:
- An anonymous user buys a product through the express checkout cart button (we have no information on their address yet)
- He approves to pay the product costs via PayPal
- The user gets redirected to the shop. The shop now knows the address of the user (through by PayPal)
- The shop can now calculate the shipping rate based on the address given and patch the PayPal auction.
- When trying to finalize the purchase (capture the payment), PayPal returns a "PAYER_ACTION_REQUIRED" response containing a "payer-action" link, because the user approved the costs of the product only, not the costs of the product + shipping cost.
- The "payer-action" link redirects the user to PayPal, where he needs to reconfirm the new updated amount.

Unfortunately, there seems to be no error in sandbox mode for whatever reason. It seems like overcharging in sandbox mode is just fine, maybe there is a setting to tweak this behavior.

The remaining question is, whether this is something, that should be handled by the service provided through ✨ Print details from error messages returned by paypal Active . Or something else entirely. @tomtech @jsacksick what do you think about this?

Now, that we enabled the express checkout buttons, there are a LOT of messages like these in our shop. We temporarily disabled the express buttons, until this is properly handled.

When I apply the patch, update the db and add a new link, I get the following error:

TypeError: _menu_link_attributes_add_attributes(): Argument #2 ($options) must be of type array, null given, called in /var/www/html/web/modules/custom/menu_link_attributes/menu_link_attributes.module on line 36 in _menu_link_attributes_add_attributes() (line 47 of modules/custom/menu_link_attributes/menu_link_attributes.module).

@grevil looks like this needs an update hook to set this TRUE by default

Nope, it is good to go! "ViewsExposedFilterBlocksBlock" is a plugin, so the "defaultConfiguration()" method will do the job for us! LGTM!

Drupal 11 is stable for a while now, there won't be any more new patches from the bot! Setting this "Fixed".

clear() is public now! This is ready for review!

Great stuff! Thank you, @tomtech! 🎉

grevil → created an issue.

Something along these lines should suffice. I am unsure, whether "commerce_order.order.paid" is the correct event to use. But I couldn't find a better.

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

grevil → created an issue.

Magically fixed! 🌈

Sorry, we thought it might relate to 🐛 Orders are not completed Active .

@jsacksick, what do you think about the fix? It's pretty unclear, to us, how this fixes the issue.

Sadly, @alex.bukach did not reply.

We should indeed try to finish this (cleanly) as this could impact conversions and shop owners might not notice this issue at all.

Hmmm... Actually, I think we did it this way so that the alteration is done runtime rather than affecting the adjustment that is stored in DB. This way the adjustment label would always be on the current site language...

Can't remember exactly anymore. But we could still add a new setting (off by default) which prevents the subscriber to do anything, until activated!

EDIT: Or maybe there is something like an conditional subscriber in drupal / commerce? Unsure about that.

Alright! Seems to work as expected! We just had 2 test purchases inside a live environment using amazonPay and googlePay. Both worked as expected (Klarna was tested before inside a sandbox environment, which also worked great).

Setting this to "Needs review" for the Centarro people to take a final look at the changes!

New patch. Old patch had a bug I introduced, sorry for that.

@tomtech thanks for the feedback! And great to hear, that you guys are on it! Really appreciate the work of your team (and especially by @vmarchuk for working on this issue!).

I disabled support for link and PayPal. We will test it live now and come back to you guys again!

Static patch of the current MR attached.

Sorry, back to "Needs work" regarding PayPal.

Alright, this is pretty far now.

Regarding the payment methods:

Klarna
Works great and as expected.

AmazonPay
Works great and as expected.

GooglePay
Works great and as expected.

ApplePay
Not tested. I do not have Safari.

Link
Could not test link, since I couldn't log into my account for some reason

PayPal
After confirming the payment I get an error and get sent to the "checkout/{order_id}/order_information" page with the error:

Payment failed at the payment server. Please review your information and try again.

There are still a couple of console errors and I feel like a few parts of code should use existing commerce services instead of "reinventing the wheel" (e.g. the shipping address validation). But generally speaking, this is fairly close!
We will test it in a live environment tommorow and give further feedback.

Ignore #40 ✨ Stripe Express Checkout Element Integration Active we now skip the adjustment if it is of type "tax" and already included in the line item price. Meaning, the original implementation of "commerce_stripe_get_express_checkout_order_line_items" by @vmarchuk is restored again (with the mentioned check included).

But I am still unsure if is the correct approach, to pass the tax as a line item. But according to the Stripe AI assistan it is:

Certainly! Here's how you can add tax to a line item using Stripe.js without automatic tax calculation:

example.js
const stripe = Stripe('your_publishable_key');

const lineItems = [
  {
    price_data: {
      currency: 'usd',
      unit_amount: 2000,
      product_data: {
        name: 'T-shirt',
      },
    },
    quantity: 1,
  },
  {
    price_data: {
      currency: 'usd',
      unit_amount: 200,
      product_data: {
        name: 'Sales Tax',
      },
    },
    quantity: 1,
  }
];

stripe.checkout.sessions.create({
  mode: 'payment',
  line_items: lineItems,
  success_url: 'https://example.com/success',
  cancel_url: 'https://example.com/cancel',
}).then(function(session) {
  // Handle the session
});
In this example:

We define two line items in the lineItems array:

The T-shirt priced at $20.00 (2000 cents)
The sales tax as a separate line item, priced at $2.00 (200 cents)
We don't use the automatic_tax parameter, as we're manually adding the tax as a separate line item.

But on docs provided as resources by the AI I couldn't find this approach. But yea seems good enough.

as mentioned, waiting for other community members to chime in here, especially the ones that contributed patches previously. But from a pure code point of view, this looks good now

I don't think anyone will chime in here anymore. Can we get this fixed?

Ok, I found the issue. Adjustments were added as "extra_line_items", leading to this error.

Although this code seems intentional, I could not yet find the reason it was coded this way. @vmarchuk could you give feedback, on why the adjustments were added as line items? It might be needed for "onShippingRateChange()" and "onShippingAddressChange()" but not passed to the js eventually? Or we need to revert https://git.drupalcode.org/project/commerce_stripe/-/merge_requests/129/... again and just add a check?

Awesome! Thanks, @dydave and @ressa for your quick back 2 back finalization of this! 😍

Ok, the problem is the VAT that is part of the line items, even if it should be already included in the price of the products:

I'll have a closer look at this tommorow.

Added a few comments on the MR. Furthermore, when pressing e.g. the amazonPay checkout button, I get the following error in console:

Uncaught (in promise) IntegrationError: The amount 1986 is less than the total amount of the line items provided.

And the Amazon popup just loads infinitely.

I do not get this error when e.g. using the express checkout buttons from commerce_paypal.

Note, that !MR 129 is against 2.0.x, seems like that is a mirror of 2.x?

@vmarchuk the testing environment is currently broken, could you revive it?

Already told Jonathan about that via Slack, he is already at it / forwarded it.

I added a general hint in 🐛 Documentation improvements Active , that based on that setting, some active payment methods will be unavailable. Would that suffice already? If so, this issue can be closed as duplicate.

Alright, I ended up reworking the original MR. This should be it now! Please review!

Note, that !MR 175 targets 2.x and MR !122 8.x-1.x.

Adjusted accordingly. Please review now.

I'd prefer "PaymentExceptionHandler". None of the other classes have a "PayPal" prefix.

Also, with what we're doing, wouldn't that mean that we get boh the generic Drupal Commerce error + this one?

Yea, we would get the error + a message with the same error. I agree, that this wouldn't be very nice UX wise. I'd say, we throw a generic Error Text and let the message be more specific.

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

Let's wait for the error handler service from ✨ Print details from error messages returned by paypal Active , after which we continue here.

POSTPONED on https://www.drupal.org/project/commerce_paypal/issues/3490120 ✨ Print details from error messages returned by paypal Active .

Ok, that should be it! Please review!

Note that this service is currently pretty "barebones", but can be extended in follow-up issues like 🐛 Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Active . We think it only makes sense to do the error handling in one place, which we can further refine in the future.

@rakesh.regar provided @tomtech's patch as an MR.

Changes LGTM and makes the JS a bit more safe. Unsure if it will fix the original issue, as there aren't clear steps to reproduce (other the ones from @tomtech).

The only thing is, that in the case "data.status" is "403", the added message will quickly disappear through location.reload(). Other than that, it looks fine!

Let's hear feedback from others.

This is meant as a general solution for all cases where a "BadResponseException" is being processed. Sorry if the MR confused you. I just picked out one place where this is the case.

A service would be preferable if we want to hide information about specific errors or specific roles.

List of the available errors and their messages: https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187...

But what's shown in the screenshot looks kinda buggy (as we see the tags etc)

Yea just quick concept code, I wanted to hear your feedback first.

Here a first concept how this could look like. Of course, we should streamline this approach through moving this code into its own service and using the service, whenever we catch a "BadResponseException".

Inside the service we could also filter out, for which error we want to display further information and for which not (if needed). We can also differentiate between what error should be thrown for an admin user vs. what should be shown for a normal user (using a dedicated permission).

What are your thoughts on this @jsacksick? Would this be worth pursuing? Here is a screenshot of the current MR (although the Markup is escaped):

Thank you!

@jsacksick you forgot to initiate the UUID service! Which currently leads to the following error when refunding:

Error: Call to a member function generate() on null in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->refundPayment() (line 669 of modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).

Interestingly, I can't even refund a payment on latest dev release.

I get the following error:

Error: Call to a member function generate() on null in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->refundPayment() (line 669 of modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).

Here is a list of all available issue descriptions:
https://github.com/paypal/paypal-js/blob/a2e716e0a0982fe79c8c51e8c04e187... (note, that 422 is UNPROCESSABLE_ENTITY).

IMO, the error message descriptions are fairly safe in an admin context.

Of course we have to be careful to not publish sensitive information and should have a look at PayPals own implementations, in which cases they espose these messages.

There are no user information in these error descriptions.

Alright, I fixed the remaining issues noted in #28. That should be it now!

Ok, this is it! Done!

Thanks for the feedback @larowlan! We'll discuss this internally and come back to you!

Talked with @anybody internally. Not really worth taking a deeper look at this.

This shouldn't happen. "ignore_tags" has their default setting defined in the Doctrine Annotation above each filter plugin. So "ignore_tags" should always resolve at least to an empty string, not NULL. Maybe a simple clear all caches will fix the issue?

There is no need for an update hook, since this is a plugin. Filters never implement "defaultConfiguration()" for some reason. Not here nor in core. They are always defined in the doctrine annotation / PHP Attributes.

Done, please review.

grevil → created an issue.

grevil → created an issue.

Alright, that should be it!

All done, please review!

Ok, I will prepare everything to declare "theme" deprecated. Thanks for your feedback!

EDIT: I created a CR draft: https://www.drupal.org/node/3523553 →

grevil → created an issue.

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

But I am generally confused, why we are doing so much fuss about this? IMO, we should just adjust the label of "controls" from "Show playback controls" to "Use native controls" and overwrite the default from TRUE to FALSE and be done with it.

I can't seem to find any code unsetting "controls", or am I missing something here? As already stated before in #8 "controls" is not defined by us, but comes from "FileMediaFormatterBase", meaning it needs to be unset explicitly in the "VidstackPlayerVideoFormatter".

Ok, the script is present now! Unfortunately at the time, that the vidstack player gets initiated, klaro hasn't finished processing the script yet, and therefore we still have the same warning in the console output:

Uncaught Vidstack Player JS library could not be loaded.

So if klaro is present, we somehow need to wait for klaro until we can fire our script. I tried to add klaro/klaro as a dependency to our script, but it doesn't seem to fix the problem...

@anybody, any idea? Should we continue with this problem in the vidstack_player module?

@anybody I updated the IS just yesterday:

Only instance in core would be BlockForm, but there the condition is manually unset, so it needs to be manually set again before testing

I will add the info, that it needs to get manually set in code first, together with the line.

Agreed. I ended support for the latest 4.x release.

Should be fixed in 5.x.

4.x only receives security fixes.

Please reopen, if this issue still applies to 5.x

Alright, I changed the key combination to "Alt + h" (since the Key combo for admin toolbar search is "Alt + a" I thought "Alt + h" might be more in line.

Furthermore, I implemented the collapse / expand button. @thomas.frobieter will now begin for the final styling + remaining fixes.

After that, we can do a final DX discussion.

Did some minor adjustments. Generally works great and definitly helps! Great work @sascha_meissner!

Someone should take a final look at the CSS changes and test it with the gin theme active.

Furthermore, I am unsure if the current key combination of "ctrl + alt + h" will suffice for toggling the toolbar. Maybe we should add a small button somewhere, which toggles the toolbar?

At last, we should discuss, whether the current key combination feels the most DX friendly and document the key combination somewhere (probably in the tooltip of the toggle button)

EDIT:
Ah, and:

Have an indicator that your toolbar is currently not displayed [...].

(#48)

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

Thanks all!

We shouldn't support Drupal 6 @anybody :P

We also shouldn't create a 6.x release yet. Otherwise, we will be getting into the cherry-pick fiasco again.

Added a few omments.

Branch is 19 commits behind and needs to get rebased.

Are we sure we still want this? If still relevant, we should target 6.x.

Thank you, @9mattjones86.

Feel free to create a GitHub issue @https://github.com/dimsemenov/PhotoSwipe/tree/master.

I could not replicate this problem. Could you define steps to reproduce this issue?

The fix provided will work for both media and file.

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

Never mind, the test was incorrect after all. I added the test changes to 5.x dev.

Ok, test failure is actually valid but unrelated, I will create a follow-up issue for that.

RTBC!

Let's wait for tests to succeed first.

Fixed! Fairly simple fix.

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

Thanks! So summarizing this, there are two questions left:

1. Should we leverage the batch API?
2. Should we preserve the "changed" date?

Let's wait for @grimreaper's feedback!

Ok, I adjusted the code, so that we now use checkboxes instead of the select. As @anybody and @yoroy already mentioned, this makes much more sense UX wise.

The only problem is, that this change requires an update hook, as we can now select multiple themes instead of only one. But I can't seem to find a good way on how to update a condition configuration, as it usually just gets merged into the form it was used. @larowlan do you know of a good way on how to update all form configs using the condition? If not, we could alternatively leave the "theme" config in as a fallback (simply wrapping an array around it) and deprecate it. Although I am not super keen to do so...

Anyway, apart from the update hook this can go back to "Needs Review", note that I did a total rewrite, so this needs to get reviewed throughout again.