🇮🇱Israel @jsacksick

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.

jsacksick → created an issue.

jsacksick → created an issue.

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

Merged, thanks!

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

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

This was already fixed as part of 🐛 PHP 8.4: Implicitly marking parameter as nullable is deprecated Active .

Merged, thank you Klausi! 🎉

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

I fixed this and tagged a new release, updating to 2.2 should fix this.

Yes indeed, probably should have prioritized this whenever I worked on tagging a stable release. However I don't think I'm going to have time to work on this this week at the very least...

Hm, your patch isn't the correct one... Was probably optimistic to make these changes as I didn't realize this would imply forcing requiring Commerce 3.

Will fix this.

It's probably best if you install it to get more familiar with what the module can or cannot do.

Nope, there is no way to add new emails from the UI today.
We could potentially leverage Commerce email and add support for that though. That would actually be a nice addition.

hm... I guess this approach is ok though we could probably copy what the EditQuantity views field handler is doing:

$violations = $order_item
        ->validate()
        ->filterByFields(array_diff(array_keys($order_item->getFieldDefinitions()), [
          'purchased_entity',
          'quantity',
        ]));

This way we flag only errors for the purchased entity field and the quantity... Thoughts?

There is a "Resend confirmation" action from the invoice view page, as well as an operation on the invoices list.

I just don't see what needs to be answered here. The initial report is from 2012, and isn't relevant to how the module is currently working?

Rules is mentioned and that is a Drupal 7 thing for example... Just not sure there is anything actionable here.

Tests are failing, but I'll fix that separately.

How would you describe this use case?

Let's say a customer didn't receive the email as expected (was sent to spam or the email service was temporarily offline, having a way to resend the invoice allows to address that.
Similarly to the "Resend receipt" operation we have for orders.

@afagioli: Why did you reopen a 7.x feature request? 7.x is dead.

Went ahead and merged this.

Ok, here's a first attempt at this... This would deserve new tests, but don't really have the time to write them atm...

My concern from #3 is still valid so I wonder how we could mitigate that... I don't think looping over invoices is an acceptable solution for sites with a large number of invoices... And I don't think this is desirable either.

Maybe we add a descriptive text explaining the potential issues when updating the directory?

Also note that this report is probably a bit confusing as we are actually checking if the invoice file exists before generating an invoice already.

The suggested solution implies the invoice is generated from the admin which is most likely not the case for 90% of the merchants.
Not really sure how to properly address that.

The fact that we make sure to not regenerate an invoice file everytime is for compliance / immutability reasons... To make sure the invoice content isn't different if regenerated / resent a second time.

With that said, using the invoice UUID instead of the invoice number in the filename could help mitigating that.
I just wonder if this is an acceptable solution and would not be considered a breaking change.

Closing this as a duplicate of ✨ Provide a resend confirmation action link for invoices Active which implemented a different approach. Feel free to reopen this if you believe there is still a gap and the need is not properly addressed.

The scope expanded... I'm hoping I'm not making a mistake by overridding the label() method but I think this change makes sense and is kind of similar to what was done for orders (although we don't use the type there).

Could you please explain how to completely delete the invoice entity?

Deleting the invoices from the file system should do the trick, have you tried that?

Note that I'm altering the label() method to have consistent labels accross the UI.

Attached a screenshot of what the action link looks like. Note that I added a "Download" action link as well.

jsacksick → created an issue.

This is done, thanks!

hm... Why not though I think we should have a "Resend" operation from the invoices list (similar to "Pay" and "Download") for consistency.
So I think the patch should be updated to add this for invoices instead of the order... Though having it for orders could also be valid, but I'm just voting for consistency here...

This introduces a breaking change, but I don't think we have a choice here (besides introducing a new method with a different name doing the same thing)...

jsacksick → created an issue.

Ok I made the change anyway, opened an MR, let's see if the tests are passing.

Confirmed the fix locally myself, merged!