New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TASK] Make script compatible with PHP 7+ #73
base: master
Are you sure you want to change the base?
Conversation
Maybe a PHP version check would be better, to not break functionality on older PHP versions.
Do you have writings where we could check that? As far as I know, the It's a genuine question, we have scripts that are still use the |
Before making any changes, I'll have to figure out how to make a non strict MySQL Database with empty values in non-empty fields get fixed so that the migration to PostgreSQL is possible in the first place. |
* @return array | ||
*/ | ||
function getConfig($strPath) | ||
{ | ||
$arrRetVal = []; | ||
|
||
|
||
if(empty($strPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but imho, it should be moved to line 106, just before calling the getConfig($strPath)
function.
This is because getConfig($strPath)
expects $strPath
to be not empty
.
// Verify the extensions are loaded before running. | ||
if (!extension_loaded('pgsql')) { | ||
echo "Postgresql not enabled: you need the 'pgsql' module.\n"; | ||
exit(1); | ||
//exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pgsql
extension is required, since it runs the actual COPY
command.
Side note: the pgsql
extension is not deprecated.
@@ -99,24 +108,24 @@ function getConfig($strPath) | |||
} | |||
if (ini_get('register_argc_argv') == 0) { | |||
echo "register_argc_argv is not turned on, we can't process command line arguments.\n"; | |||
exit(1); | |||
//exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to read command line arguments without register_argc_argv
extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use for my CLI scripts
foreach($argc as $some)
I was migrating from MySQL to PostgreSQL ans couldn't find a single tool that worked nearly correctly until I stumbled upon this script.
This clearly deserves more visibility, but that's not the point of my PR.
As I was trying to make it work, I crossed some issues that I adressed directly (these are all quickfixes, none of them are proper fixes and nothing is properly documented):
I hope you'll consider accepting my PR and maybe want to work on this script some more for even further improvements (and since PHP 7.4 is out, I think there would be some nice improvements that you could make, just for the sake of it) and even publishing it on composer.
Best Regards
Tizian