🇺🇸United States @AaronBauman

aaronbauman → created an issue.

Opened MR448 converting patch #32 into MR, and moved the availability logic into an AvailabilityChecker

Seems like this should all be going into an AvailabilityChecker instead of jammed into the process method:

      $purchased_entity = $order_item->getPurchasedEntity();
      if (!$purchased_entity) {
        if ($order_item->hasPurchasedEntity()) {
          $order_items_to_remove[$order_item->id()] = $order_item;
        }
        continue;
      }
      // Check if the product or variation is unpublished.
      $product = $purchased_entity->getProduct();
      if (empty($product) || !$product->isPublished() || !$purchased_entity->isPublished()) {
        $order_items_to_remove[$order_item->id()] = $order_item;
        continue;
      }

MR44 is passing, back to NR

Hid the other 2 MRs

aaronbauman → changed the visibility of the branch 3107993-3x-template-not-found to hidden.

aaronbauman → changed the visibility of the branch 3107993-template-is-not to hidden.

+1 RTBC pls merge

MR27 gtg please merge.

Please commit

So it is, thanks!

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

Patch #2 works great for me, thanks

This doesn't have anything to do with Salesforce module.

Please create an issue against core, or whatever contrib module is responsible for this.

This is committed to 2.0.4

This patch uses the "amount_to_collect" received from Taxjar and creates a single, Adjustment on the Commerce Order for the tax amount.

Please review.

Fixed, thanks for the patch

The module does send the shipping cost to Taxjar, but I honestly don't know how that gets handled on the Taxjar end.

Do you know of a jurisdiction in which shipping is taxable, so that we could test it?

Fixed, thank you for the patch

Yes, you can implement hook_commerce_taxjar_tax_request_alter and hook_commerce_taxjar_transaction_request_alter to set $request_body to an empty array based on your business logic. This will avoid sending the request to TaxJar altogether.

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Closing all 7.x issues. Please reopen if this issue still pertains to D10 version

Good idea, thanks for the patch

Fixed, thanks for the patch

Good idea, thanks for the patch

Thank you for the patch, this is now moot

Fixed in D10 version, thank you

Fixed, thank you

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

This was due to a patch I was running locally. Working as expected without that patch.

Emailed andyg5000 requesting commit and release access.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

Yeah, i'm running into all sorts of problems.

I had to switch back to the basic credit card pane, and even that is be a bit buggy.

aaronbauman → created an issue.

aaronbauman → created an issue.

https://git.drupalcode.org/project/salesforce/-/merge_requests/96

aaronbauman → created an issue.

Re: detecting config overrides, this is now a core feature: 📌 Implement new #config_target framework for config forms Active

Re: automatically invalidating token, I'm not convinced this is a good idea.
Any sort of issue may arise where we get a false positive, and invalidate the token for the wrong reason.
I'm think ✨ Provide a way to utilise multiple Salesforce auth providers Active gets us closer toward a more robust solution.

What do you think of putting the provider id(s) on the mapped object, instead of the mapping?

I'm thinking through a few scenarios here, let me know what you think:

1. Dev Sandbox vs Prod, refreshed and up to date - fields may be the same. Mapping could be applicable to both. No reason to have duplicate mappings, but we need separate Mapped Objects.
2. Full Sandbox vs Prod, refreshed and up to date - Again Mapping could be applicable to both. IDs for Mapped Objects are the same. Mapped Object could also be relevant in both orgs.
3. Dev Sandbox vs Prod, out of date - Mapping could still be applicable to both, but would be useful to be able to designate only one. Need separate Mapped Objects.
3. Full Sandbox vs Prod, out of date - Mapping could still be applicable to both, but would be useful to be able to designate only one. Could use same Mapped Objects.

aaronbauman → created an issue.

I'm talking about the situation where a subscriber might want to postpone pull.

The "disallowPull" method doesn't support this, outside of manually forcing another pull attempt.

There's no permanent disallow that i know of...

oh nice, https://www.drupal.org/project/advancedqueue → might do the trick

i'm all about maintaining less code whenever possible

One reason is to dedupe the queue entries.
This is happening already in Push Queue, but not Pull Queue.

Since the core database queue serializes all the data, there's no simple way to dedupe without loading every single queue item.
Not feasible.

For this same reason, I think Pull Queue should also be converted to a custom backend.

Another reason - not currently happening for push queue, but again not feasible with core queue - is so that queue items can be batched to minimize API usage.

Fixed, thanks

I'm not sure it makes sense to change behavior of this field plugin, which is already widely in use.

