New PaymentMethodUnavailable method created when clearing cache using Features and hook_default_payment_method().

The problem is that the PaymentMethodEntityController expects that payment_method entities are always loaded by ID, but entities can be loaded by name as well. See entity_load_multiple_by_name() in entity.module from the Entity API module.
I think the payment methods are loaded by name when they are loaded from the feature, see EntityDefaultFeaturesController::export_options() in entity.features.inc from the Entity API module.

The attached patch adds a check to PaymentMethodEntityController: it checks if the requested payment methods are one of the loaded payment methods' names before instantiating a new PaymentMethodUnavailable.

On a simple test environment with only one payment method in a feature, the patch in #3 seemed to work fine. But in a more complex environment with multiple payment methods in a feature, the PaymentMethodControllerUnavailable still shows up. I also get duplicate payment methods after first uninstalling Payment and then installing the feature. It seems as though Payment doesn't force the payment methods' machine names to be unique, but I yet have to verify this on a clean install.

I will investigate the issue further and I'm working on automated tests.

I figured out that the fact payment methods can be loaded by name is not the only problem. When building default payment methods in _entity_defaults_rebuild() from Entity API module, Entity API checks if such payment method already exists by trying to load it. Payment module receives a request to load a payment method that does not exists and returns a PaymentMethodUnavailable, which in turn is saved by Entity API based on the state of the payment method, which is ENTITY_CUSTOM (see property $status from the class "PaymentMethod").

See this code from _entity_defaults_rebuild() entity.module:

// Load all existing entities.
$existing_entities = entity_load_multiple_by_name($entity_type, array_keys($entities));

foreach ($existing_entities as $name => $entity) {
  if (entity_has_status($entity_type, $entity, ENTITY_CUSTOM)) {
    // If the entity already exists but is not yet marked as overridden, we
    // have to update the status.
    if (!entity_has_status($entity_type, $entity, ENTITY_OVERRIDDEN)) {
      $entity->{$keys['status']} |= ENTITY_OVERRIDDEN;
      $entity->{$keys['module']} = $entities[$name]->{$keys['module']};
      $entity->is_rebuild = TRUE;
      entity_save($entity_type, $entity);
      unset($entity->is_rebuild);
    }

    // The entity is overridden, so we do not need to save the default.
    unset($entities[$name]);
  }
}

Entity API saves entities that have the status ENTITY_CUSTOM and not have the status ENTITY_OVERRIDDEN. This is currently true for PaymentMethodUnavailable payment methods.

Attached are two patches, one with tests and one without tests.

Tests

• Three test methods are added to PaymentTestEntityCrudWebTestCase to test loading payment methods by name and test import/export functionality.
• A test called PaymentTestDefaultPaymentMethodWebTestCase is added to test hook_default_payment_method().

Fix

• For the name loading problem, the same fix as in #3 is provided.
• For hook_default_payment_method() an incomplete/experimental fix is added. The status property for PaymentMethodUnavailable is set to ENTITY_FIXED, so Entity API skips it in _entity_defaults_rebuild() (instead of saving it). The test PaymentTestDefaultPaymentMethodWebTestCase will still have one "exception", though.

Some of the tests provided by #5 are not executed. See #2229787: PaymentTestEntityCrudWebTestCase is not executed by testbot.

5: payment.payment-unavailable-load-by-name-2174395-5.patch queued for re-testing.

5: payment.payment-unavailable-load-by-name-2174395-5-tests-only.patch queued for re-testing.

I've rerolled the patches against the current 7.x-1.x.

Spooky. Somehow the very-same patches created via git format-patch work while they don't when they are created via git diff.

@torotil
Thanks for the reroll. There is still one exception during the new added "Default payment hook" test that I didn't knew how to fix.

I also wasn't entire sure if the following is right:

