Skip to content

Conversation

@vasike
Copy link
Contributor

@vasike vasike commented Mar 16, 2017

No description provided.

Copy link
Contributor

@mglaman mglaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some concerns around the post update function.

*/
function commerce_payment_post_update_1(&$sandbox) {
// Get the user ids that have 'commerce_remote_id' field not emtpy.
$result = \Drupal::entityQuery('user')->exists('commerce_remote_id')->execute();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't we only want to run this the first time, and I think we could just do a count query

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A count query would probably be more efficient, let's do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be done now


// Process entities by groups of 20.
$limit = 20;
$result = array_slice($result, $sandbox['current'], $limit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not put a range on the query instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it necessary?

if (\Drupal::entityQuery('user')->exists('commerce_remote_id')->count()->execute() == 0) {
return t('No user Remote ID field data to update.');
}
$result = \Drupal::entityQuery('user')->exists('commerce_remote_id')->execute();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should run the count query only under if (!isset($sandbox['current'])) {
After that it's cached in $sandbox.

Then run the real query with a range each time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be done

/** @var \Drupal\commerce_payment\PaymentGatewayStorageInterface $payment_gateway_storage */
$payment_gateway_storage = \Drupal::entityTypeManager()->getStorage('commerce_payment_gateway');
$payment_gateways = $payment_gateway_storage->loadMultiple();
uasort($payment_gateways, [$payment_gateway_storage->getEntityType()->getClass(), 'sort']);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to use $payment_gateway_storage->getEntityType()->getProvider() to get the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no idea, what you mean

return $this;
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a point in having these methods? We usually need the remote customer ID before creating a payment method, so we need them on PaymentGatewayBase instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i also thought the same initially. but working on it looks like PaymentMethod entity methods

@bojanz bojanz force-pushed the 8.x-2.x branch 7 times, most recently from 6be1d5c to b8a7444 Compare April 27, 2017 20:46
@bojanz
Copy link
Contributor

bojanz commented Jul 12, 2017

Committed in 14786dd.

@bojanz bojanz closed this Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants