refactor server_import and related files #542

Merged
merged 20 commits into from Jul 29, 2013

Conversation

Projects
None yet
2 participants
@xmujay
Contributor

xmujay commented Jul 27, 2013

No description provided.

@ghost ghost assigned lem9 Jul 27, 2013

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 27, 2013

Contributor

Hi Marc,
this pull request is about the refactor about import page for server, table, database. I have tested fully.

changes:

  1. rename display_import.lib.php to display_import.inc.php
  2. split long code to functions
  3. move function to display_import.lib.php
  4. fix the included file: table, database, server
  5. render html at once
  6. split long js

thanks for your review

Contributor

xmujay commented Jul 27, 2013

Hi Marc,
this pull request is about the refactor about import page for server, table, database. I have tested fully.

changes:

  1. rename display_import.lib.php to display_import.inc.php
  2. split long code to functions
  3. move function to display_import.lib.php
  4. fix the included file: table, database, server
  5. render html at once
  6. split long js

thanks for your review

@@ -139,6 +139,9 @@ function PMA_importRunQuery($sql = '', $full = '', $controluser = false,
}
$sql_query = $import_run_buffer['sql'];
$sql_data['valid_sql'][] = $import_run_buffer['sql'];
+ if(! isset($sql_data['valid_queries'])) {

This comment has been minimized.

@xmujay

xmujay Jul 27, 2013

Contributor

here is to fix undefined error of $sql_data['valid_queries'] after imported

@xmujay

xmujay Jul 27, 2013

Contributor

here is to fix undefined error of $sql_data['valid_queries'] after imported

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 27, 2013

I am not aware of such a jQuery 1.8.3 method, please give me a URL of its documentation.

I am not aware of such a jQuery 1.8.3 method, please give me a URL of its documentation.

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 27, 2013

Contributor

Bin,
when trying a table import, I don't see the table name in the top portion. Instead I see a big number like 31.

Contributor

lem9 commented Jul 27, 2013

Bin,
when trying a table import, I don't see the table name in the top portion. Instead I see a big number like 31.

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 27, 2013

Contributor

thanks Marc, I am fixing the issues now. thanks

Contributor

xmujay commented Jul 27, 2013

thanks Marc, I am fixing the issues now. thanks

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 27, 2013

Contributor

Hi Bin,
you mention "fully tested". Which upload progress mechanisms have you tested (progress, apc)?

Contributor

lem9 commented Jul 27, 2013

Hi Bin,
you mention "fully tested". Which upload progress mechanisms have you tested (progress, apc)?

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 27, 2013

Contributor

Hi Marc, I just test from my side on database, table, server levels import. the import process is working well and not error output.

but I have no idea about the apc testing..

Contributor

xmujay commented Jul 27, 2013

Hi Marc, I just test from my side on database, table, server levels import. the import process is working well and not error output.

but I have no idea about the apc testing..

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 27, 2013

Contributor
Contributor

lem9 commented Jul 27, 2013

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 27, 2013

Contributor

thanks, it is useful. I will test upload bar branch now.

BTW, the table name issues have been fixed.

Contributor

xmujay commented Jul 27, 2013

thanks, it is useful. I will test upload bar branch now.

BTW, the table name issues have been fixed.

1. fix table name after import table
2. using jquery getScript to load js file
@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 27, 2013

Contributor

still have some upload bar issue. close it. will reopen when fixed. thanks

Contributor

xmujay commented Jul 27, 2013

still have some upload bar issue. close it. will reopen when fixed. thanks

@xmujay xmujay closed this Jul 27, 2013

@xmujay xmujay reopened this Jul 28, 2013

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 28, 2013

Contributor

Hi Marc,

for the js part, If I split to a separated file, the js file can't use the PHP variable. so now I add a new function to render javascript out : PMA_getHtmlForImportNoPlugin

for this part of js code, I will refactor more if I have idea to split js to a separated file. thanks

Contributor

xmujay commented Jul 28, 2013

Hi Marc,

for the js part, If I split to a separated file, the js file can't use the PHP variable. so now I add a new function to render javascript out : PMA_getHtmlForImportNoPlugin

for this part of js code, I will refactor more if I have idea to split js to a separated file. thanks

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 28, 2013

Contributor

I have tested both upload on progress and apc.

the status bar can working well.

but the process is so quick, so if you want to see the bar, you can set "if (finished == true)" to "if (false)"
or upload a large file. thanks
https://github.com/phpmyadmin/phpmyadmin/pull/542/files#L3R553

Contributor

xmujay commented Jul 28, 2013

I have tested both upload on progress and apc.

the status bar can working well.

but the process is so quick, so if you want to see the bar, you can set "if (finished == true)" to "if (false)"
or upload a large file. thanks
https://github.com/phpmyadmin/phpmyadmin/pull/542/files#L3R553

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 28, 2013

Contributor

the side bar should be reloaded when import finished

Contributor

xmujay commented Jul 28, 2013

the side bar should be reloaded when import finished

@xmujay xmujay closed this Jul 28, 2013

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 28, 2013

Contributor

Hi Bin,
did you intentionally close this pull request?

Contributor

lem9 commented Jul 28, 2013

Hi Bin,
did you intentionally close this pull request?

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 28, 2013

Contributor

Hi Marc,

Yes.
I close this pull request because there is a remaining issue should be fixed.

I have no idea how to put this line about reload the navigation panel when import finished.
https://github.com/phpmyadmin/phpmyadmin/pull/542/files#L3L140

I will discuss with other developers on mail list and make more investigations.

I will reopen the pull request when fixing this issue.

BTW, other features are all working well. thanks

Contributor

xmujay commented Jul 28, 2013

Hi Marc,

Yes.
I close this pull request because there is a remaining issue should be fixed.

I have no idea how to put this line about reload the navigation panel when import finished.
https://github.com/phpmyadmin/phpmyadmin/pull/542/files#L3L140

I will discuss with other developers on mail list and make more investigations.

I will reopen the pull request when fixing this issue.

BTW, other features are all working well. thanks

@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 29, 2013

Contributor

Hi Bin,
please be careful because a bug fix was merged today, see 80e6ecc

Contributor

lem9 commented Jul 29, 2013

Hi Bin,
please be careful because a bug fix was merged today, see 80e6ecc

@xmujay

This comment has been minimized.

Show comment
Hide comment
@xmujay

xmujay Jul 29, 2013

Contributor

thanks Marc,

I have fixed all the issues about this pull request.

and I tested both on process and apc upload for database, server, table import.

thanks for your review.

Contributor

xmujay commented Jul 29, 2013

thanks Marc,

I have fixed all the issues about this pull request.

and I tested both on process and apc upload for database, server, table import.

thanks for your review.

@xmujay xmujay reopened this Jul 29, 2013

+ $html .= ' .show(); ';
+ $html .= ' $("#import_form_status").load("import_status.php?'
+ . 'message=true&' . $import_url . '"); ';
+ $html .= ' PMA_reloadNavigation(); ';

This comment has been minimized.

@xmujay

xmujay Jul 29, 2013

Contributor

here is to reload the navigation panel when import finished on JS level.

thanks Marc's advice

@xmujay

xmujay Jul 29, 2013

Contributor

here is to reload the navigation panel when import finished on JS level.

thanks Marc's advice

lem9 added a commit that referenced this pull request Jul 29, 2013

Merge pull request #542 from xmujay/server_refactor2
refactor server_import and related files

@lem9 lem9 merged commit 36a22d2 into phpmyadmin:master Jul 29, 2013

1 check passed

default The Travis CI build passed
Details
@lem9

This comment has been minimized.

Show comment
Hide comment
@lem9

lem9 Jul 29, 2013

Contributor

Good job!

Contributor

lem9 commented Jul 29, 2013

Good job!

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