I finally tracked down this bug we've been having in the Payment UI for a while. At admin/commerce/config/payment we display two RulesPluginUI::overviewTables with different conditions in each one. The first shows enabled payment method rule configurations and the second disabled configurations. Randomly the disabled ones stopped showing... and this led me through a whole series of debug messages until I realized it didn't have anything to do with the value (i.e. does active == FALSE) but rather with which one came first. If I put the disabled on top, they showed just fine.

Turns out it was actually a caching issue tucked into EntityAPIController::load and EntityAPIController::getDefaults. The $defaultsFiltered property was getting set to TRUE the first pass through, but it wasn't specifying for which $conditions array it was doing so. This meant that my subsequent calls weren't attempting to get default configurations.

I was able to work around this by calling EntityAPIController::resetCache between my tables, but it seems like the $defaultsFiltered property could be made an array that stored values specific to the $conditions array (based on some sort of hash or something... I got it to work in a quick test by serializing the conditions array). Otherwise we just need to have documentation that states you must reset the cache between any two function calls that load entities and expect the default module-defined rule configurations to be present.

CommentFileSizeAuthor
#8 rules_plugin.patch1.15 KBfago

Comments

fago’s picture

hm, it does:

      $this->cacheSet($not_overridden_defaults);
      $this->defaultsFiltered = TRUE

Thus subsequent default entities should be served from the static cache. So I don't see what's wrong here.

Maybe, you can figure out the steps needed to reproduce that? Perhaps you can do some with some basic rules_config_load_multiple() / entity_load calls?

rszrama’s picture

Hmm... not sure, fago. The steps to reproduce are basically install Drupal Commerce 7.x-1.0-alpha2 and go to the payment settings screen where we have the two overview tables. I can try this again and dump the cache between tries to see what exactly is and isn't being cached, but for some reason or another clearing the cache between building those two tables resolved the issue I was having of missing rule configurations. : ?

My hunch in the original post was basically that getDefaults() is only being called the first time and filtering to active configurations based on my $conditions. The second time I have a different set of $conditions, but I don't see any filtering being done for default configurations outside of getDefaults(). And, based on the snippet you posted and lines 195-199, that means the second set of $conditions will never be applied somehow.

I can't try this again today, but I'll keep it in the queue. At the very least, we're working now. : )

fago’s picture

If you have a look at cacheGet(), there the conditions are also applied. Also the code first gets all defaults, puts them into the cache and then it applies the conditions. So I don't see what's wrong here, but obviously something is.

fago’s picture

Status: Active » Postponed (maintainer needs more info)

I've overhauled the CRUD controller a bit during the last days, so I simplified the loading logic. Could you please try whether your problem persists with the simplified logic? (=recent code) Thanks!

rszrama’s picture

Hmm... so I seem to have a new problem. Default configurations aren't being recognized at all now, with no changes at all to my implementations of hook_default_rules_configuration(). Configurations that exist in the database are visible, but any default configurations that haven't been overridden cannot be seen in the UI. Trying to debug now... but it is late and I might need to crash. ; )

rszrama’s picture

Status: Postponed (maintainer needs more info) » Active

Ahh, you know what, this actually did work. I no longer have to clear the cache. However, as I mentioned above, there was another bug in that my default configurations weren't being detected at all. The problem was that the overview table checks for the condition that plugin => 'reaction rule'. But for default rule configurations, this plugin property is never set even though the rules_reaction_rule() function is used in hook_default_rules_configuration() as recommended in rules.api.php:

/**
 * Define default rules configurations.
 *
 * This hook is invoked when rules configurations are loaded. The implementation
 * should be placed into the file MODULENAME.rules_defaults.inc, which gets
 * automatically included when the hook is invoked.
 *
 * @return
 *   An array of rules configurations with the configuration names as keys.
 *
 * @see hook_default_rules_configuration_alter()
 */
function hook_default_rules_configuration() {
  $rule = rules_reaction_rule();
  $rule->label = 'example default rule';
  $rule->active = FALSE;
  $rule->event('node_update')
       ->condition(rules_condition('data_is', array('data:select' => 'node:status', 'value' => TRUE))->negate())
       ->condition('data_is', array('data:select' => 'node:type', 'value' => 'page'))
       ->action('drupal_message', array('message' => 'A node has been updated.'));

  $configs['rules_test_default_1'] = $rule;
  return $config;
}

To get my default configurations to show up, I had to manually add $rule->plugin = 'reaction rule'; after the rule is created. It seems to me that the plugin name should automatically be added by rules_plugin_factory() since it's passed as an argument to this function anyways. What do you think?

fago’s picture

Project: Entity API » Rules
Version: 7.x-1.x-dev » 7.x-2.x-dev
Component: Entity CRUD API - main » Rules Engine
StatusFileSize
new1.15 KB

arg, I recently removed the code initializing that value for defaults. I really need to add UI tests, thanks.

Anyway, $rule->plugin duplicates $rule->plugin(), so I prefer not having it at all. Attached patch implements the condition checking using the method, does that work for you?

fago’s picture

Status: Active » Needs review
rszrama’s picture

Status: Needs review » Reviewed & tested by the community

Patch works for me. Yay! : )

fago’s picture

Status: Reviewed & tested by the community » Fixed

Great, committed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.