8000 symfony backward compatibility support by mente · Pull Request #69 · pact-foundation/pact-php · GitHub
[go: up one dir, main page]
Skip to content

symfony backward compatibility support #69

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 8, 2018
Merged

symfony backward compatibility support #69

merged 4 commits into from
May 8, 2018

Conversation

@mente
Copy link
Contributor
@mente mente commented May 2, 2018

Depends on #68

In the end it was about replacing Process with ProcessBuilder. Other than that works smoothly.

WIP until #68 is merged and I have settled out proper travis setup for symfony 2.x/3.x/4.x components

@mente mente force-pushed the feature-symfony-2 branch 3 times, most recently from 05f6c1b to de9c418 Compare May 5, 2018 03:08
@mente
Copy link
Contributor Author
mente commented May 6, 2018 •
edited
Loading

I didn't give up on it. Fallen into "works on my machine" syndrome. Trying with docker image, no luck so far :-/.

Maybe any hints how to reproduce it locally? As I said it works on my Mac. Trying docker image with ubuntu

@samuelnogueira
Copy link
Contributor
samuelnogueira commented May 7, 2018

By using \Symfony\Component\Process\ProcessBuilder, exec prefix isn't added to the command, which apparently troubles symfony process (https://symfony.com/doc/current/components/process.html#process-signals).
This change should fix it:

-         $this->process = ProcessBuilder::create(\array_merge([$scripts->getMockService()], $this->getArguments()))
+         $this->process = ProcessBuilder::create(\array_merge(['exec', $scripts->getMockService()], $this->getArguments()))

@mente
Copy link
Contributor 10BC0 Author
mente commented May 7, 2018

Thanks, will give it a try!

I've checked command line from master branch and from my branch and it all look the same. Aso somehow symfony/process 3.3.9 is installed, whilst latest stable should be somewhere 4.0.x where ProcessBuilder is already removed and I will have to find another option :|

@mente
Copy link
Contributor Author
mente commented May 7, 2018

So I've found now a root cause: symfony/symfony#21474

when a simple command should be run, Process now accepts an array of arguments (the "binary" being the first arg). Making this the responsibility of Process (instead of ProcessBuilder) gives two benefits:
escape becomes an internal detail that doesn't leak - thus can't be misused (see here)
since we know we're running a single command, we can prefix it automatically by "exec" - thus fixing a long standing issue with signaling

Thus said it complicates backwards compatibility :-/

@mente mente force-pushed the feature-symfony-2 branch 4 times, most recently from ff5a6da to 4b38cfe Compare May 7, 2018 20:56
mente added 3 commits May 7, 2018 23:03
@mente
use phar distribution of php-cs-fixer
c7d68de 
Another step towards symfony 2.x support. `php-cs-fixer` has dependencies on symfony 3.x . By using phar instead dependencies are not being leaked to the overall library. This PR includes [tm/tooly-composer-script](https://github.com/tommy-muehle/tooly-composer-script) which actually just installs the same phar in the end of the composer command.

PS It looks like `composer fix` haven't been run for a while, but I leave it up to maintainers to define/revisit CS rules and CI pipeline for it
@mente
replace multiple version constraints with single lower and upper bound
cf8df38 
Limit upper bound to latest known dev version of symfony which is known so far - 4.1. It doesn't mean that newer versions won't be supported, but just to keep yourself [on the safe side](https://getcomposer.org/doc/faqs/why-are-unbound-version-constraints-a-bad-idea.md)
@mente
UR wrap symfony 2.x hack for running process in separate class
7dbbc26
@mente mente force-pushed the feature-symfony-2 branch from 4b38cfe to 7dbbc26 Compare May 7, 2018 21:04
@mente
Copy link
Contributor Author
mente commented May 7, 2018

I've isolated symfony 2.x hack and compatibility in ProcessRunner class. Rebased latest master to both PRs. Appveyor is failing (same for #68 ) on installing php step, which is before tests.

What do you think about current approach?

@mattermack
Copy link
Contributor
mattermack commented May 7, 2018

Re: AppVeyor

Chocolatey is down: https://chocolatey.org/packages/php

Chocolatey Gallery Chocolatey.org is currently offline We will be back Shortly

mattermack
mattermack reviewed May 7, 2018
View reviewed changes
src/PhpPact/Standalone/Runner/ProcessRunner.php Outdated
namespace PhpPact\Standalone\Runner;

use Symfony\Component\Process\Process;
use Symfony\Component\Process\ProcessBuilder;
Copy link
Contributor
@mattermack mattermack May 7, 2018

Choose a reason for hiding this comment

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

I thought ProcessBuilder was removed in later version of Symphony. Double checking here ...

Copy link
Contributor Author
@mente mente May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It is removed since 4.0. But using namespace doesn't trigger any error.

Another problem is that despite >=2.8 <4.1 version constraint for symfony/process composer still takes 3.3. And composer why doesn't explain why. Same in master branch but probably there are still php-cs-fixer dependencies

Copy link
Contributor
@mattermack mattermack May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mente,
I can see this returning an error or warning in a later version of PHP. What about using fully qualified names instead of use a namespace?

mattermack
mattermack reviewed May 8, 2018
View reviewed changes
src/PhpPact/Standalone/Runner/ProcessRunner.php
}
}
if ($useProcess) {
$process = new Process(\array_merge([$command], $arguments));
Copy link
Contributor
@mattermack mattermack May 8, 2018

Choose a reason for hiding this comment

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

$process = new \Symfony\Component\Process(\array_merge([$command], $arguments));

$pb = new \Symfony\Component\Process\ProcessBuilder(\array_merge([$command], $arguments));

Copy link
Contributor Author
@mente mente May 8, 2018

Choose a reason for hiding this comment

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

i understand the reason behind ProcessBuilder FQCN, but why Process?

PR updated!

@mente mente force-pushed the feature-symfony-2 branch from 12cd8d9 to 3831084 Compare May 8, 2018 20:10
@mente mente changed the title WIP symfony backward compatibility support symfony backward compatibility support May 8, 2018
@mente
Copy link
Contributor Author
mente commented May 8, 2018 •
edited
Loading

Another problem is that despite >=2.8 <4.1 version constraint for symfony/process composer still takes 3.3. And composer why doesn't explain why. Same in master branch but probably there are still php-cs-fixer dependencies

So it was this piece of composer.json in the end:

    "config": {
        "platform": {
            "php": "7.0"
        }
    }

And symfony/process 4.x requires php ^7.1.3. Commit message doesn't say too much unfortunately. Will prepare another PR to solve this issue (if you find it as an issue)

@mente mente mentioned this pull request May 8, 2018
Merged
@mattermack mattermack merged commit 9d6248d into pact-foundation:master May 8, 2018
@mattermack
Copy link
Contributor
mattermack commented May 8, 2018

I think you got it!

@mente
Copy link
Contributor Author
mente commented May 8, 2018

Woohoo, thanks! Now back to upgrading our apps from symfony 2.x to somewhere more relevant :D

@mente mente deleted the feature-symfony-2 branch May 8, 2018 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattermack mattermack mattermack left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mente @samuelnogueira @mattermack
0