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

Add Database Connection Helper Command #122

Merged
merged 13 commits into from
Feb 11, 2020
Merged

Add Database Connection Helper Command #122

merged 13 commits into from
Feb 11, 2020

Conversation

igmoweb
Copy link
Contributor

@igmoweb igmoweb commented Feb 6, 2020

WIP

Issue: #1

  • composer server db
  • composer server db info
  • composer server db spf

composer server db info sample:
Captura de pantalla 2020-02-06 a las 16 23 59

Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @igmoweb, styling of the output looks good. Just a few little tweaks to do.

docs/database.md Outdated Show resolved Hide resolved
docs/database.md Outdated Show resolved Hide resolved
docs/database.md Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically there now but a few final improvements to make, thanks for persevering with something unfamiliar to you, I hadn't realised that. I'll make an extra effort to not assume any prior knowledge in future when writing up specs for things like this

inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
inc/composer/class-command.php Outdated Show resolved Hide resolved
docs/database.md Outdated Show resolved Hide resolved
docs/database.md Show resolved Hide resolved
$columns,
$lines
), $return_val );

return $return_val;
}

protected function db( InputInterface $input, OutputInterface $output ) {
$db = $input->getArgument( 'options' )[0] ?? '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$db = $input->getArgument( 'options' )[0] ?? '';
$db = $input->getArgument( 'options' )[0] ?? null;

Just going to test this out, I think it's necessary for the default command to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested both ways and it always works but anyway, null looks slightly better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh of course, the switch doesn't do strict matching. It's a == comparison 👍


exec( "open $output_file_path", $null, $return_val );
if ( $return_val !== 0 ) {
$output->writeln( '<error>You must have Sequel Pro installed to use this command</error>' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$output->writeln( '<error>You must have Sequel Pro installed to use this command</error>' );
$output->writeln( '<error>You must have <href=https://www.sequelpro.com/>Sequel Pro</> installed to use this command</error>' );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that <href will only render properly in iTerm2 and GNOME. I don't use either of both and the output looks very ugly. I have added the link between parenthesis if that's ok

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh me either, yeah good call - I missed that bit in the docs

inc/composer/class-command.php Outdated Show resolved Hide resolved
passthru( "$base_command mysql --database=wordpress --user=root -pwordpress", $return_val );
break;
default:
$output->writeln( "<error>The subcommand $db is not recognized</error>" );
Copy link
Contributor

@roborourke roborourke Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$output->writeln( "<error>The subcommand $db is not recognized</error>" );
$output->writeln( "<error>The subcommand $db is not recognized</error>" );
return 1;

Adding a non zero exit code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've assigned 1 to $return_val instead. Although we already have one early return, it can get confusing if we write more code in that function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good shout, makes sense to me

inc/composer/class-command.php Outdated Show resolved Hide resolved
Copy link
Contributor

@roborourke roborourke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to make one small change to the connection type value in the sequel pro XML as it was complaining about missing SSH details but this is all working nicely. Good work @igmoweb 👍

@roborourke roborourke merged commit 61012be into master Feb 11, 2020
@roborourke roborourke deleted the 1-db-command branch February 11, 2020 10:35
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.

2 participants