Posted by Dane Powell on August 9, 2011 at 6:04pm
5 followers
| Project: | Elysia Cron |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | gotheric |
| Status: | needs review |
Issue Summary
The way that Elysia Cron (EC) handles variables is really unorthodox, and presents major compatibility issues. For instance, you cannot export EC settings using Strongarm / Features because EC reads variables directly from the variables table, instead of using the quite standard variable_get() / variable_set() functions. Why does EC not use these functions? It is very bad form to hit the database directly like EC is currently doing.
EC should either be using standard Drupal variable-handling protocols (variable_get / variable_set), or it should use CTools to make Elysia Cron settings exportable, and store them in their own table.
Comments
#1
The reason EC doesn't use variable_get/variable_set is for performance improvements.
In a system where EC is called every minute (for fine-grained control of cron jobs), variable_set will invalidate variable cache too often, so the site has a HUGE performance degradation.
Skipping cache handling of variable_set (that invalidates cache for every write) solves the problem brilliantly.
However EC stores variables in {variable} table in Drupal standard-way, so a module that accesses it directly should have no problem about it.
I don't know "Strongarm / Features", and i don't know why they simply do not export {variable} table.
When i have some time i'll look at it to see if there are some methods to be compatible with them.
#2
You really should not be using the variables table without using variable_set/variable_get. Fortunately there are some good solutions.
The best long-term solution would be to create your own table and store the frequently-changing variables there, rather than in variables. Then use variable_set/variable_get just for true configuration variables.
In the short term, it would be quite easy to tweak the code so that frequently-changing variables (such as last run time) are stored using your custom method in the variables table, and infrequently-changing variables (configuration variables) are stored/read using variable_get().
You should really look into Strongarm/Features - they are quite standard and popular modules that allow you to store configuration in code rather than in the database. This allows you to re-use the same code across multiple sites and easily manage configuration updates.
#3
Agree, also stumbled upon this now. Variables are available in strongarm for export but does not show up when reverting a feature.
#4
I hadn't noticed how radically the variable handling changed between the last stable release and the development version. Most/all variables thankfully are no longer stored in the variables table, but in a custom elysia_cron table. This patch makes the rule, weight, and context fields of that table exportable using Features.
If we can't find any reviewers for this, you can trust that it's a fairly minimal change that adds very useful functionality! So the sooner it gets committed, the better.
#5
Committed your patch.
Just one edit: why you put "no export" to "disabled" field? (it is useful to export it)
#6
I've never used the 'disabled' field myself, I didn't know if that would be useful to export. Apparently it is! :)
#7
Hi Dane :)
I was pleased to find this today (thanks!) and gave it a shot, but it's not working for me. I exported the config via features and pushed the code to my staging site, but the exported config isn't reflected on the elysia cron admin pages, even after hitting the "reset to defaults" button. Reverting the elysia_cron config via features' UI results in an empty elysia_cron database table (I've run into this problem randomly with other features configs). Is it just me? I notice that "disabled" appears twice in each exported rule:
function tbm_default_elysia_cron() {
$export = array();
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = ':default';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export[':default'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = ':offers_update_expired';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export[':offers_update_expired'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'amazon_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['amazon_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'backup_migrate_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['backup_migrate_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'captcha_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['captcha_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'ctools_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['ctools_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'dblog_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '0 */6 * * *';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['dblog_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'field_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['field_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'googleanalytics_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['googleanalytics_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'logintoboggan_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['logintoboggan_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'node_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['node_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'redirect_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '*/15 * * * *';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['redirect_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'rules_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '*/15 * * * *';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['rules_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'statistics_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['statistics_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'system_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['system_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'tbm_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['tbm_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'tbm_expiration_notification';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '*/15 * * * *';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['tbm_expiration_notification'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'tbm_expire_old_listings';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '*/15 * * * *';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['tbm_expire_old_listings'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'tbm_offers_update_expired';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '* * * * *';
$cron_rule->weight = 0;
$cron_rule->context = 'offers_update_expired';
$export['tbm_offers_update_expired'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'update_cron';
$cron_rule->disabled = TRUE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['update_cron'] = $cron_rule;
$cron_rule = new stdClass;
$cron_rule->disabled = FALSE; /* Edit this to true to make a default cron_rule disabled initially */
$cron_rule->api_version = 1;
$cron_rule->name = 'votingapi_cron';
$cron_rule->disabled = FALSE;
$cron_rule->rule = '';
$cron_rule->weight = 0;
$cron_rule->context = '';
$export['votingapi_cron'] = $cron_rule;
return $export;
}
#8
Also just noticed my watchdog flooded with:
Notice: Undefined property: stdClass::$in_code_only in variable_features_export() (line 158 of /home/textbook/sub_staging/public_html/sites/all/modules/strongarm/strongarm.module).#9
Hey JD :)
Ugh, well as usual, I have grossly underestimated the complexity of this issue. Basically, elysia_cron_get() and elysia_cron_set() need to be updated to use ctools_export_crud_load() and ctools_export_crud_save(), respectively. The upside to this is that ctools handles a lot of the custom caching that EC currently does, and is also version-agnostic (D6 vs D7), which greatly simplifies the code. The downside is that the database schema will have to be altered to include a primary key, and the 'disabled' field will need to be changed (as you pointed out 'disabled' is a reserved keyword with the ctools export API).
I'm working on it, don't know how long it will take.
#10
The schema has a primary key (name).
No problems about changing "disabled" name, it should be quite simple.
But i don't want to have a dependency with ctools, so wrap every ctools calls with the proper "if (module_exists...)" of "if (function_exists...)".
Thanks
#11
I think you should really reconsider the benefits of making ctools a dependency- it's become the de facto standard for handling CRUD (create, read, update, delete) operations- so much so that many people want its CRUD functions included in core. All of the best-known modules (i.e. Views) require ctools. Using ctools has many benefits- you don't have to maintain separate database-handling code for D6 and D7, there's a lot less complexity, free well-designed caching and export, etc... the only potential downside is that people have to use one other module that they likely already have (the kind of people using EC are also the kind of people who are likely using other major modules already requiring ctools)
#12
Also, I think this issue is going to have to wait on #1302856: Properly segregate code branches, because it will require schema updates to bring EC into compliance with Drupal standards (numeric serial primary key, machine name, and human-readable name, instead of just 'name'), and I'm hesitant to do any schema updates with EC's non-orthodox architecture.
#13
Just for the record, here's as far as I got today (assuming ctools would be a requirement)
I should tell you that if you don't lend support to this issue of making ctools a dependency, and the linked issue of cleaning up the code branches, I will take my efforts elsewhere, most likely to start a fork of Elysia Cron. Don't get me wrong, I've loved EC and found it very useful, but I'm worried that it's not being maintained in a sustainable manner. I recognize that you've been fairly receptive to change until now, which I really appreciate, but as a non-maintainer there's only so much I can do to help with such fundamental issues, and I don't sense that you have the time or the desire to make the necessary changes.
#14
The main reason behind Elysia Cron is one: performance.
Mainly for that, i don't want mandatory (unnecessary) dependencies with other modules.
Furthermore, Elysia cron should be a "nearly core" module, so it should not have dependencies with third party modules.
Everyone should be able to use it, even site administration that needs to have a MINIMAL set of modules, to keep all site's code under control and maximize site performance.
(For example: I don't use views module nor ctools module in none of my sites, and i know a lot of drupal developers that do the same thing...)
Having a minimal approach make the module usable by everyone.
Even from a developer point of view, adding a mandatory dependency only to achieve the "backup features", not a core feature for a cron management module, is a bad idea, IMHO.
I think a good backup module should be transparent, and should give the possibility to use it in a "non mandatory" manner.
However: the philosophy behind Elysia Cron is to be minimal and fast.
If you want to add an OPTIONAL dependency there is no problem about it.
But i cannot accept a MANDATORY dependency.
Sorry.
About the fork...
I'm happy that you like my module, and there is absolutely no problem if you want to fork its code to start a module that follow your needs! (and probably the needs of a lot of other drupal users!)
It's open source!
Really, it's an honour for me to know that someone have used my work to start something else :)
If you want, before forking the project you can consider trying to alter other cron projects started after Elysia Cron and similar to it (there are several now). Maybe they can be more similar to your point of view. I don't know.
Let me know what you will decide, maybe in future we can share some thoughts/code/forces for our projects.
#15
Thanks for your thoughtful comments. Perhaps what I interpreted as a "lack of time or desire" on your part is really just a fundamental difference in how we approach module design. I prefer relying on existing modules as much as possible to prevent duplication of effort, although I certainly understand the risks and burden sometimes associated with that approach. You prefer having complete control of your code to the smallest detail, in order to eke out every last bit of performance, even if that means eschewing existing design patterns :) I don't suppose either approach is inherently good or bad, but I do believe that the Drupal community as a whole tends to look at things more my way- perhaps that is why people complain about how slow Drupal can be! :)
So, I believe that I will research existing cron modules and see if any can serve as a basis for the type of project I have in mind.
By the way, Features is not just a "backup feature", but IMHO a critical tool for any site developer- I am surprised you've never used it. It allows you to store configuration in code, so that (for instance) instead of manually setting up Elysia Cron on every new site you create, you can simply include an "Elysia Cron Settings" Feature in an installation profile and it's set up automatically. Similarly, Features provide a way of storing configurations in version control and moving configurations between development, staging, and production platforms. I don't know how I got by without them!
#16
That's how/why I use features :)
Is it necessary to use ctools in order to offer features support? I thought a module could offer optional features support without ctools. Either way, can't this be accomplished without taking a required dependency?
#17
Hey JD- that might be possible, yes- I've always implemented Features compatibility through CTools, because you get the same functionality with way less effort, plus some awesome CRUD functionality for free.
I won't be putting in the effort on EC for this though... right now my next best hope is Ultimate Cron: #1296898: Integrate with Features / CTools Exportables
#18
I've committed a new version with ctools export support, let me know it this works for you.
It's only for D7 (i'll commit a D6 version in some days).
(To reset db settings, and use ctools/features defaults, you should do a "reset to defaults" from elysia cron settings page. You can also use the "Revert components" code from features, but that will reset also cron statistics).
FYI: I've analyzed ctools better. Indeed it has some good ideas behind it, but the code of the export functionality is really messed up!
For a project like "elysia cron" using it's database layer add too much overhead, and gives no real benefit... i really prefer not to stuck with it...
Adding in a non mandatory way is possible (i've done it), and SHOULD be easy reading the documentation/API... but in real world is not so easy:
1. ctools export doesn't properly support NULL values in tables (and that's a GREAT problem, IMHO).
2. it support a lot of callbacks (and this is good), but doesn't use them always. For example, you can define a callback to make you own load function, but in some cases it doesn't use it, and that makes the callback quite useless...
3. it doesn't support partial records (records with only some fields that are "configuration"). However this can be considered a problem of the caller that has a poor db structure (elysia cron should separate config data from stats data, that's true).
If problems 1 and 2 were not there the coding would be quite easy.
Maybe i'll propose some patches to ctools mantainer...
Apart from this... now the ctools/features integration should work.
Let me know if it's true...
#19
Thanks gotheric, I may try that. I'm still trying to compare Ultimate Cron and Elysia Cron on performance, features, maintainability, etc... they each seem to have their own advantages, but the goals are largely the same. I wonder if you might consider joining forces... but that's another issue :)
I agree that ctools, despite its value, still has some issues. The NULL handling is really annoying, and something that has bitten me more than once lately :) I searched but couldn't find any issues regarding it- one of us should definitely investigate further and open an issue.
I haven't seen the issue with callbacks not being used...
I also ran into the issue with 'partial' exports, and came to the same conclusion- we ended up restructuring the Ultimate Cron schema to handle this, and this resulted in a much more sane schema IMHO.