🇮🇱Israel @jsacksick

A change record might be a good idea, especially since the functionality isn't going to be turned on for existing installs.

@anybody: I really don't know when TBH, there are other priorities on my todo, this is a feature request which means no guarantee that the change is going to be accepted as is.
As a side note, there are conflicts.

This change also implies changes to the localization which be problematic for existing installs.

Yeah, in which case it's a harmless error... It could be that the url is blocked and cannot be accessed from the environment, not so much we can do here.

jsacksick → changed the visibility of the branch 3528108-stripe-connect-implementation to hidden.

jsacksick → created an issue. See original summary → .

There is also:

Deprecated: Drupal\jsonapi_hypermedia\AccessRestrictedLink::__construct(): Implicitly marking parameter $target as nullable is deprecated, the explicit nullable type must be used instead

Deprecated: Drupal\jsonapi_hypermedia\AccessRestrictedLink::createLink(): Implicitly marking parameter $context as nullable is deprecated, the explicit nullable type must be used instead.

jsacksick → created an issue.

The email is no longer sent on destruct() so you can try updating to the latest version.

The MR should fix this.

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

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

Merged! 🎉! Thanks everyone!!

So RTBC then? Would be great to finally close this... I think this is ready! Would appreciate an RTBC.

I'm also not sure what this is fixing... There's just too little details.

@goz: What we don't see in your response is whether the order is a draft order? IF it is a draft order then on order save, the adjustment will be cleared by the OrderRefresh service right? So that is probably expected? You'd need to try on a non draft order.

jsacksick → created an issue.

Which Drupal core version are you on? Are you still on Drupal 9? I'm thinking we should drop compatibility as it's no longer supported.

Actually, now having second thoughts on the approach... Perhaps we should simply add a tax type setting... "Append the tax rate to the adjustment label"... Would remove the need for the subscriber and can be easily toggled within the UI.

I made some changes / improvements to the patch.

1. We were not checking that the adjustment percentage was not empty from the subscriber (this could have caused errors, altering adjustments with no percentage set)...
2. All tax adjustment labels were altered, regardless if the adjustment was added by a non local tax type plugin (For example, we have to make sure to not alter the adjustments added by the Avatax module as the module provides its own custom labels)
3. Renamed the subscriber to OrderTotalSummary
4. Fix the variables casing to stay consistent with the Commerce codebase and especially code within the same method (i.e changes to the OrderTotalSummary where camelCase was introduced)

I have no problem with the new event subscriber etc... But that is a change we probably should have introduced prior to tagging 3.0.0 as I'm pretty sure the change introduced will not please anyone... Merging this will cause the adjustment label to change instantly on live sites as well...

Also, there are test failures, and we have no new tests testing the new label... So not 100% convinced we can just go ahead and commit this change as is.

We might need to merge the 3.x changes and kick off the tests to see where things stand.

@rhovland: Does the current patch allow for that? (from the MR)? I'm considering merging it as it looks ready.

I added a checkout flow setting:

    $form['display_sidebar_checkout_complete'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Display sidebar on checkout completion'),
      '#description' => $this->t('Whether the sidebar should be shown on the checkout completion page.'),
      '#default_value' => $this->configuration['display_sidebar_checkout_complete'],
    ];

The default configuration is FALSE (again for the sake of not breaking existing sites).

Here is how the checkout completion page looks like after the changes:

jsacksick → created an issue.

The thing is I'm not sure this is the right fix as the label method has the following signature:

  /**
   * Gets the label of the entity.
   *
   * @return string|\Drupal\Core\StringTranslation\TranslatableMarkup|null
   *   The label of the entity, or NULL if there is no label defined.
   */
  public function label();

The label method can return NULL, so modules dealing with labels should account for that, so to me this is a won't fix.

Changed my mind on this and decided to commit the update hook after testing it on a site experiencing this. Thank you!

The environment is fixed.

When the email is resent, it doesn't necessarily make sense to resend to the BCC (which is why it was implemented that way).
You're free to alter the form itself if you want a different behavior. Also in the patch you're loading the order type even though this happens right after already.

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

I'm not sure it makes sense to call it BadResponseExceptionHandler as it means we're deriving the name on the service based on the exception thrown. What if tomorrow we simply catch Request exceptions instead? Or if Guzzle renames this?

