Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CDRO
Copy link

@CDRO CDRO commented Dec 9, 2019

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):

  1. The script asks for mysql, which is deprecated, so I removed the exit, since we work with PDO for MySQL anyway
  2. The script asks for pgsql extension, which is also deprecated and not used since we work with PDO for PostgreSQL
  3. The ini option for argc and argv is not needed anymore, so I removed the exit there too
  4. Since I was running the script on a frontend side first and not on cli, I had to have a fallback that worked in any case so I moved some stuff around for the configuration, so that by default the script would look for config.json in its current directory and remodelled the switch case
  5. The script stopped saying "Script is terminated." without further information, so I added the information on why it stopped (at least the first time I came accross it).

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

@AlfredoRamos
Copy link

Maybe a PHP version check would be better, to not break functionality on older PHP versions.

The script asks for pgsql extension, which is also deprecated and not used since we work with PDO for PostgreSQL

Do you have writings where we could check that? As far as I know, the pgsql extension has not been deprecated yet. The official documentation doesn't say anything about it, like the old deprecated mysql extension does.

It's a genuine question, we have scripts that are still use the pgsql extension.

@CDRO
Copy link
Author

CDRO commented Dec 10, 2019

Maybe a PHP version check would be better, to not break functionality on older PHP versions.

The script asks for pgsql extension, which is also deprecated and not used since we work with PDO for PostgreSQL
Sounds finde, I'll consider integrating that, as it absolutely makes sense.

Do you have writings where we could check that? As far as I know, the pgsql extension has not been deprecated yet. The official documentation doesn't say anything about it, like the old deprecated mysql extension does.

It's a genuine question, we have scripts that are still use the pgsql extension.
I might have to rephrase. The check for the pgsql extension does fail even though the extension is installed. In my installation, I installed pgsql and pdo_pgsql and all the pg_* functions work but the extension pgsql is not installed.

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)) {
Copy link
Owner

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);
Copy link
Owner

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);
Copy link
Owner

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?

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants