๐Ÿ‡ฎ๐Ÿ‡ฑIsrael @jsacksick

Account created on 8 October 2010, over 13 years ago
  • Senior Drupal Developer at Centarroย 
#

Merge Requests

Recent comments

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

See TaxTypeBase::resolveCustomerProfile() if you need different logic.. But in case you're shipping goods, VAT is determined based on where the goods are delivered.

For example, let's say I'm ordering from a website in France, to be delivered outside the EU, I shouldn't be charged VAT. In the event where I'd deliver the goods within the EU, I should be charged VAT.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Commerce usually uses "duplicate" and "Duplicate" instead of "clone". See ProductVariation, stores etc... Let's keep it consistent.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I think it is form_commerce_order_item_add_to_cart_form_commerce_product_VARIATION_2_alter no? ("commerce_product_variation" and not "commerce_product").

Judging by this code:

if ($purchased_entity = $order_item->getPurchasedEntity()) {
        $this->formId .= '_' . $purchased_entity->getEntityTypeId() . '_' . $purchased_entity->id();
      }

In any case, I think the way to opt out for this should probably be via a setting (e.g: Settings::get('commerce_cart_extended_form_id', TRUE)).
This isn't really a setting that belong to a UI IMO as this setting would be primarily used by developers... Ofc the alternative approach is to just swap the add to cart form class and override the gerFormId() class.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Well commerce_api should definitely not depend on commerce_checkout. The "checkout" module should be used on a fullstack site, not on a Headless site. Patch isn't acceptable as is.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

The reason why the amount is "reset" from the early order processor is that it represents the unaltered shipping rate amount, prior to invoking event subscribers and prior to applying promotions.

In case we don't reset it, then the subscribers are going to alter an already altered amount. So in case there is a price markup applied from a subscriber, we'll keep applying the markup on each refresh indefinitely?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

So the patch from #23 has changes from the other issue I linked in comment #18.
I don't think introducing another persistent field with a confusing name is going to help. If all we need to do is store the amount before alterations (which was originally the intent of "original_amount", then we can/should use the shipment data for that.
We would set the value from the early order processor and unset it from the late order processor or something.
This would simplify the code and wouldn't require adding additional getters/setters etc.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I don't really get why there is a need for a new field... Thought that is why original amount was added already?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

The patch is incorrect. It cannot be done this way... Since the commerce_order module doesn't depend on commerce_cart which is the reason why the filter is added like it is today.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Tax is always applied post promotions... Which is also the commerce core tax behavior for order items?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

hm... You might miss cache context / tags then perhaps...

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Reviewed, what was the problem with page attachments again?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Introducing a new service is not bad idea in general, the problem is that with the current methods defined, there's no room for additional arguments in the future.
Let's say you'd like to store the quantity that was added, or even the order ID, then your methods are suddenly obsolete.
Maybe you can go with:

CartConfirmationManager as the service name?
Then define the following methods:

  • recordAddToCart(), from here you can accept an array (quantity, order_item_id)...
  • getCartItemInfo()
  • clear()
๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

hm... Helper is a weird name, maybe too generic... And why would you need to have a method to delete the order item ID?
Also, surprised it isn't the case already, but we night need to store the quantity that was added as well in case somebody decides to not use Views for the cart confirmation.
Also the MR has code commented out.
We can maybe keep the code simple, just store directly into the private temp store...Once you introduce an interface then you have to maintain it and you can no longer break BC so you have to think carefully :p.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Why would a redirection occuring at this stage? You're throwing a redirect exception from onReturn()?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Private storage is a good idea, but I wouldn't save the whole HTML there, I'd simply save the data that I need to render the message, so just the order item ID?
Then whenever the private storage is accessed, you can just load the order item, and render the theme function?
But yeah, using the messenger is kind of hackish (that is how it was done in D7 as well as far as I remember).

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

So the first patch "Provide Views argument for current user's profile ID" can be removed?

yes.

Or will patch #25 work?

It should... Try?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

The first patch got committed and is present in the last release, the other patch seems to have the line your code is complaining about... But looks like it's importing the right namespace into the code...
In any case, this bug isn't reproducible on a clean install, so closing this.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

@drupalfan2: Do you have any patches applied? the unpatched code doesn't have this line...

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Yes, that is because invoices are immutable. Once they're generated, the invoice file is stored on the disk and a reference to the invoice file is stored on the invoice.

