🇮🇱Israel @jsacksick

There is only support in the admin for payment gateways that support tokenizations, which isn't the case for PayPal Checkout.

Checked very quickly the logic, the first thing that stands out to me is the fact that we're passing the $meta array as is to createFromPurchasableEntity(), prior to any filtering, is that expected?

Thanks for the help with testing!

Let's see if the following helps...

Looks like a test broke which is problematic... It seems the "formatted" property is no longer returned:

1) Drupal\Tests\commerce_product\Functional\Jsonapi\ProductVariationResourceTest::testGetIndividual
The 'data' member was not as expected.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
         5 => '456DEF'
         6 => 'en'
         7 => Array (
-            0 => '$4.00'
-            1 => '4.00'
-            2 => 'USD'
+            0 => '4.000000'
+            1 => 'USD'
         )
         8 => true
         9 => 'hwryDce3'
     )
 )

Tests are fixed! Merged!

@Grevil: Can you check if the attached patch helps?

jsacksick → created an issue.

Please read: https://drupalize.me/tutorial/overview-theming-views.

As a reminder, support requests are generally NOT answered from this queue:

With the abundance of support available in Drupal Answers, the Drupal focused Stack Exchange site, we are no longer answering support requests in the issue queue.

Support requests opened here will be closed (won't fix), so please search for existing answers or post new questions at:

https://drupal.stackexchange.com (using the drupal-commerce tag)

Arity is needed, I think this is necessary when you don't want to combine order items... this is the "index"/"delta".

The tests should check that the validation is working, that the unit price field cannot be set, and that other custom fields can be set.

Tests are failing, and we might need new tests as well.

I don't really understand this report. Is this a support request? A bug report?

With the abundance of support available in Drupal Answers, the Drupal focused Stack Exchange site, we are no longer answering support requests in the issue queue.

Support requests opened here will be closed (won't fix), so please search for existing answers or post new questions at:

https://drupal.stackexchange.com (using the drupal-commerce tag)

To prevent setting base fields, we used to have the following code in OrderProfile::propertyDefinitions().

$excluded_fields = array_merge(
      array_values($entity_type->getKeys()),
      array_values($entity_type->getRevisionMetadataKeys()),
      ['is_default', 'data', 'created', 'changed'],
    );
    $fields = array_filter($fields, function ($item) {
      return !($item instanceof BaseFieldDefinition);
    });

Don't think hardcording the "field_" prefix is a good idea. The "field_" prefix can be easily removed and in fact, in many of the projects I'm working on, we just skip it.
Also, not a big fan of enforcing required fields to be submitted, I see this as a breaking change... And +1 on preventing setting the unit price. I started by a simple patch in #16, and this is now going into a complete different direction.

I think we should validate only the fields that are passed, validating a field that isn't passed could be blocking / limiting and frustrating :). And we should prevent setting the unit price, other than that... Passing fields via meta remains a workaround as usuaully the right way to do this is via attributes, this was needed on a project I was working on last year, but this approach is also problematic for more complex fields (and reference fields for example, as this would allow for setting any reference without real access checks / validations...

There's no such thing as a "major" feature request.
Furthermore, ProductVariationAttribtesWidgetCustom isn't provided by Commerce, this must be a custom widget present in your project.

The ProductVariationWidgetBase doesn't have caching around the loadEnabledVariations() method. Also there is no way loading 4000 variations at once on a single page can work properly.

You'd have to write your own widget with your own logic... Not something that can be provided as part of Commerce core...

@phannphong: Could you upload an interdiff? Also FYI Commerce core itself has been requiring PHP8+ for a long time already.

One thing I was wondering, should we dispatch an event so we can set the payment_method from an event subscriber that lives in commerce_payment? Also createWithSampleValues() isn't static (at least the parent method isn't). If we don't go with the event, I guess we should go with a moduleExists() check (ofc less elegant).

Then the system starts to create a Product entity with sample values, and creates quite a lot of other data.

And that is a problem?

This isn't really a "major" issue as entering a huge price like this is probably affecting demo sites or people just trying random things... But still, this should be fixed and we shouldn't crash like this.

@kumkum29: Stripe payment element is implemented as an offsite gateway, and does not implement the SupportsCreatingPaymentMethodInterface interface, which would is the reason why it's filtered out by the code I posted in the related Commerce issue.
This means we cannot tokenize payment methods from the user pages. I'm not sure if there is a technical reason behind it, but this explains why at least.

Closing this as a duplicate of #3123333: Error: Customer requested on payment throws InvalidRequestException → since the reported issue looks similar to the issue you've referenced.

Access denied is returned in case there are no payment options. You need to configure a payment gateway that supports tokenization... For example Commerce Stripe, Commerce Braintree, Commerce Authorize.net...

See the code below:

if (!$payment_options) {
      $payment_options = $this->buildPaymentOptions($form_state);
      if (!$payment_options) {
        throw new AccessDeniedHttpException();
      }
      $form_state->set('payment_options', $payment_options);
    }

ANd then buildPaymentOptions() has that:

    $payment_gateway_storage = $this->entityTypeManager->getStorage('commerce_payment_gateway');
    $payment_gateways = $payment_gateway_storage->loadByProperties(['status' => TRUE]);
    $payment_gateways = array_filter($payment_gateways, function ($payment_gateway) {
      return $payment_gateway->getPlugin() instanceof SupportsCreatingPaymentMethodsInterface;
    });

Or perhaps the right fix is to just set a query_access handler?

The attached should do.

Does the attached patch help?

The hook_commerce_checkout_pane_info() was necessary in Commerce 1.x (the D7 version), not in Commerce 2. The order shouldn't be saved from submitPaneForm() as this is done by the checkout flow plugin already.
Just FYI, submitPaneForm() is invoked when the global form is submitted, not right when the checkbox is checked.

With the abundance of support available in Drupal Answers, the Drupal focused Stack Exchange site, we are no longer answering support requests in the issue queue.

Support requests opened here will be closed (won't fix), so please search for existing answers or post new questions at:

https://drupal.stackexchange.com (using the drupal-commerce tag)

This change doesn't look right... There is no payment_method field if there is no payment module enabled? The payment_method field is added as a base field by the commerce_payment module.

I guess we can consider your fix but in the meantime you could just have a condition that checks if $form_state->has('selected_variation'); otherwise you can fallback to the first variation referenced from the product.

@ozin: The second point from the issue was ignored.
Let's update the error message to be:

Coupon code matching is case-insensitive, and codes must be unique. %value cannot be used because it matches an existing code.

ofc we need to update the tests accordingly.

Getting the existing code would require defining a custom constraint validator and considering the logic baked into that validator, let's pick an easier route and let's just update the validation message.

Posting a backtrace would help... WIthout it, not much we can do...

Merged.

Does that mean once we add apple_pay and google_pay in Commerce core, those from Braintree would stay defined as braintree_apple_pay, , not as apple_pay method even if it is?

Well not really, we could write an update hook to migrate to those... Challenge is figuring out the fields needed for each gateway in advance in core... But I guess modules could also add their fields on top of the ones defined by core? That'd require implementing both a hook_entity_field_storage_info() and hook_entity_bundle_field_info() though... Perhaps we should make it easier to extend bundle plugin field definitions.

That sounds like a support request which aren't answered from this queue...
This bug report isn't one... There is nothing in your report that can help pin point the issue.

This is necessary when selling custom product with a refresh price

Can you elaborate? "selected_variation" isn't set in this case because that is used by the ajax refresh callback, but there are no attributes elements in case of a single variation?

We have to prefix the payment method types defined by "braintree_". This isn't done for PayPal, but Braintree is PayPal (sort of), but don't want to be blocked in the future if Commerce ever defines an Apple Pay and a Google Pay payment method type.
Braintree cannot "own" those.

Works great, 2 comments though: navigator.clipboard.writeText is async, it returns a promise so technically:

The following try catch isn't going to work.

            try {
              const successful = navigator.clipboard.writeText(link);
              if (!successful) {
                console.log('Failed to copy link');
              }
            } catch (err) {
              console.log('Failed to copy link');
            }

So we either need to convert the code to look like this:

navigator.clipboard.writeText("This is the text to be copied").then(() => {
  console.log('Content copied to clipboard');
  /* Resolved - text copied to clipboard successfully */
},() => {
  console.error('Failed to copy');
  /* Rejected - text failed to copy to the clipboard */
});

Or create a function and make it async:

async function copyContent() {
  try {
    await navigator.clipboard.writeText('This is the text to be copied');
    console.log('Content copied to clipboard');
    /* Resolved - text copied to clipboard successfully */
  } catch (err) {
    console.error('Failed to copy: ', err);
    /* Rejected - text failed to copy to the clipboard */
  }
}

Additionally, there is no visual feedback after successfully copying the text, like on Github (See the attached screenshot):

Let's use the following icon for this: https://icons.getbootstrap.com/icons/check-lg/.

Also, we need to store the device_data in the payment method. JSON encoding the remote ID doesn't seem like the right approach to me... This would be a breaking change.

Attaching a patch for use in composer.json.

Hi @Povilas Uogintas and thanks for your work. Would it be possible to split the changes? For example, make a separate patch for collecting / passing the device data?
I think your approach is correct (add a setting to toggle the device data collection).

What I don't understand is that the "device_data" parameter is described as required in some cases, but haven't seen any "complaints" in our queue and not sure why...

To get your changes in, splitting this into different feature requests is definitely going to make this easier.

The "completed" time is when the order state was set to "completed", which is likely not the case when the order is placed... You're likely looking for the order placed time instead...
Also, this would just print a timestamp which is probably not what you want... Additionally, support requests are generally not answered from this queue.

I think you're looking for "getPlacedTime".
So something like...
({{ order_entity.getPlacedTime|format_date('html_date') }})

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

Thank you Andy!

This is the followup issue for reference: 🐛 Main layout region is too narrow for our entity forms on wide screens Fixed .

The Claro approach was copied, there was a followup issue addressed as the width got reduced on the order view page accidentally too. I'm also not a huge fan of the restricted width for the edit forms, but I guess we just followed Claro's lead.

Perhaps removing the libraries was too brutal... Did that cause you any trouble? But I guess even keeping the files would have been problematic, since the libraries.yml definition was removed right?

Updating the order state isn't really a good idea, because if the order is no longer a draft, your customer isn't going to be able to resume checkout and it would no longer be an "eligible" cart.

There's no such thing as a "critical" support request. Unless you provide a backtrace, or clearly reproducible steps on a clean install, there isn't much we can do... This error could be caused by 10000 different reasons...

Merged, thanks!

There has been work around improving that recently. We're leveraging commerce_log for that. Log entities are created on payment failures (See for example ✨ Add logging for payment failures during checkout Fixed , or 📌 Make it easy to add payment info to \Drupal\commerce_payment\Exception\PaymentGatewayException Active .

Guessing you could somehow using relationships build a view of orders that have logs of a certain type?

In order to not to break the existing UI, I feel like we should add a threshold to the existing UI (e.g: Up to 50 attributes we just keep the existing UI), and if there is more than that, we should skip the inline form completely or something?

We were missing the test group (added that). But also suddenly not sure about the fix, whenever I don't skip the test methods defined by the base class, there seems to be an issue with the denormalization of the adjustments field... Also... now extending getSupportedTypes() from the normalizer as the previous approach :

  /**
   * {@inheritdoc}
   */
  protected $supportedInterfaceOrClass = Any::class;

is deprecated in D10.2. I'd need to double check when this change was introduced to see if we need ot raise the required core version... Since I'm not sure the issue is actually fixed... Putting this issue on hold for now.

Please remember that it is necessary to proceed with the payment by bank transfer to complete your purchase.

With PayPal?

Otherwise I would have to modify the Completion message pane text programmatically.

Because you're looking to output different messages based on the payment gateway chosen?

What would you put in that field if I may ask? The field is there for manual payments as this gives merchants the ability to provide instructions on how to proceed with the payment? But for PayPal, the order is technically already paid once you complete checkout?

The prefered approach is the cron fix.
The advancedqueue patch has been committed, and the patch leveraging it is ready (See 🐛 multiple recurring orders get created if cron is badly configured Needs work )L

Are you sure this is the only place that is problematic to support this? There's probably plenty of code assuming variation types are returned by $product_type->getVariationTypeIds().

The event to listen to is:
commerce_order.cancel.pre_transition (fired on order presave) or commerce_order.cancel.post_transition (fired after the order is saved).

The state machine module is responsible for dispatching events when a state transition is applied.

hm.. now that I'm looking at this code, I think a mistake was made, the profile uid should be set to 0, since this is creating billing/shipping profiles which should technically be owned by the order and/or the shipment.

Actually, let's go with this patch... Which provides the "Any" normalizer conditionally in case it is not defined by the serialization module.

Perhaps we could provide our normalizer, only if the core one introduced in #2915705: TypedData 'Any' can't be normalized to array if it stores an object → doesn't exist?

The attached patch works... However I don't think we can commit it as this means Commerce just provides a normalizer for the "Any" data type, which is used by other field types... which is a problem... But attaching the patch anyway..

Did you review the comment from @tBKoT? Probaly temporarily disabling the webform_product module to see if this is still happening?

These are errors that can't be reproduced on a clean install... Something probably went wrong while upgrading to D10 or something (referring to the other issues reported already)...

Can you try resaving your product type perhaps? This error would seem to indicate the variation types setting isn't properly configured... Could be that it was set in an update hook, and then the change got reverted when importing config or something?

@trickfun: the patch was reverted. I still believe the right fix is within the Entity API module.

Hi, why did you apply the patch in the first place? I'm confused, the patch from the issue linked was reverted.

@jsacksick, Commerce API is a dependency of Commerce Cart Flyout.

No, Commerce Cart API is, not Commerce API.

Also, your fix would set a precedent, if we start doing that here, we'd need similar "hacks"/workarounds in pretty much all other checkout panes... Except yeah, this one might be the only one saving the order at this stage.

hm... I'm not sure why you're bringing up Commerce API here? Could you be a little bit more specific? Are you manipulating the same order via the API and via the regular fullstack checkout at the same time?

Marking this as needs work, we should favor leveraging the Advancedqueue support for unique jobs instead...

You're somehow on a broken install... This isn't a critical issue as something is obviously wrong with your setup.

Try running the following code:

$entity_type = \Drupal::entityTypeManager()->getDefinition('commerce_payment_method');
\Drupal::service('entity.bundle_plugin_installer')->installBundles($entity_type, ['commerce_paypal']);

You mentioned on the other duplicate issue you've created being logged in... I'm not sure that has any impact at all... The profile is owned by the shipment, it has a uid = 0... In other words, it is NOT attached to the currently logged in user, but to the shipment.
Is this happening for all of your orders? Did you try checking out as another user? Or as anonymous?

Why are you opening 2 different bug reports for the same issue?

But it's a view...

I think that's the View for the Active tab (admin/commerce/orders/list). I am talking about the Orders tab which is at (admin/commerce/orders).

admin/commerce/orders/list is admin/commerce/orders...

jsacksick → created an issue.

jsacksick → created an issue.

Actually, didn't realize the query was changed in the pricelist builder, the changes are wrong, we're no longer sorting by weight there, this has to be fixed ASAP as it is very confusing... Working on restoring the sort in a followup issue.

@tBKoT: Can you look into using the highest value between the created and the renewed timestamp as suggested in comment #13?

Makes sense, merged!

Can you attach a screenshot? Also wondering about Views? No idea if some people override the default list by a view.

Thanks everyone! Merged!

I think we should skip loading entities all together... Doing direct update queries to the DB makes the most sense here... We don't really want to involve loading / saving entities... It'd take hours for a site with hundreds of thousands of entities this way... So let's just write an update hook that performs direct update queries to the subscription table, similar to what we did for setting the balance field for orders in the latest Commece release.
Removing the "needs tests" tag as tests were added.

hm I'd say "UNIQUE_DONT_OVERWRITE" as we don't want to queue duplicate jobs.

How is that not available in 2.9? The change was committed? There are 2 new operators.

I'd be a major issue if there was a way to constantly reproduce the issue. This isn't happening on a Vanilla install, otherwise tests would be failing.
Do you have any custom code that could be causing this?