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

Symfony 4 support #123

Merged
merged 8 commits into from
Dec 4, 2017
Merged

Symfony 4 support #123

merged 8 commits into from
Dec 4, 2017

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Nov 24, 2017

Replaces #121 and #122

@sroze
Copy link
Contributor Author

sroze commented Nov 24, 2017

Not sure what's the failure about but that doesn't look like related 🤔

@dunglas dunglas force-pushed the symfony-4-support branch 2 times, most recently from 5330d8f to 5e5d089 Compare November 24, 2017 19:13
@sroze
Copy link
Contributor Author

sroze commented Nov 27, 2017

@stof failing tests seems to be unrelated... need to dig into another PR.

@sroze
Copy link
Contributor Author

sroze commented Nov 27, 2017

This failing tests are normal because they related to the formaction being added in Symfony 3.3 (and added to the test suite in minkphp/driver-testsuite#6) ...

@sroze
Copy link
Contributor Author

sroze commented Nov 27, 2017

Or... we just merge this PR 💃
minkphp/driver-testsuite#17

composer.json Outdated
},

"require-dev": {
"mink/driver-testsuite": "dev-master",
"symfony/http-kernel": "~2.3|~3.0"
"symfony/http-kernel": "~2.3|~3.0|~4.0",
"phpunit/phpunit": "^4.8.35|^5.7"
Copy link
Member

Choose a reason for hiding this comment

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

this requirement does not make sense if you use vendor/bin/simple-phpunit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Removed.

@sroze
Copy link
Contributor Author

sroze commented Nov 27, 2017

Added the skip as you recommended Christophe... it feels very much hack. But I don't see how to detect any other way that the DomCrawler is the right version.

@sroze
Copy link
Contributor Author

sroze commented Nov 27, 2017

All green 🎉

@ruudk
Copy link
Contributor

ruudk commented Nov 30, 2017

Any news on this :D ? This is blocking me from upgrading to SF 4.0 🙏

@sroze
Copy link
Contributor Author

sroze commented Nov 30, 2017

That's in @stof's or @everzet's hand to merge I guess :)

'testHtml5FormAction',
'testHtml5FormMethod',
))
&& !method_exists('Symfony\Component\DomCrawler\Tests\FormTest', 'testGetMethodWithOverride')
Copy link
Member

Choose a reason for hiding this comment

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

Can't we find a method detecting relying on public classes instead ? test cases can be refactored in any patch release as they are purely internal.

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 know but I looked at finding another way and I can't, it didn't change :/

Copy link

Choose a reason for hiding this comment

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

I can't see any significant API changes for Browserkit 3.3 that we can latch onto

An alternative would be to use muglug/PackageVersions (or the upstream ocramius version if the PHP versions work out) in dev to explicitly detect the installed versions

Choose a reason for hiding this comment

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

Or parse composer info in the travis bash, and set an env var etc. to be detected in the script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, seems much more complex options. The only sensible alternative (to me) would be a DISABLE_ACTION_ATTRIBUTES_TEST environment variable that we simply set in Travis for the "old" builds.

@everzet everzet merged commit 7c45c64 into minkphp:master Dec 4, 2017
@sroze sroze deleted the symfony-4-support branch December 4, 2017 14:58
@aik099 aik099 mentioned this pull request May 1, 2018
@stof stof mentioned this pull request Dec 9, 2018
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

7 participants