Allow user to specify rules' machine-readable names, as with rule sets

Agreed. It works that way for d7, but if we implement that for d6 we need to find a way to still auto-generate a machine name, so that creating rules doesn't necessarily involve even a step more (= finding a machine name).

"$module_$name" would be fine, as long as we have a way of ensuring it stays unique. E.g. adding two rules with the same label has to work too.

I'm just aware of the little nice js snippet of d7 that does it nicely, but allows the user to specify a custom name too; e.g. it's used here http://api.drupal.org/api/function/node_type_form/7
We also use it for rules 7.x-2.x, but I don't know whether this could be easily backported to d6?

Subscribing. If there are any tasks I could take on for this...let me know, and if I have some time I'll take a stab.

Hi!

Here is a patch that does the following:

* Adds the field "Machine Name" to the add form.
* Adds javascript that creates a machine names on the fly just as in the D7 version. This javascript is a modified version of what they use in features, and creates machine names the same way.
* Fixes the import page so that it can be used with real machine name.
* Removes the logic for enumerating rule names in the features integration. The features integration now uses the same machine names as those created in the UI, to avoid duplication of rules.

Review time! =)

Changing back to the appropriate component.

I tried the patch on a clean D6 install (Drupal 6.19, Rules 1.3). It seems that the machine name is not called correctly when creating a new triggered rule.

I got the following error when I created a triggered rule with name 'Alpha' and automatic machine name 'rules_alpha':

user warning: Unknown column 'alpha' in 'where clause' query: SELECT COUNT(*) FROM 100929_rules_rules WHERE name = alpha in /Users/johan/Sites/100929/sites/all/modules/rules/rules_admin/rules_admin.rule_forms.inc on line 92.

A similar error is displayed when adding rules to rule sets.

The rules seem to trigger fine, though – the actions are carried out and it looks well.
Everything seems to be working well together with Features as well – I've tried exporting, importing, enabling, disabling both triggered rules and rule sets inside features (even on the same site where they were created).

Good patch! Just a small bit left.

Oops, forgot '' in the query. Here's a fix for that.

Forgot to set status.

I forgot the db_result around the db_query. Here is a fix.

+++ rules_admin/rules_admin.js	30 Sep 2010 08:30:46 -0000
@@ -13,3 +13,30 @@ Drupal.behaviors.RulesAdminSetAddArg = f
+      $('#edit-name').parents('.form-item').hide();
+      ¶
+      $('#edit-label').keyup(function() {

trailing white space

Powered by Dreditor.

Fixed coding standard issue above and some other coding standards errors.

Looks fine to me!

+1, but I suggest one or two more reviews should be added before marking it RTBC.

thanks for working on that :)

I gave the patch a test and but the machine name field did not appear for me (on add). Apart from that I think it should be visible during edit too. I think it's fine if people can rename existing rules and it's the same for rules 2.

+  // Check that the machine name is valid.
+  if (preg_match('/[^a-z0-9_]/', $machine_name)) {
+    form_set_error('name', t('Machine name must be alphanumeric and underscores only.'));
+  }

I'd like machine names to match [a-z][a-z0-9_]* as I suggested in http://drupal.org/node/902644#comment-3536372. I think we can stay with the same JS regex for that.

+        $('.rules-name-suffix').empty().append(' Machine name: rules_' + machine + ' [').append($('<a href="#">'+ Drupal.t('Edit') +'</a>').click(function() {

"Machine name" should run through t() too.

H!

Thanks for the review!

The javascript should have shown up on the add page, at least it does so on my machine. Could it be cache-related?.

Anyway here's a new patch that does the following:

* Changed so that the first character must be a letter (we now use [a-z][a-z0-9_]*)
* Changed the javascript so that it changes a machine name which contains anything else than [a-z] as the first character to be replaced by the chacter 'a'. If anyone knows a more fitting replacement, be free to pop in a suggestion.
* Added the possibility to edit rules machine names. I must say that I don't agree with you that the machine names should be changable, since that can screw up features a bit. If you change the machine name which you have in a feature for instance, this will cause the rule to duplicate, since the feature contains a codified version of the same rule, and there's really no clean way of preventing that.
* Since you currently can't change machine names for rule sets, I added the possibility of doing that as well.
* Rewrote machine name validation for rule sets, all machine name validations are now performed by the function rules_admin_validate_machine_name. By doing this, we also squashed a bug when creating rule sets, Currently, there is no check that the machine name already exists for rule sets, which causes the rule set add form to overwrite an existing rule set if it has the same name. This can't happen anymore.
* Fixed translation in javascript.

As you can see, the javascript is not on the edit form. I did this since we don't want the machine name to change just because we change the label. This is also how it is handled in the D7 version of rules.

wow, good work.

Well renaming Rule sets might produce troubles, e.g. in actions or other components referring to them. So I don't think we have to provide that, but I guess we can leave it up the user if he really wants too? For triggered rules, I'd expect no such problems. However you are right with the features problem. To solve that we need to disallow renaming Rules provided in code, e.g. just show the name with the form element #disabled.

So I agree with you, generally allowing renames is problematic. So either way, disallow renames at all or just disallow renames for exported rules, would be ok for me.

+    $redirect = '/admin/rules/rules/' . $name . '/edit';

There should be no leading slash.

+    drupal_goto($redirect);

Redirects from submith handlers should be issued via FAPI.

+  $set_info['name'] = drupal_substr($set_info['name'], drupal_strlen('rules_'));

Hm, rule sets not necessarily start with a leading 'rules_' because modules may specify custom names. In that case I'd suggest just displaying that + set the form element to #disabled.

+    drupal_goto('admin/rules/rule_sets/' . $name . '/edit');

Again, redirects in submit handlers have to use FAPI.

* If I clone a Rule, there is no name prefilled. I'd suggest just doing it like Rules2: Add in the JS for machine name generation and append an _(cloned) to the label resulting in an appended _cloned in the machine name too.

Thanks for the review.
I will be coming back with a fixed patch later, just setting the issue to the right status for now =)

Sub. Great work here!

Hi!

This new patch fixes the errors in the review above and makes sure that only rules and rule sets that have the status "custom" can have their name changed.

I have left the feature to rename a rule set, since that is how it is for D7, but it can be easily removed. If we do that then we should probably open another issue for D7 and suggest removing the feature from there as well.

I tested this path for a good 20 minutes and was unable to break it. I simulated upgrading to machine names (machine names are just the previous triggered rule id). Worked just fine. Exporting to a feature worked as per normal. The locked machine name for rules defined in code also works as one would expected.

Certainly +1 from me. Tempted to mark as RTBC...

Hi,

This patch works very well and solves all my problems with features and rules.
Nice job!

+1 for me.

Sry, for coming back so late to this.

I've gave this another test-run and the improvements look great. However it looks like rule-sets are broken now - there are no rules listed any more even if a set has rules.

Apart from that I found a coding style issue:

+  $changable_name = $set_info['status'] == 'custom';
+  // Does our name start with rules?
+  if(preg_match('/^rules_/', $set_info['name'])) {
+    $set_info['name'] = drupal_substr($set_info['name'], drupal_strlen('rules_'));
+  }

missing space, after if.

- * Prefix exported rules with the module name (if given) and change the status to default.
- * If used through the features integration the module name is given.
+ * change the status of the rules to default.

Hm, I guess too much of this comment is gone? At least it has to start uppercase.

Ah, I did not use the full rule name as input to rules_admin_overview_table.

Here is a patch that fixes that problem and the coding style errors above.

That looks great. I gave this another test-run and everything seems to work fine. -> Committed!

Thanks for your patience to bring that home! :)

Moving to "Provided Module Integrations" component.