Also, Braintree defines an ErrorHelper for example with 2 static methods (See https://git.drupalcode.org/project/commerce_braintree/-/blob/8.x-1.x/src...).

Maybe we can call the service ErrorHelper too? Or perhaps PaymentExceptionHandler? Also, with what we're doing, wouldn't that mean that we get boh the generic Drupal Commerce error + this one? Which isn't necessarily good for UX?

For example, see here: #3489875: Needs to redirect users on UNPROCESSABLE_ENTITY > PAYER_ACTION_REQUIRED - No checkout possible Paypal expects to (re)direct the user to a certain action URL.

But this issue is just about refunds for now, right?

An error handler service or helper would make sense if we were doing some mapping between a code and a human readable message which isn't the case here? We're just spitting the error returned right?

Passing a formattable markup to an exception doesn't look very clean... Perhaps we just need to use the messenger?

Fixed this and tagged a new release.

This was already fixed as part of 📌 Remove translation and capitalization from the category annotation Active and should be in the latest release?
Wait which Commerce version are you using?

Actually you're reverting the fix? Probably an incompatibility between the Commerce version you're using and Commerce Product Tax.

Ok the way this should be handled is like the following to support earlier versions of Drupal:

  $constraint_definitions = $this->constraintManager->getDefinitions();
    if (isset($constraint_definitions['FileExtension'])) {
      $form['csv']['#upload_validators'] = [
        'FileExtension' => ['extensions' => 'csv'],
      ];
    }
    else {
      $form['csv']['#upload_validators'] = [
        'file_validate_extensions' => ['csv'],
      ];
    }

Ok removing the extra properties from the AdjustmentItem failed... But removing the AdjustmentPropertyDefinition worked... Wondering why it was added in the first place?

Do we actually need to declare each individual property like the following? The normalizer is on the value property, isn't that sufficient?
The normalization still works without those... Or is that needed for setting adjustments though the API?

$properties['type'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Type'))
      ->setComputed(TRUE);

    $properties['label'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Label'))
      ->setComputed(TRUE);

    $properties['amount'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Amount'))
      ->setComputed(TRUE);

    $properties['source_id'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Source ID'))
      ->setComputed(TRUE);

    $properties['percentage'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Amount'))
      ->setComputed(TRUE);

    $properties['included'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Included'))
      ->setComputed(TRUE);

    $properties['locked'] = DataDefinition::create('adjustment_property')
      ->setLabel(t('Locked'))
      ->setComputed(TRUE);

Isn't it

Also let's not introduce the new methods in Product and ProductVariation and focus on the initial issue solved.
I'm still not convinced by the approach as ideally we should also rewrite the logic that fixes the backreference to the product from the variation.

This logic isn't problematic when you're referencing a small number of variations but starting from 50 variations (which isn't that high, saving a product would become awfully slow)... And crash if the number is higher.

I am getting error: Undefined method: getVariationTypeId()

This is because the PR is against Commerce 2, the method was removed in Commerce 3, the patch needs to be rerolled.

Attaching a static patch for use with composer.

jsacksick → created an issue.

Attaching a static patch for use with Composer.

jsacksick → created an issue.

+1: would be great to have a new release containing the PHP 8.4 deprecation fix.

+1 on tagging a new release. PHP 8.4 has been released in November 2024 already.
Attaching a patch for use with Composer..

Already fixed as part of 📌 PHP 8.4: Implicitly nullable parameter declarations deprecated Active but unfortunately not released, would be great to tag a new release.

jsacksick → created an issue.

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

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

jsacksick → created an issue.

Also this doesn't look right:

    $config = [
      'mode' => $this->configuration['mode'] ?? '+aCJK',
      'format' => $this->configuration['format'] ?? $format,
      'autoScriptToLang' => $this->configuration['autoScriptToLang'] ?? TRUE,
      'autoLangToFont' => $this->configuration['autoLangToFont'] ?? TRUE,
      'tempDir' => \Drupal::service('file_system')->getTempDirectory(),
      'default_font_size' => $this->configuration['default_font_size'] ?? 0,
      'default_font' => $this->configuration['default_font'] ?? '',
      'orientation' => $this->configuration['default_paper_orientation'] ?? static::PORTRAIT,
    ];

    $config += $this->configuration;

Shouldn't we leverage the default configuration here?
Also the use of \Drupal::service() isn't recommended, we should be using dependency injections here.

Don't really have time to personally test this, but I'm willing to commit this considering this was RTBCED... However, aren't we missing changes to the composer.json? So that the mpdf library is populated?

Shouldn't we require mpdf/mpdf?

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

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

Attaching a static patch to use for 2.x.

Can we get new releases tagged? I actually didn't see this and created 🐛 Fix implicit nullables for PHP 8.4 Active . It's been a while and neither the latest 2.x or 3.x release have these fixes in.

Shoot... Realized this was already fixed as part of 🐛 Deprecations PHP 8.4 Active but not yet part of a tagged release...

jsacksick → created an issue.

I'm fine with validating all the fields just like the patch is doing atm.

There was a release tagged today?

jsacksick → created an issue.

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

I committed a slightly different fix, testing the interface instead.

Ok merged the MR 28.

I also see that the MR was closed, is this not relevant anymore?

Out of curiosity, why are you passing a quantity of 0? What is the use case for that? I'm just trying to understand how that is happening?

Marking this as fixed, as we've been supporting D10 for long.

I believe that issue is outdated.

Merged, thanks everyone!

I'm now using the CommerceContentEntityStorageSchema to allow specifying indexes straight from the entity type definition.

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

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

jsacksick → created an issue.

This is already fixed in dev.

Duplicate of 🐛 AssertionError: "commerce" must be defined in MODULE_NAME.field_type_categories.yml Active .

Committed the patch from #5.

@cslevy: I don't understand, is the patch from #4 or #5 the correct one? The patch from #5 is the same as #3 and the MR right?
If this fixes issues, I'm willing to commit the patch as is.