Argument 1 passed to rules_config_delete() must be of the type array, object given

[herbdool]

I get this error:

TypeError: Argument 1 passed to rules_config_delete() must be of the type array, object given, called in /app/core/includes/module.inc on line 975 in rules_config_delete() (line 937 of /app/modules/contrib/rules/rules.module

I was importing some configuration. But I'm not quite sure why since rules are not yet in config.

[herbdool]

This was already brought up by me apparently #47 and the function was removed. Can we get a new release of this module @laryn?

[laryn]

@herbdool Yes, I've just been added as a co-maintainer and working towards a new release soon after some clean up.

[bugfolder]

There's a name collision going on here.

Configuration Manager defines hook_config_delete(). So when a configuration is deleted, module_invoke_all() will call all possible functions of the form mymodule_config_delete(), which includes rules_config_delete().

But in the Rules module, we have this:

/**
 * Delete rule configurations from database.
 *
 * @param array $ids
 *   An array of entity IDs.
 */
function rules_config_delete(array $ids) {
  return entity_get_controller('rules_config')->delete($ids);
}

So rules_config_delete() isn't an implementation of hook_config_delete(). Badness ensues.

So, I think we need to rename at least that function (maybe others for consistency?).

[bugfolder]

Ugh. rules_config_delete() is called by several Ubercart sub-modules (uc_flatrate, uc_weightquote, uc_taxes). So renaming the function will break those calls.

(Good thing Laryn is a co-maintainer of both Rules and Ubercart.)

[bugfolder]

Well...I see no alternative but to rename the function rules_config_delete(). If it exists by that name, it will be invoked (erroneously) as an instance of hook_config_delete().

Perhaps rules_configuration_delete()? I see that rules has its own hook, hook_default_rules_configuration(), so there is precedent for spelling out "configuration".

And then that means that those calls in Ubercart will need to be updated to use the new name. And so when the new version of Rules is released, there will need to be a Rules-version-dependent release of Ubercart to keep them in sync.

[herbdool]

This function was already removed from Rules. I don't know if @laryn has made a new release yet. With the move to CMI I don't know if it's necessary in Ubercart either.

[bugfolder]

@herbdool, this function has nothing to do with CMI. The 'rules_config' entity, like all entities, is stored in the database. A 'rules_config' entity is, basically, a "rule". The problem is that this particular function happens to have a name that matches a CMI hook (which, of course, wasn't an issue in D7).

[bugfolder]

Also, the functionality does seem to be needed by Ubercart. Here, for example, is a function from the uc_taxes module:

/**
 * Deletes a tax rate from the database.
 *
 * @param $rate_id
 *   The ID of the tax rate to delete.
 */
function uc_taxes_rate_delete($rate_id) {
  // Delete the tax rate record.
  db_delete('uc_taxes')
    ->condition('id', $rate_id)
    ->execute();

  db_delete('uc_taxed_product_types')
    ->condition('tax_id', $rate_id)
    ->execute();

  db_delete('uc_taxed_line_items')
    ->condition('tax_id', $rate_id)
    ->execute();

  // Delete the associated conditions if they have been saved to the database.
  rules_config_delete(array('uc_taxes_' . $rate_id));
}

Note that this is deleting any tax $rate_id-related rules from the database. It's not doing anything with CMI.

[bugfolder]

And reading through the discussion of #47, I think the function should not have been removed, just renamed (with corresponding changes in Ubercart).

Looks like it was removed in PR #80 on 3/17/21, but, like the cat, it came back. It's currently in the 1.x-2.x branch and git blame says it's been there for the past 12 years.

[herbdool]

Yeah I get that it had a different purpose - I know it didn't do anything with CMI.

Perhaps it's time to convert the configuration storage for Rules from the db to CMI, though. There's an issue on the backlog for that.

[bugfolder]

Perhaps it's time to convert the configuration storage for Rules from the db to CMI, though.

Yeah, I see that's #69. I see the discussion of the issue in #25 (although that dealt only with import/export).

I'm far from an expert in this, but it strikes me that converting the Rules module to store all individual rules in CMI and provide the hooks and CRUD interface to other modules that work with Rules (like Ubercart) would be a pretty big task, much larger than just the import/export capability. But I'd like to get @laryn's thoughts, since Rules is currently his baby.

And, getting back to this issue: since rules_config_delete() is currently in the 1.x-2.x branch, and it will definitely break any config sync where old config files will be removed, we do need action, however limited, on this issue. (Another collision-free renaming candidate: rules_delete_rules_config(). Thought about using rules_rules_config_delete(), but that would be interpreted as an instance of hook_rules_config_delete().)

(Incidentally, a workaround for anyone who encounters this problem while trying to do a config import is to manually delete any config files listed as "to be removed" from the active config directory prior to running config sync. Not a great solution, since any side effects of config removal won't happen, but it can at least get you past the error.)

[laryn]

I may have brought this cat back to life when I merged from upstream to try to get things all up to date. I haven't thought through all the implications of moving to CMI -- my immediate goal has been to try to get this functional and up to date as is. I'd love for the tests to all be passing before we make a new release. @bugfolder you may honestly have your head around things much better than me at the moment. No pressure, but do you want to team up on Rules as well (as we've done for Ubercart). I'd be happy to add you as a co-maintainer.

[herbdool]

I've worked on converting Feed's configuration from the db to CMI so I think it's fairly doable here too. The meat of the configuration functionality is here: https://github.com/backdrop-contrib/rules/blob/1.x-2.x/includes/rules.core.inc. I think it's fairly well extracted so that's good news. Instead of having to query relations between different tables, each rule could be stored as a nested array in its own config file.

[laryn]

@herbdool The offer to comaintain stands for you, too, if interested. ;)

[herbdool]

I'm holding off from co-maintaining but if I've got time I can see about converting to CMI.

[bugfolder]

@laryn, I'd be happy to join as co-maintainer. Rules feels like it's practically a part of Ubercart (of course it's more; it's got 277 installs, vs UC's 61).

But the same warning as when I joined UC: the things I file PRs on, I'm pretty comfortable with, but there's vast swaths that I'm not comfortable with (at least not yet), and the whole entity system is one of those things! (Someday...)

That said, I'm quite positive on the notion of converting Rules wholly to CMI. It feels a lot like Views in that regard. As the issue discussion noted, it would be way nicer to distribute rule sets as collections of JSON files versus implementations of hook_default_rules_configuration() (which UC contains a slew of, incidentally). So I would welcome @herbdool's offer to work on this.

I would also welcome getting the automated tests to work, but that's way out of my expertise right now. I do observe that the same functional tests are failing for Rules and UC related to file_exists(), etc., which suggests that the problem lies on in these modules but in the testing configuration.

[bugfolder]

Circling back to the original purpose of this issue, it looks like renaming rules_config_delete() to rules_configuration_delete() is the simplest and cleanest issue to fix the underlying problem of the accidental hook implementation. And then this will require corresponding changes in Ubercart, so I'll create and link an issue there.

[argiepiano]

I bumped into this trying to help fix Ubercart tests. Ubercart is very tightly integrated with Rules, so this kind of issues mess up many things there.

I agree that a first necessary step is to rename rules_config_delete() to rules_configuration_delete() to avoid the collision.

Switching the storage of rules_config entities to CMI is a very large task, and not as urgent as the above.

Incidentally, several ported contrib modules present the same problem. For example webform stores its configuration entities in database (see backdrop-contrib/webform#192). Also even Entity Plus, which has the ability to create configuration entities (those who define bundles of custom entities), still stores those in the database. So, a general overhaul of those is needed at some point, but since the db storage causes no harm, I don't see a rush to do that.

[bugfolder]

Here's the corresponding Ubercart issue: backdrop-contrib/ubercart#305.

[argiepiano]

@bugfolder, There are a few things to keep in mind:

Most importantly: rules_config is an entity type defined with hook_entity_info in rules.module. That means that when you delete, save or insert a rules_config, the hooks being called are hook_rules_config_delete(), hook_rules_config_insert() and hook_rules_config_presave() etc . There is no way around this, as EntityPlusControllerExportable automates those hook calls based on the entity type (i.e. rules_config). See this line.

So, that means that the hooks being called are STILL named hook_rules_config_delete() etc instead of hook_rules_configuration_delete(). This may be potentially confusing. There are a couple of implementations of that hook in the Rules project.

The only way around this is to rename the entity type to rules_configuration, but this may have unintended consequences (for one, the table in the database is rules_config, and we would need to create an update hook in install to rename that).