If you want to regenerate old invoices, you're going to have to delete the generate invoices from the disk and eventually clear the invoice file reference.

Marking this as "works as designed" as we implemented this behavior on purpose.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Comment #3 was ignored...

You can't put a required parameter after an optional one.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Go to "admin/config/people/profile-types".

TypeError: Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout::createPaymentMethod(): Argument #2 ($payment_details) must be of type array, /chroot/home/aab0448d/86038db258.nxcli.io/html/modules/contrib/commerce/modules/payment/src/PluginForm/PaymentMethodAddForm.php on line 83 in Drupal\commerce_paypal\Plugin\Commerce\PaymentGateway\Checkout->createPaymentMethod() (line 842 of /chroot/home/aab0448d/86038db258.nxcli.io/html/modules/contrib/commerce_paypal/src/Plugin/Commerce/PaymentGateway/Checkout.php).

PayPal checkout doesn't support saving/tokenizing payment methods (card on file), do you only use PayPal on your site?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

This is definitely not a "major" bug unless you're able to provide reproducible steps on a clean install.
Do you allow multiple "customer" profiles? Check the profile type settings.
The add payment method form probably shows you the billing profile form for the billing profile to attach to the payment method.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

The invoice type to use is determined at the order type level, if you need / want different logic, you'd need to write your own code and invoke your own logic for that.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Committed the change, thanks! Will tag a new release with the fix.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I guess this needs to be governed by a setting.. Perhaps not all users want that?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Not really sure... The profile copying logic lives in commerce_shipping, you might want to debug it... See the ProfileFieldCopy service under commerce_shipping.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

We could set the UID of one of the two profiles for the newly created user's id.

Not sure what you're referring to? The profile is copied to the Addressbook if the "Copy to addressbook" checkbox is checked during checkout.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Not sure which version you're using as this was fixed as part of ๐Ÿ› PHP 8.3 Deprecated function: Calling get_class() without arguments is deprecated Fixed .

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

That means that you have duplicates in the CSV you're importing (i.e. different prices for the same quantity & product).

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

DId you unregister the commerce subscriber doing the same? Or did you simply not define a number pattern at the order type level?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

But what issue did you run into? How can we help if we don't even know what this issue is for?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

The initial report had "commerce_number_pattern" in the backtrace and due to the use of the "lock" API which is no longer used.
Since then, unrelated other reports were added to this issue, but without reproducible steps or any explanation to why this issue is linked to Commerce?
Comment #5 is about the "cache_entity" bin, but that would be a core issue I'd say... Also, in general using the db cache backend isn't really the best when it comes to performance (Redis or Memcache would be preferable)...

@joseph.olstad and @vensires: Which issues are you experiencing? What makes you think this is caused by Commerce?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I didn't test that change I must admit, I was pinged by somebody from my team who reviewed the contextual filter and was surprised to see the setting not required as the contextual filter cannot work without the #required attribute.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

hm... Extremely weird that the setting applies to all contextual filters... So you're saying just the "#required" attribute is causing this? WIthout the #required flag everything works ok?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Are you using the contextual filter introduced in the latest release? From issue โœจ Provide Views argument for current user's profile ID Fixed ?

Downgrading the priority to "major" as it doesn't render your site unusable afaik: https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... โ†’

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Merged the MR.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

jsacksick โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

jsacksick โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

You need to use the "Payment process" pane.
The Checkout payment process pane should only be used in the paypal_checkout checkout flow.
Also, please look for previous reports as this has been previously reported a lot.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Committed, thanks!

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

It can happen quite easily... For example, let's say you've started a cart as an anonymous then login... The cart is going to be assigned to the user that just logged in.
If you repeat the same operatio a second time, you then end up with 2 carts.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Change looks ok, perhaps we should return NULL if $request is NULL as it can be NULL. See the CurrentLocale service.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

@Anybody: I don't really see the problem of keeping this functionality in a contrib module... Basic calculation shows that Commerce combine carts is used by ~4% of the Commerce installs (I just divided the reported usage of both modules).
Could probably be a 3.x target? I personally don't think this is a critical feature to have in core.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

V3 is going to be D11 compatible. I pushed a commit to allow installation with D11, not sure what fails yet.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