+++ b/payment/payment.classes.inc
@@ -612,6 +612,7 @@ class PaymentMethodUnavailable extends PaymentMethod {
+    $this->status = ENTITY_FIXED;

This fix is experimental, as noted in #5:

For hook_default_payment_method() an incomplete/experimental fix is added. The status property for PaymentMethodUnavailable is set to ENTITY_FIXED, so Entity API skips it in _entity_defaults_rebuild() (instead of saving it). The test PaymentTestDefaultPaymentMethodWebTestCase will still have one "exception", though.

Side note: the patch in #13 did not apply because it tried to patch a non-existing file:

--- a/payment/tests/payment_test/tests/PaymentTestDefaultPaymentMethodWebTestCase.test
+++ b/payment/tests/payment_test/tests/PaymentTestDefaultPaymentMethodWebTestCase.test

This file was added by the patch in #5.

The status property for PaymentMethodUnavailable is set to ENTITY_FIXED, so Entity API skips it in _entity_defaults_rebuild() (instead of saving it).

I don't think that this is entirely wrong. ENTITY_FIXED is exactly what we should use for on-the-fly generated entities like PaymentMethodUnavailable (that should not be saved). Perhaps a better place to set it is in PaymentMethodEntityController::load() exactly when it is returned instead of a real PaymentMethod. Although this is a matter of taste as long as PaymentMethodUnavailable isn't used anywhere else.

I'm again attaching two patches - this time the tests should pass.

Setting to needs review again since only the test-only patch failed.

Any traction on this issue? We are hit by the bug that increments regularly the PMID. So the Basic Payment Method was most of the time generating invalid rules (name based on PMID) in combination with Features.

I analysed this problem, and I found that problem in function "load()" of class PaymentMethodEntityController. Problem occured because in code, compares ID's of loaded entities and ID's, which was sent into the "load()" function. In some cases to the "load()" function can be sent name of entity instead of ID (pmid). And when pmid compares with name of entity, I got this wrong unavailable payment method.

I wrote simple patch, which fixes this problem on my project.

Can someone please update the issue summary with a description of the cause of this problem?

@eugene.ilyin: please don't just dump patches in issues where others are already working on a patch. It is very counter-productive. At the very least you should describe how and why your patch differs from the others.

I've updated the issue summary. Hopefully, the cause of the problem is more clear now.

If you look at the documentation of DrupalEntityControllerInterface::load(), it says that the parameter contians entity IDs and not machine names. This sounds like we need to fix the calling code, instead of the entity controller.

Xano: There is still the issue with ENTITY_FIXED.

PaymentMethod as an exportable entity has a status attribute. The on-the-fly generated PaymentMethodUnavailable can not be stored in the database whatsoever. So it should indeed have a status of ENTITY_FIXED:

/**
 * A bit flag used to mark entities as fixed, thus not changeable for any
 * user.
 */
define('ENTITY_FIXED', 0x04 | 0x02);

That's what fixed the issues for me. Honestly I do not know what #26 is all about.

Maybe setting the status ENTITY_FIXED only hides the symptoms, though.

This is the calling code from entity.module:

/**
 * A wrapper around entity_load() to return entities keyed by name key if existing.
 *
 * @param $entity_type
 *   The entity type to load, e.g. node or user.
 * @param $names
 *   An array of entity names or ids, or FALSE to load all entities.
 * @param $conditions
 *   (deprecated) An associative array of conditions on the base table, where
 *   the keys are the database fields and the values are the values those
 *   fields must have. Instead, it is preferable to use EntityFieldQuery to
 *   retrieve a list of entity IDs loadable by this function.
 *
 * @return
 *   An array of entity objects indexed by their names (or ids if the entity
 *   type has no name key).
 *
 * @see entity_load()
 */
function entity_load_multiple_by_name($entity_type, $names = FALSE, $conditions = array()) {
  $entities = entity_load($entity_type, $names, $conditions);
  $info = entity_get_info($entity_type);
  if (!isset($info['entity keys']['name'])) {
    return $entities;
  }
  return entity_key_array_by_property($entities, $info['entity keys']['name']);
}

This is the entity definition of entity type "payment_method". The entity type has a name key and is exportable.

$entity_info['payment_method'] = array(
  'label' => t('Payment method'),
  'controller class' => 'PaymentMethodEntityController',
  'features controller class' => 'PaymentMethodFeaturesController',
  'entity class' => 'PaymentMethod',
  'module' => 'payment',
  'base table' => 'payment_method',
  'entity keys' => array(
    'id' => 'pmid',
    'label' => 'title_specific',
    'name' => 'name',
  ),
  'exportable' => TRUE,
  'access callback' => 'payment_method_access',
);

From entity.api.php:

- EntityAPIControllerExportable: Extends the regular controller to
additionally support exportable entities and/or entities making use of a
name key.

From class EntityAPIControllerExportable in entity.controller.inc:

/**
 * Overridden to support passing numeric ids as well as names as $ids.
 */
public function load($ids = array(), $conditions = array()) {
  (...)
}

I think because Payment supports the exportable feature of the Entity API, it should also support loading payment methods by name.

Alright, thanks for looking into this! I'll have to dive into core's entity API and the Entity module's API a bit more to confirm this is the desired behavior, but it looks like we need to cater for this in Payment indeed.

See #2296483: entity_load_multiple_by_name() uses entity_load() with names instead of IDs.

The patch should suppress the error, but I don't think it will ever properly work, as DrupalDefaultEntityController::buildQuery() explicitly uses the passed on IDs in a condition on the entity type's ID field. If names are passed on, this will never result in any entities being loaded.

@Xano
I just tried the following code and I got one payment method entity back (where "example" is the machine name of one payment method):

$entities = entity_load('payment_method', array('example'));

This works because EntityAPIControllerExportable overrides the buildQuery() method (and PaymentMethodEntityController extends EntityAPIControllerExportable):

/**
 * Support loading by name key.
 */
protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
  // Add the id condition ourself, as we might have a separate name key.
  $query = parent::buildQuery(array(), $conditions, $revision_id);
  if ($ids) {
    // Support loading by numeric ids as well as by machine names.
    $key = is_numeric(reset($ids)) ? $this->idKey : $this->nameKey;
    $query->condition("base.$key", $ids, 'IN');
  }
  return $query;
}

Sorry for causing the confusion. I looked at the wrong controller class. The patch looks good and I just committed it.

Thank you for helping out!

I realize the fix I committed was for the loading issue only. If the default payment method/constant problem still exists, can we tackle that in a follow-up issue?

@Xano
I see you committed code based on the patch from #26, but the patch from #21 also included tests for loading payment methods by name. Should these tests be handled in a new issue as well?

Let's commit the tests for the loading issue here. Thanks for noticing!

This contains only the test bit.

Thanks again!