🇮🇱Israel @jsacksick

Merged!

Haven't converted OrderBillingAddress and OrderShippingAddress yet due to the arbitrary key that is used there ("profile_scope").
It doesn't feel right for the attribute constructor to define it, but I wonder what alternative we have here...

jsacksick → created an issue.

jsacksick → created an issue.

jsacksick → created an issue.

I started by defining TaxType and TaxNumberType attributes... But now I'm wondering... Perhaps it makes sense to prefix our attributes (just like our annotations, by Commerce, to be consistent with what was done for annotations...)

So TaxType would become CommerceTaxType for example.

jsacksick → created an issue.

jsacksick → created an issue.

FYI, that change broke all my migrations... After I reverted to 6.0.2, all my migrations ran successfully again...
Not really sure what information I can provide, somehow it seems no rows was detected by the migration.

None of the changes look like breaking changes, so we can probably make the changes in both branches actually. I said we should target 3.x without looking at the changes, my bad. Surprised these are the only changes required :).

Let's address the points I raised, then we can merge this, though I haven't been actively maintaining IEF, so perhaps geek-merlin would like to do that?

I think we should support D11 in the 3.x branch, that makes more sense to me so we can discontinue the 1.x branch.
Thoughts here?

jsacksick → created an issue.

Update latest release of IEF version in composer.json so that any module requiring commerce module should be able to download it. Later on for dev dependencies as well in order to make D11 CI passing

That isn't needed, as soon as a D11 compatible release is going to be tagged, it is going to be picked up... Unless D11 support is added in a new major branch, but I'd expect the IEF 3.x branch to support D11, need to coordinate with the other maintainers on that.

Make sense, but need to keep this issue open for other changes needed for D11 compatible release.

What are these?

I think I can still go ahead and merge those changes since the required changes for D11 to work are non dependent on Commerce anymore, and I wouldn't want the changes to become stale and have to handle conflicts later...

"Name on card." is probably coming from a contrib payment gateway module? You'd have to open an issue there if it's missing a t() somewhere.
Regarding "Billing information", all occurrences of these strings are wrapped in t(), so not sure why this isn't translating...

Tests are now finally green! However wondering if we shouldn't wait for the dependencies to have D11 compatible releases before merging this? Or I guess we can merge the changes and tests will pass once the dependencies will have D11 releases?

Merged, thanks!

Let's open an MR with the changes, to see if the tests are passing, if the initial proposed patch works, let's roll with it.
@rhovland: could you open an MR please?

No this can't be merged as is and yes, we're going to need to remove the custom repositories and custom branches.
But in order for Commerce to be fully compatible with D11, we need to make sure our dependencies have D11 compatible releases.

See https://git.drupalcode.org/project/commerce/-/jobs/2206237

jsacksick → created an issue.

Profile & State machine both have D11 compatible releases...

Only problem I see is we aren't using dependency injection here.

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

Not against the idea, but the problem is the signature change especially for the WorkflowTransition... The description should be added last, and should default to an empty string so we don't break any code instantiating workflow transition objects.

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

This commit? https://git.drupalcode.org/project/commerce/-/merge_requests/279/diffs?c...

Profile is already D11 compatible, so I don't understand why we'd need to pull a specific MR? Will work on D11 compatibility for State machine, as for IEF, that might still be needed yes.

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

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

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

This is too late to fix, See https://www.drupal.org/project/physical/issues/3345698#comment-15025989 📌 Align with the commerceguys/intl changes Fixed for a workaround.

Thx!

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

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

There are some changes that seem wrong in the MR...
Also, since this is 3.x, let's require Drupal 10.3 minimum so we can simplify code like the following:

method_exists($renderer, 'renderInIsolation') ? 'renderInIsolation': 'renderPlain';

I'll take this over, thanks for the work done so far.

Closing this back as a duplicate, don't know if we can add credits after the MR got merged already, will try that in the other issue.

Merged!

This is a support request which 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)

FYI, once you override the template from the theme, you have to rebuild caches so your override is taken into account.

Also, you can get the first name and last name using the following code:

  /** @var Drupal\commerce_order\Entity\OrderInterface $order */
  $order = $variables['order_entity'];
  if ($billing_profile = $order->getBillingProfile()) {
      /** @var \Drupal\address\AddressInterface $address */
      $address = $billing_profile->get('address')->first();
     // First name.
     $address->getGivenName(),
     // Last name.
     $address->getFamilyName(),
  }

Can we close this then?

But I still think, sending the receipt email should already be evaluated in the order's customer context.

What you're suggesting would imply that the order receipt should switch the active user to the order customer. I think that is "risky". Admins are allow to resend receipts from the admin and they should have sufficient rights for the receipt to be rendered without issues.

Went ahead and merged the request, let's move forward and address what's remaining as followup issues like suggested.

The variation field injection is performed by the ProductViewBuilder. In another words, this logic kicks in only when products are rendered (in any view mode).

So, this can be triggered if you're rendering products in any view mode using Views. This is NOT triggered if you're rendering fields and simply rendering variations using the add to cart form formatter.

I think the problem here is simply cron that's ran as user 0 (anonymous).
What's happening is that the order item access is determined by the access on the order. Considering cron is executed as user 0, this can't work.

This is more like a support request than a bug report.

For example, if you'd like to run your cron as user 1, you can do the following:

$account_switcher = \Drupal::service('account_switcher');
$account_switcher->switchTo(new UserSession(['uid' => 1]));
// Run your code...
// then call the switchBack() method.
$account_switcher->switchBack();

@Richard Cheung: Are you saying your payment gateways have a condition on the order customer role rather than the current customer role?

We now have a condition on the current user role (See #2965067: Add a condition to filter by current user role → ).

Actually, disabling the profile copying would have to be done from Commerce shipping itself, since it is the one adding that checkbox, via the following code:

/**
 * Implements hook_commerce_inline_form_PLUGIN_ID_alter().
 */
function commerce_shipping_commerce_inline_form_customer_profile_alter(array &$inline_form, FormStateInterface $form_state, array &$complete_form) {
  // Attach the "Billing same as shipping" element.
  $profile_field_copy = \Drupal::service('commerce_shipping.profile_field_copy');
  if ($profile_field_copy->supportsForm($inline_form, $form_state)) {
    $profile_field_copy->alterForm($inline_form, $form_state);
  }
}

We'd probably need to alter the supportsForm() method to check if the form is the order edit form.

Also, not sure when we started diverging, See #3314776: Synchronize the order admin view page and edit form meta regions → .

Ok, I opened a MR that makes changes to the order form. Only thing that looks weird to me is that we show the completed time before the placed timestamp... But probably ok to leave this as is...

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

Hi, I'm not sure I fully understand this feature request. Are you trying to build a view that outputs products that are discounted using promotions?

Now try to filter on products which are on offer to show only those products which are within the date range, this is however not possible.

Not sure what you mean here by data range?

Regarding "Price different formatter", are you referring to https://www.drupal.org/project/price_difference_formatter → ?

This can happen after uninstalling a payment module reusing fields from other payment method bundles (e.g commerce_paypal).

However, this has to be fixed either from the payment gateway defining the payment method type, or by addressing / fixing #3203352: Allow field storage definitions to be reused across bundles → .

You can add a sticky field just like nodes indeed.

Marking this as RTBC still, as we have tests and we've implemented the approach I favored (i.e. the boolean).
Perhaps we can update the test that was added to confirm the job types that were queued?

Basically, ensure that we have one "commerce_recurring_order_close" and another job for renewing the order "commerce_recurring_order_renew".

I think:

- set a limit on the query to avoid processing more than reasonable number of orders at the same time

should be addressed in a followup issue yes.

MR looks good to me, but let's see what other people think about it.

- adjust the order query to avoid orders that are already queued from being processed

I think we should do that already no? We can probably add a notExists()? Or maybe an OR condition group on notExists() + commerce_recurring_queued != 1.

Merged, thanks!

There is also the core_version_requirement that has to change, and there is more that's needed, we need to remove the use of deprecated code... Was planning to come to that at some point.

We use billing period because that is how it's called in the code everywhere...
We may have to update all prepaid orders yes, I understand this is making you nervous, however this has to be fixed as the current situation is really confusing and is far from ideal...

I think we need an update hook updating all opened recurring draft orders referencing prepaid billing schedules, to ensure their billing periods is adjusted. I'm just hoping that doing this isn't going to cause a billing cycle to be skipped or something.

So actually revising what I said in #13.

What we could do is a following, loop on all billing schedules, and separate prepaid from postpaid billing schedules. We essentially need an array containing prepaid billing schedule IDS and another one containing postpaid billing schedule IDS.

Then, assuming we fix the initial billing period, we could adjust the query to do the following:

• Create a first "AND" condition group to have a condition on the both the end date and a condition to get orders referencing postpaid billing schedules.
• Create a second AND condition group to have a condition on the start date AND a reference to a "prepaid" billing schedule.
• Then an OR condition group to get either recurring orders with a billing period end date in the past referencing postpaid billing schedules OR recurring orders with a billing period start date in the past AND referencing a prepaid billing schedule.

But what about existing orders? This would be a breaking change as we wouldn't bill existing orders properly unfortunately.

Using loadForUpdate() makes sense IMO, I don't really consider this a scope creep. The method didn't previously exist, so why not using it to make this even more robust.

In regards to the boolean field, the only concern I see is in #93 and it's about setting a default value. I think this concern is valid whether we use lock, data, or boolean field.

Regardless of the picked solution, we can't properly backfill I mea, it is ok to me if a "queued" boolean or whatever name we picked has 0 even if the recurring order is placed, what really matters to me is fixing the actual issue and making sure we no longer queue the same recurring order for closing / renewal.

Just to clarify, I don't disagree that there are things that can and should be improved. But changing this now will lead to other issues and other people complaining that it doesn't work like the way it used to be, that's all I'm saying...

It's hard to find the right behavior that'd satisfy everyone here... Depending on the business and what's being sold (digital vs physical etc)...

#83 is leveraging the Advancedqueue patch, but once again, I'm no longer convinced relying on Advancedqueue to fix duplicates is the answer.
Also, we're collecting subscriptions by passing the order to the RecurringOrderManager, so looks like we can't easily avoid loading the orders without rewriting the cron job completely.

Additionally, #102 is a pragmatic approach to an issue that was opened 7 years ago in an attempt to not drag this further and revisit this later since we can't really find an agreement on a "perfect" solution.

I think the boolean makes more sense too, but it seems there were reservations about this approach too.

My concern is that, I certainly wouldn't expect changes (as a merchant) around how the title is handled for variations if I'm already running a live cycle, which is why I feel it is too late to change anything in Commerce 2 at this point.

Regarding:

The final thing that lead me here, is the slight visibility of the parent product in the product variation CRUD forms. (e.g. /product/43/variations/9/edit)

This is relatively easy to fix, by altering the route and specifying a different title callback. I'm guessing for some merchants, it might actually make sense to output the SKU in there too?

Also, the product title is visible in the breadcrumb.
Additionally, for a variation type that opts in to automatically generate variation titles, without actual attributes, the parent product title is going to be used.

@Anybody: Could you elaborate on the pain points you're currently experiencing? Are there no way to work around those?

So how should we proceed? Any chance to first get some Centarro feedback here to push this into the direction they like

Honestly, this wasn't really on my radar, this issue was initially opened 5 years ago and afaik, this never was a major concern on any of the projects I've been working on.

I had that one on my radar instead 🐛 Auto generated product variation title does not change when product title changes Needs review .
This late in the project lifecycle, this should target 3.x.

Core suffers from the same problem... Not sure what is the best way of fixing this...
You can reproduce the same issue with the number field type, specifying the same precision / scale.

Got the following error:

Numeric value out of range: 1264 Out of range value for column 'field_test_decimal_value'

We could add a try catch block around the $save_return = $this->entity->save(); in ProductVariationForm, but not convinced this is the right fix as we're supposed to just save the entity from there...

The attached patch is setting the #max value, but I don't really like this fix.

The "unique" job support that was introduced isn't really helping us...
It wouldn't prevent a job to be queued if it failed...

See 🐛 In some cases the duplicated jobs are created Active .

Probably silly idea: add a "queued" state?

Not a good idea, the workflow can be changed.

Maybe we'll just go with the data flag... And in theory, if the queue is properly processed, we shouldn't be loading the same orders over and over...

@rszrama: There is currently nothing we could query on to simply adapt the query. There is a billing period field on the order, and that just holds the start and the end date, nothing else.

The subscription references the billing schedule entity, and the billing type (prepaid vs postpaid) is stored on that config entity, so unfortunately, nothing we can query on.

I believe this is why the current logic works as is... We always close recurring orders at the end of the billing period... I wonder if we shouldn't find a way to simply "cheat" and just display a different billing period. This is far from a perfect solution, but it'd solve the display problem I suppose...

Merged, thank you!

With 5px marging, and hopefully pushed another fix addressing the remaining test failures...

Added divs and a 10px top margin to the order date:

Also pushed a commit to hopefully address the test failures.

MR opened, let's see which tests are failing. The receipt looks like this now :)

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

Duplicate of 🐛 Overriding default invoice template Active and there are many others already in the queue :).

Duplicate of 🐛 The "overridden_unit_price" flag is never cleared after unchecking the checkbox Fixed .