@FMB: Perhaps we need to add this to the Profile module directly?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I committed your patch. If you're in the mood for writing a change record, that'd be great... Added the following note to the release notes for now:

As part of #3315270, the storage of the profiles in the form state changed from an array of profiles indexed by profile type to an associative array (indexed by profile type) of arrays (indexed by delta) of profiles.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Committed.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

jsacksick โ†’ created an issue.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

hm... Not sure what does it have to do with Profile? There's no submodules?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

"Needs review" is for issues with patches.
Where are you referring to exactly?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Thanks for the report, fixed.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Committed, thanks!

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Seems like this is a Drupal Core bug, or should it be investigated from the side of Drupal commerce as well?

Hard to tell... Would need to try reproducing this myself but since it works without this module enabled... Well could be a core bug.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Support requests aren't answered from this issue 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)

If you have a bug to report, you're welcome to do that, or reclassify this as a bug with reproducible steps.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I can see the "inline_form_errors" in the backtrace... Did you try disabling that temporarily? I've never experiened this before but I'm also not using this module...

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I think the extra setting isn't required and make the UI more confusing...
We should probably simply override getComparisonOperators() to add extra operators.
Perhaps we should add "between" or 2 extra operators?

"< >" 
"<= >=" 

Not sure about the labeks. Perhaps "Between (exclusive)" and "Between (inclusive)"?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Merged, thanks!

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

jsacksick โ†’ made their first commit to this issueโ€™s fork.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Considering the change you're referring to is 4 years old, I'd suggest not changing the method return type at this point..
I can't really think of a good name for an additional method returning the translatable markup object though...

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

I also wonder if we can find a middleground between no tests and 2MB of tests data :).
@ sickness29: Is that possible?

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Also...

I see this comment:

Select the profile type to use. If a profile type is configured to support multiple profiles, it won't show up here as an option.

However, I don't actually see any code filtering out profile type allowing multiple profiles??

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Looks good at first glance, haven't tested it manually.

One thing though, can we simply implement the create method and skip overridding the constructor? This way we don't have to worry about potential breaking changes to the parent constructor.

So basically something like:


public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
$instance = parent::create($container, $configuration, $plugin_id, $plugin_definition);
$instance->entityTypeManager = $container->get('entity_type.manager');
$instance->currentUser = $container->get('current_user');
return $instance;
}

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Hi,

Products, product variations and stores are distinct entities.
Deleting orders, order items, profiles etc shouldn't have any impact / affect the product information.

Depending on the volume of data you have to delete, you could simply use VBO. However if you have a large amount of data to delete, I'd recommend performing the deletion via a cron job running periodically.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Well the patch has failing tests... And doesn't respect coding standards, so it is not RTBC.
Additionally, anybody applying the patch with existing shipment weight conditions is going to experience issues with it (configuration is going to be lost).

This change is a breaking change, it needs to be backwards compatible to support existing shipping methods using this condition...

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Do you need a schema with this constraint at all?

Well, to me having the possibility to set prices for the same quantity, for the same product variation within the same pricelist is a bug, and shouldn't be supported.

On a project where I have custom logic for tracking price history/price changes, the logic failed as there was a different price returned/detected everytime.

But maybe it's sufficient to display a message like "We have duplicates detected, please check" and let the user deal with it? Just a thought.

This is precisely what we're doing?

Update to the commerce_pricelist_item schema was skipped due to duplicate prices.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Or maybe we should just apply the schema change, and have it fail so the update can be re-ran.

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Instructions were added to the release notes on how to disable "partner ads", See https://www.drupal.org/project/commerce/releases/8.x-2.38 โ†’ .

๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

We already include the drupal form library.

CheckoutFlowBase::buildForm() has the following code:
$form['#attached']['library'][] = 'commerce_checkout/form';

And the checkout form library has core/drupal.form as a dependency.

form:
  version: VERSION
  css:
    theme:
      css/commerce_checkout.form.css: {}
  dependencies:
    - core/drupal.form
๐Ÿ‡ฎ๐Ÿ‡ฑIsrael jsacksick

Well the change isn't correct then... What we can do here is just remove the constructor and implement the create() method, calling the parent and then setting the entity type bundle info.

Production build https://api.contrib.social 0.62.1