Allow other modules to alter dump parameters
smk-ka - July 30, 2009 - 16:52
| Project: | Demonstration site (Sandbox / Snapshot) |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | smk-ka |
| Status: | closed |
Description
This primary purpose of this patch is to add hook_demo_dump_alter() for other modules, to allow them to alter the list of tables before they are dumped. This required to move the database tables list to demo_dump_form, and changing several functions to behave more API-like (which is A Good Thing™).
| Attachment | Size |
|---|---|
| demo_dump_alter.patch | 16.63 KB |
#1
@@ -49,7 +53,7 @@ function demo_dump_db($filename, $exclud- * Returns the name of the active database.
+ * Return the name of the active database.
Nice catch! But actually, D7 (HEAD) changed the coding standards for PHPDoc summaries to be in the third person form, as in "Returns foo bar.". :-/
So we might want to revert this change.
@@ -142,7 +163,9 @@ function _demo_dump_table_data($table) {if ($query_size + strlen($insert_buffer) > 50000) {
- $output .= ";\n";
+ // Flush buffer to disc.
+ fwrite($fp, $output . ";\n");
+ $output = '';
If I wouldn't know what we are doing here and why, I would not grok why $output is entirely reset in a loop here. That could use a comment.
@@ -77,7 +77,10 @@ function demo_admin_settings_submit($for-function demo_manage() {
+/**
+ * Menu callback to display manage snapshots form.
+ */
+function demo_manage_form() {
...
-function demo_manage_submit($form, &$form_state) {
+function demo_manage_form_submit($form, &$form_state) {
Hmmm... We should move those changes to a separate issue, I think.
...oh, scratch that, I see now that it's required for the other changes.
@@ -144,60 +150,62 @@ function demo_dump() {+/**
+ * Menu callback to display snapshot selection form.
+ */
Ideally, consistently use "Menu callback; Display snapshot selection form."
@@ -235,11 +254,11 @@ function demo_reset($filename = 'demo_si- if ($table != $dt_watchdog || $is_version_1_0_dump) {
+ if ($table != $watchdog || $is_version_1_0_dump) {
Do we still need this?
@@ -298,6 +317,8 @@ function demo_reset($filename = 'demo_si+ // Save time of last reset.
+ variable_set('demo_reset_last', $_SERVER['REQUEST_TIME']);
...
- variable_set('demo_reset_last', $time);
...
demo_reset(variable_get('demo_dump_cron', 'demo_site'), FALSE);
- variable_set('demo_reset_last', $time);
oh, nice! This could finally solve the "cron not always resetting" issue! :)
...shouldn't we use time() here?
...hrm, thinking twice about that, it might as well break cron runs again, because demo_cron() calculates the difference to the last cron run. :-/
@@ -342,6 +363,24 @@ function demo_get_fileconfig($filename =+ $inc_file = drupal_get_path('module', 'demo') . '/database_' . $engine . '_dump.inc';
+ if (!file_exists($inc_file)) {
+ drupal_set_message(t('@engine support not implemented yet.', array('@engine' => ucfirst($engine))), 'error');
+ return FALSE;
+ }
+
+ require_once $inc_file;
Why aren't we using module_load_include() here?
This review is powered by Dreditor
#2
Rerolled with sun's suggestions, except:
This looks wrong: a sentence continues lowercase after a semicolon. Core doesn't seem to have a clear rule either, but continues most frequently with a lowercase word.
Yes, as long as DROP TABLE is not moved to the SQL dump we still need to drop tables before restoring.
No, time() would be the current time which can be different on each invocation (depending on the number of cron implementations that ran before). $_SERVER['REQUEST_TIME'], on the other hand, is always the time when the request started, which is much closer to how the system crontab works. The mixing of time() and $_SERVER['REQUEST_TIME'] before could have been the cause for miscalculations.
#3
Thanks for reporting, reviewing, and testing! Committed to all branches.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.
#4
Automatically closed -- issue fixed for 2 weeks with no activity.