Import form doesn't set Rule status to 'Overridden'

I'm trying to write an upgrade for an action plugin that I made and I'm having the same problem using the export function and then trying to use the import function... Something like:

$rules = rules_config_load_multiple(FALSE);
$rule = array_pop($rules);
$export = json_decode(str_replace('flag_fetch_entity_by_user','flag_fetch_node_by_user', $rule->export()), TRUE);
rules_import($export);

This is crude but I wanted to see if it could be done before I got too deep. I would expect the same behavior Angry Dan expected in that an imported rule with the same name as an existing rule would be overridden.

Suggestions?

I'm going to mark this as a critical error, as it can cause seemingly random, often severe changes in a sites behaviour.

This is a bug in the import form. It imports the new code, copies the rule id from the existing rule to the new rule but doesn't copy the status so that rules that were 'Default' before stay default. This means that when caches clear the imported configuration is completely lost!

The patch makes sure that the config status gets updated when the import form is submitted.

N.B. Comment #1 is a completely seperate issue I think.

Retesting ...

Still waiting for someone to review this patch and verify it works to fix the problem ...
I would also be nice to have some clear instructions for reproducing the problem, as right now the conditions under which this arises are not clear at all ...

In #4 I said it wasn't clear how to reproduce this. I don't know what I meant by that, as this seems pretty clear and reproducible to me now from just the information in the original post. It doesn't have anything to do with Commerce, so you can ignore that part of the original post.

To summarize the problem, if you have an existing rule provided by code (status will be 'Default'), and you import a rule with the same name using the import form and check the overwrite checkbox, the expectation is that this rule will now have the new imported values and have status 'Overridden', and you will be able to revert it back to the code-provided values using the revert link. What happens instead is that the existing rule DOES get temporarily changed in memory, but clearing the cache will improperly revert it back to the code-provided values. Additionally, even though this rule has been changed in memory, it shows a (wrong) status of 'Default' therefore you don't have a revert link to use ...

But the patch in #2 isn't the solution - it doesn't do the correct thing. (BTW this is one of the reasons why we should have tests for every feature).

The attached patch fixes this properly, but still needs tests.

TL;DR:
The status of the rules configuration is stored as a bitmask. There are four possible values, as defined in entity.module:

/**
 * A bit flag used to let us know if an entity has been customly defined.
 */
define('ENTITY_CUSTOM', 0x01);

/**
 * A bit flag used to let us know if an entity is a 'default' in code.
 */
define('ENTITY_IN_CODE', 0x02);

/**
 * A bit flag used to mark entities as overridden, e.g. they were originally
 * definded in code and are saved now in the database. Same as
 * (ENTITY_CUSTOM | ENTITY_IN_CODE).
 */
define('ENTITY_OVERRIDDEN', 0x03);

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

Now, the patch in #2 indicates that we should COPY the status of the existing configuration and use that same status for the imported configuration:

+    // Copy existing status to make sure the status gets correctly updated.
+    $rules_config->status = $existing_config->status;

This is WRONG. Consider what happens if the existing configuration is ENTITY_IN_CODE. When overwriting this existing configuration, the imported configuration should be set to ENTITY_OVERRIDDEN and not remain as ENTITY_IN_CODE (which is what a simple copy would do).

Instead of a simple copy, what we should be doing is setting the ENTITY_CUSTOM bit in the existing status before we copy it:

+    // Set the ENTITY_CUSTOM bit in the status bitmask, because the
+    // configuration has now been customized by the import.
+    $rules_config->status = $existing_config->status | ENTITY_CUSTOM;

If we set this bit, then the before/after statuses change the way we want them to:

Before: ENTITY_IN_CODE
After: ENTITY_IN_CODE | ENTITY_CUSTOM, which is the same as ENTITY_OVERRIDDEN

Before: ENTITY_CUSTOM
After: ENTITY_CUSTOM  | ENTITY_CUSTOM, which is still ENTITY_CUSTOM

Before: ENTITY_OVERRIDDEN
After: ENTITY_OVERRIDDEN  | ENTITY_CUSTOM, which is still ENTITY_OVERRIDDEN

(Note that for the last two cases, the patch in #2 will work, but for the first case it fails exactly the same way it did before that patch.)

The remaining question is what to do with existing configurations that have the status ENTITY_FIXED. Should we allow these to be overwritten on import? Note that we don't allow editing or deleting or enabling or disabling or reverting if the status is ENTITY_FIXED. I would argue that we shouldn't allow overwriting on import either - the import should fail in the validation function.

New patch attached which has been manually tested to fix the problem and work in all the above cases, but we still need tests to ensure the import sets the status as expected and to ensure that future patches don't break this functionality.

Here's a test that demonstrates the bug - this should FAIL without the patch from #5.

Now the fix from #5 combined with the test from #6. This should succeed.

Slight change needed - test user didn't have correct permissions.

Once again, here is the test-only patch which should fail, and the test-plus-fix patch which should run green.

Committed.