Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

DB_utility restore() #2105

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants

Added a new function which can restore a backup generated by backup().

I have added the documentation.
Please let me know if any tests need to be written for the function.

aniketpant added some commits Dec 30, 2012

@aniketpant aniketpant New Feature: Database restore
Added a new function for easy restore of the active database.

Signed-off-by: Aniket Pant <me@aniketpant.com>
c5f5021
@aniketpant aniketpant Added documentation for restore()
Signed-off-by: Aniket Pant <me@aniketpant.com>
a7acf70
@aniketpant aniketpant doc: fixed documentation for restore()
Signed-off-by: Aniket Pant <me@aniketpant.com>
f37914d

@timw4mail timw4mail and 2 others commented on an outdated diff Jan 2, 2013

system/database/DB_utility.php
+
+ $db_name = $CI->db->database; // get name of active database
+
+ $CI->dbforge->drop_database($db_name); // drops the active database
+ $CI->dbforge->create_database($db_name); // creates the original database again
+
+ $CI->db->close();
+ $CI->load->database();
+
+ $flag = FALSE;
+
+ foreach ($queries as $query)
+ {
+ if ($query != '' || $query != NULL)
+ {
+ $CI->db->query("SET FOREIGN_KEY_CHECKS = 0");
@timw4mail

timw4mail Jan 2, 2013

Contributor

This is a MySQL specific query.

@aniketpant

aniketpant Jan 2, 2013

@timw4mail I should rather remove that part and go ahead with the query. Or should I put in other db based commands?

@timw4mail

timw4mail Jan 2, 2013

Contributor

Ideally, any db-specific functions should be called in their individual drivers.

@narfbg

narfbg Jan 3, 2013

Contributor

Indeed, you should follow @timw4mail's advice for any additions to the DB classes.
However, in this particular case - it's better to remove it altogether, it's otherwise pointless to have the foreign_key_checks option when creating a backup. Why would you want to use it there if you'll be enforcing it in here? It doesn't make sense.

@aniketpant

aniketpant Jan 3, 2013

I agree. I will fix this.

@narfbg narfbg commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ $CI->load->database();
+ $CI->load->dbforge();
+
+ $db_name = $CI->db->database; // get name of active database
+
+ $CI->dbforge->drop_database($db_name); // drops the active database
+ $CI->dbforge->create_database($db_name); // creates the original database again
+
+ $CI->db->close();
+ $CI->load->database();
+
+ $flag = FALSE;
+
+ foreach ($queries as $query)
+ {
+ if ($query != '' || $query != NULL)
@narfbg

narfbg Jan 3, 2013

Contributor

Use OR instead of ||, as per the style guide.

Also, both of these expressions produce the same result - you're not doing a comparison by type. A simple empty() check would be cleaner.

@narfbg narfbg and 1 other commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ $CI->dbforge->drop_database($db_name); // drops the active database
+ $CI->dbforge->create_database($db_name); // creates the original database again
+
+ $CI->db->close();
+ $CI->load->database();
+
+ $flag = FALSE;
+
+ foreach ($queries as $query)
+ {
+ if ($query != '' || $query != NULL)
+ {
+ $CI->db->query("SET FOREIGN_KEY_CHECKS = 0");
+ $check = $CI->db->query($query);
+ $CI->db->query("SET FOREIGN_KEY_CHECKS = 1");
+ if($check) {
@narfbg

narfbg Jan 3, 2013

Contributor

Again, per the style guide - curly braces must be on their own new line. And really, you should just do if ($this->db->query($query)) ... it's pointless to assign the result. Even if you keep the foreign_key_checks lines - they're better off before and after the foreach, not in it.

@aniketpant

aniketpant Jan 3, 2013

Sorry, for this error over here.

@narfbg narfbg and 1 other commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ {
+ $prefs[$key] = $params[$key];
+ }
+ }
+ }
+
+ // Check if given filepath is not empty
+ if ($prefs['filepath'] == '')
+ {
+ show_error('Parameter filepath is not set. Please specify a filepath.');
+ return FALSE;
+ }
+ // Do the following if filepath is specified
+ else
+ {
+ $CI =& get_instance();
@narfbg

narfbg Jan 3, 2013

Contributor

You don't need this. You already have access to the database via $this->db and the read_file() helper function is just an alias for the native get_file_contents(), so it's pointless to use it.

@aniketpant

aniketpant Jan 3, 2013

I will switch to get_file_contents()

@narfbg narfbg and 1 other commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ }
+
+ // Check if given filepath is not empty
+ if ($prefs['filepath'] == '')
+ {
+ show_error('Parameter filepath is not set. Please specify a filepath.');
+ return FALSE;
+ }
+ // Do the following if filepath is specified
+ else
+ {
+ $CI =& get_instance();
+ $CI->load->helper('file');
+
+ $string = read_file($prefs['filepath']);
+ $queries = explode(';', $string);
@narfbg

narfbg Jan 3, 2013

Contributor

And what if you have ; as part of the data to be inserted?

@aniketpant

aniketpant Jan 3, 2013

That's a good one. Let me sort that out and make another commit.

@narfbg narfbg commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ show_error('Parameter filepath is not set. Please specify a filepath.');
+ return FALSE;
+ }
+ // Do the following if filepath is specified
+ else
+ {
+ $CI =& get_instance();
+ $CI->load->helper('file');
+
+ $string = read_file($prefs['filepath']);
+ $queries = explode(';', $string);
+
+ if ($string)
+ {
+ // Load database and dbforge library
+ $CI->load->database();
@narfbg

narfbg Jan 3, 2013

Contributor

This does nothing.

@narfbg narfbg and 1 other commented on an outdated diff Jan 3, 2013

system/database/DB_utility.php
+ {
+ $CI =& get_instance();
+ $CI->load->helper('file');
+
+ $string = read_file($prefs['filepath']);
+ $queries = explode(';', $string);
+
+ if ($string)
+ {
+ // Load database and dbforge library
+ $CI->load->database();
+ $CI->load->dbforge();
+
+ $db_name = $CI->db->database; // get name of active database
+
+ $CI->dbforge->drop_database($db_name); // drops the active database
@narfbg

narfbg Jan 3, 2013

Contributor

Why?

@aniketpant

aniketpant Jan 3, 2013

Instead of truncating, I thought it would be better to drop and recreate the db.

@narfbg

narfbg Jan 3, 2013

Contributor

Truncate is better, especially if some custom parameters have been used while creating the database. Although, I could again argue that a "restore" procedure should just execute what's in the backup.sql - any create clauses should be in there, ideally.

@aniketpant

aniketpant Jan 3, 2013

So you suggest that instead of dropping the DB manually, I let them be. And
ideally there would be create and alter commands in the backup file. I
guess we can go ahead with that. It's more suited to a generalized scenario.

Maybe later parameters can be introduced like the backup() command.
On Jan 3, 2013 5:26 PM, "Andrey Andreev" notifications@github.com wrote:

In system/database/DB_utility.php:

  •   {
    
  •       $CI =& get_instance();
    
  •       $CI->load->helper('file');
    
  •       $string = read_file($prefs['filepath']);
    
  •       $queries = explode(';', $string);
    
  •       if ($string)
    
  •       {
    
  •           // Load database and dbforge library
    
  •           $CI->load->database();
    
  •           $CI->load->dbforge();
    
  •           $db_name = $CI->db->database; // get name of active database
    
  •           $CI->dbforge->drop_database($db_name); // drops the active database
    

Truncate is better, especially if some custom parameters have been used
while creating the database. Although, I could again argue that a "restore"
procedure should just execute what's in the backup.sql - any create clauses
should be in there, ideally.


Reply to this email directly or view it on GitHubhttps://github.com/EllisLab/CodeIgniter/pull/2105/files#r2537617.

@narfbg

narfbg Jan 3, 2013

Contributor

yep

@timw4mail and @narfbg Thanks for the review on my commits.
I will immediately fix these problems and make another commit.

Thank you.

aniketpant added some commits Jan 11, 2013

@aniketpant aniketpant Fixed bugs based on suggestions
Changed || to OR
Converted statements to appropriate calls
Instead of using read_file(), using file_get_contents()
Added parameter for delimiter

Signed-off-by: Aniket Pant <me@aniketpant.com>
768787d
@aniketpant aniketpant Documentation updated to intoduce new parameter
Signed-off-by: Aniket Pant <me@aniketpant.com>
d1b0ee5

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ {
+ if (isset($params[$key]))
+ {
+ $prefs[$key] = $params[$key];
+ }
+ }
+ }
+
+ // Check if given filepath is not empty
+ if (empty($prefs['filepath']))
+ {
+ show_error('Parameter filepath is not set. Please specify a filepath.');
+ return FALSE;
+ }
+ // Do the following if filepath is specified
+ else
@narfbg

narfbg Jan 15, 2013

Contributor

You don't need this else and the whole nesting/indentation below since the above if does a return.
Additionally, it is safe to use foreach() on an empty array, so the if on line 439 could alse be removed.

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ {
+ show_error('Parameter filepath is not set. Please specify a filepath.');
+ return FALSE;
+ }
+ // Do the following if filepath is specified
+ else
+ {
+ $string = file_get_contents($prefs['filepath']);
+ $queries = explode($prefs['delimiter'], $string);
+
+ if ($string)
+ {
+ $CI =& get_instance();
+
+ // Load database library
+ $CI->load->database();
@narfbg

narfbg Jan 15, 2013

Contributor

You don't need this (and the get_instance() as well) - the DB_utility loader takes care of that. If it didn't, you wouldn't be able to use $this->db->query() below.

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ {
+ $string = file_get_contents($prefs['filepath']);
+ $queries = explode($prefs['delimiter'], $string);
+
+ if ($string)
+ {
+ $CI =& get_instance();
+
+ // Load database library
+ $CI->load->database();
+
+ $flag = FALSE;
+
+ foreach ($queries as $query)
+ {
+ if (!empty($query))
@narfbg

narfbg Jan 15, 2013

Contributor

Spaces around the exclamation mark.

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ $queries = explode($prefs['delimiter'], $string);
+
+ if ($string)
+ {
+ $CI =& get_instance();
+
+ // Load database library
+ $CI->load->database();
+
+ $flag = FALSE;
+
+ foreach ($queries as $query)
+ {
+ if (!empty($query))
+ {
+ if($this->db->query($query))
@narfbg

narfbg Jan 15, 2013

Contributor

A space needs to be inserted between the if and the opening parenthesis.
Furthermore, this whole $flag stuff is pointless - just do this:

foreach ($queries as $query)
{
    if ( ! empty($query) && ! $this->db->simple_query($query))
    {
        return FALSE;
    }
}

if ($prefs['delete_after_upload'] === TRUE)
{
    unlink($prefs['filepath']);
}

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ }
+ return TRUE;
+ }
+ else
+ {
+ return FALSE;
+ }
+ }
+ else
+ {
+ show_error($prefs['filepath'] . ' could not be read. Please make sure it\'s accessible.');
+ return FALSE;
+ }
+ }
+
+ return;
@narfbg

narfbg Jan 15, 2013

Contributor

return TRUE and change the docblock to say boolean - this is what you return everywhere else.

@narfbg narfbg commented on an outdated diff Jan 15, 2013

system/database/DB_utility.php
+ if ($flag)
+ {
+ if ($prefs['delete_after_upload'] == TRUE)
+ {
+ unlink($prefs['filepath']);
+ }
+ return TRUE;
+ }
+ else
+ {
+ return FALSE;
+ }
+ }
+ else
+ {
+ show_error($prefs['filepath'] . ' could not be read. Please make sure it\'s accessible.');
@narfbg

narfbg Jan 15, 2013

Contributor

It would be nice if we depend on db_debug here and not force an error screen:

if ($this->db->db_debug)
{
    show_error($prefs['filepath']." could not be read. Please make sure it's accessible.");
}

(also changed the code style for better readability.

Edit: Please do the same on line 453.

aniketpant added some commits Jan 21, 2013

@aniketpant aniketpant Made changes according to suggestions
Signed-off-by: Aniket Pant <me@aniketpant.com>
5f5b5c3
@aniketpant aniketpant Fixed indentation
Signed-off-by: Aniket Pant <me@aniketpant.com>
b29e5a0

@narfbg narfbg closed this Jan 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment