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

More Symfony 5 compatibility fixes #1971

Merged
merged 3 commits into from
Dec 17, 2019
Merged

Conversation

aboks
Copy link
Contributor

@aboks aboks commented Dec 16, 2019

Q A
Bug fix? Yes
New feature? No
BC breaks? No
Deprecations? No
Fixed tickets N/A

I hate to bring the bad news here after your previous efforts to support Symfony 5, but there are still incompatibilities with symfony/process` 5.x in the Rsync- and SSH-client. This PR should fix them.

Do not forget to add notes about your changes to CHANGELOG.md

Easiest way to do it, by running next command:

php bin/changelog

@loevgaard
Copy link

Could this be merged and released? :)

@@ -43,7 +43,11 @@ public function call($hostname, $source, $destination, array $config = [])

$this->pop->command($hostname, $rsync);

$process = new Process($rsync);
if (method_exists('Symfony\Component\Process\Process', 'fromShellCommandline')) {
Copy link

Choose a reason for hiding this comment

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

Maybe replace with if (method_exists(Process::class, 'fromShellCommandline'))?

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 agree that would be slightly cleaner. For now I used the exact same fix that was already present in e.g. Deployer\Utility\ProcessRunner for consistency. The best long-term solution would probably be to have a utility function somewhere in the deployer codebase to contain this compatibility layer.


private function createProcess($command)
{
if (method_exists('Symfony\Component\Process\Process', 'fromShellCommandline')) {
Copy link

Choose a reason for hiding this comment

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

Maybe replace with if (method_exists(Process::class, 'fromShellCommandline'))?

@antonmedv antonmedv merged commit 5f32216 into deployphp:master Dec 17, 2019
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.

4 participants