Rules engine should be optimized

This patch moves rule-related logic into its own Skinr Rules module. Otherwise, there are no changes in functionality.

Benefits to a separate module:

• Sites that are not using Rules can avoid a database hit by disabling the module.
• There's been some interest in Context integration. If someone were to develop a Skinr Context module, it'd almost certainly be a replacement for Rules, not an add-on.
• The code for Skinr Core is now smaller and perhaps easier to digest.
• Self gratification in checking a box next to SKINR RULES

Actually, look at this patch instead. It goes one step further and removes rule functions from skinr.module, which would fulfill the goal of #959556: Take rules out of skinr.module and put them in modules/skinr.skinr.inc.

Hi moonray, thanks for the feedback.

The separated Rules module doesn't rely on the UI module, so that was a extraneous dependency in skinr_rules.info, easy enough to strip out.

I'm not sure if you're suggesting that any UI components in the Skinr Rules module need to be extracted and placed into Skinr UI (or Skinr Rules UI? -- yikes, overkill), but I don't think that would be necessary. There are a few paths registered through hook_menu (admin/appearance/skinr/rules*), but they wouldn't have any front-end performance impact. If I'm wrong about that, those paths could be registered in skinr_ui.module instead, wrapped in the condition that skinr_rules be enabled.

One part of the rules code that I'm unclear about is this:

function skinr_skinr_config_info() {
  // ...
  $data['rules']['contextual_links']['html'] = array(
    'contextual_links_handler' => 'rules_skinr_contextual_links',
  );
  $data['rules']['contextual_links']['region'] = array(
    'contextual_links_handler' => 'rules_skinr_contextual_links',
  );
  return $data;
}

That would seem to add contextual links for html and region, but I haven't seen those working? Perhaps that should go into the Skinr UI module, but if they're not functional, perhaps the code can be stripped entirely. But I might be missing something.

Thanks.

This revised patch removes any dependencies on the UI and Rules modules from one another.

Some stats:

This isn't a detailed profile and the testing was very simple, but it gives a rough sense of the relative expense of each module. I used a fresh installation of D7.2 with Bartik + Skinr on my local machine, and the numbers above reflect averages on homepage refreshes after the cache had a chance to kick (i.e., after a page is rendered after a cache clear). The memory usage and page execution times were relatively constant between page loads.

Page execution time was hardly affected by the 18 extra queries added by the UI + Rules modules (260ms turned into 282ms), but again, this was run on my local machine where the I/O isn't taxed and there's no latency between PHP and the database.

@moonray - I don't think implementing skins in page/region is in the skinr.org docs yet. Should I open up a task for that?

I think @jacine is strongly opposed to replacing rules with anything else, especially if it has an added dependency, so #1007482: Make use of Context API instead of custom rules would be an add-on, and this patch would be its prerequisite to avoid duplicate functionality.

I'm not sure I like including the UI stuff in skinr_ui module the way you have it (if (module_exists('skinr_rules')) { ... }). But then again, having an additional skinr_rules_ui.module would be overkill as well. Are there any other options?

I couldn't think of any other options. I don't particularly like the module_exists() stuff inside of hook_menu(), but mostly because those structures are helpful in navigating a module. Other than that, it seems minor.

skinr_skinr_config_info() should be renamed skinr_rules_skinr_config_info() ... skinr.skinr.inc file to be appropriately renamed as well ... need tests ...

Okay, all sounds good. I'll try to write some tests. Also, it seems some existing ones need to be updated. I see a lot of errors thrown about base themes?