May be more appropriate to add a setting, or a new field plugin altogether.

Makes sense, merged.

FWIW, Salesforce suggests a minimum interval of 5 minutes.

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

addressed by 🐛 Call to a member function render() on string Active

Have you been using this patch in production?

The annoying thing with "disallow pull" is that it doesn't get automatically retried, ever.
So, even if the parent SFID does eventually get created, it won't trigger another pull attempt for the child record.

This is somewhat tangential, and shouldn't block this patch, but something to consider for folks that want to use this feature.

This was addressed by 📌 Drupal 11 Fixed

OK, committed. We'll see how it goes.

Committed, thanks

Was fixed in 📌 Drupal 11 Fixed

Great, thank you for the patch!

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

committed, thanks

merged

Merged, thanks

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

Yes, the whole queue system needs an overhaul.

Ideally, each mapping would define its own derivative, so that we can continue to use the per-mapping settings we have now.
We probably still need a custom database backend for the push queue, but I don't think we actually need the custom PushQueueProcessorInterface.
Probably would be useful to have a custom database backed for pull queue as well.

Anyway, thank you for the patch - this looks fine and good.
I'm hesitant to merge it because I don't really want to go down the rabbit hole of supporting a views integration in the short and medium term if we're looking towards this longer term change.

I don't think the behavior needs to change, only the comment.

Merged, thanks

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

Commited, thanks

committed, thanks

committed, thanks

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

Cross posting from 🐛 Support Facets 3 - currently does not work for taxonomy term reference exposed filter Active the form alter I came up with to workaround this issue.

 function MY_MODULE_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  if ($form_id == 'views_exposed_form' && !empty($form['#context']['bef'])) {
    // Facets 3.x + Select2 + BEF causes problems with validation.
    // Remove validation here for that scenario.
    foreach ($form['#info'] as $name => $element) {
      if (!str_starts_with($name, 'filter-facets')) {
        continue;
      }
      $key = $element['value'];
      $form[$key]['#process'][] = 'facets_exposed_filters_remove_validation';
    }
  }

The changes in 3446040 look pretty significant.

I was able to work around the bug with a form alter.
I'll stick with until the patch has some more reviews or tests.

 function MY_MODULE_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  if ($form_id == 'views_exposed_form' && !empty($form['#context']['bef'])) {
    // Facets 3.x + Select2 + BEF causes problems with validation.
    // Remove validation here for that scenario.
    foreach ($form['#info'] as $name => $element) {
      if (!str_starts_with($name, 'filter-facets')) {
        continue;
      }
      $key = $element['value'];
      $form[$key]['#process'][] = 'facets_exposed_filters_remove_validation';
    }
  }

The rector patch is insufficient, and it's not really worth moving all the work in this issue just because it's older.
Closed that one and reopening this one.

Superseded by 📌 Drupal 10 Module Version Active

aaronbauman → created an issue.

aaronbauman → created an issue.

aaronbauman → created an issue.

I'll have to keep an eye on how it performs, but i *think* this will solve the issue for me. (MR105)

Rather than the initial LDAP user query, it's the subsequent processing that was taking the most time.
The system queue processor has a timer to prevent overrunning cron, so this should be a nice and tidy solution.

OK, we don't need to use pagination, we can take advantage of Drupal queue.

Instead of iterating the entire LDAP query result set in \Drupal\ldap_user\Processor\GroupUserUpdateProcessor::runQuery, this method should create a queue entry for each item.

Then the queue process can do the iteration without blocking cron, and will easily pick up wherever it leaves off when the batch takes too long.

Should actually be relatively straightforward to untangle these.

aaronbauman → created an issue.

The patch provides detailed changes, and this thread contains a robust discussion about why those changes are necessary.

@ant1, Your comment does not add to the discussion, or provide any response to the proposal.

Resetting to Needs Review.

MR4 fixes the issue for me.

aaronbauman → changed the visibility of the branch 3502753-call-to-undefined to hidden.

Because it's not my code. It's a contrib

Well, yes, i guess that would do it.

I can't think of a reason why an entity would not be either config or content, but I guess they've found one.
You're right, there's no reason we should prevent this kind of entity from being mapped.

However, a word of caution: in terms of the motivation for isSyncing() - this check is designed to prevent push-pull feedback loops between Drupal and Salesforce. I'm not inclined to implement a workaround inside the module for this edge case, but something you'll want to watch out for.

This is fine for now, but imo we should enforce implementation of SynchronizableInterface in order for entities to be mappable.

I see you're mapping a custom object, why not just implement the interface?