Skip to content
This repository was archived by the owner on Jul 29, 2024. It is now read-only.

Add compatibility with symfony 3.* and 4.*#4

Merged
fabpot merged 1 commit into
stackphp:masterfrom
trust-abe:master
Feb 20, 2018
Merged

Add compatibility with symfony 3.* and 4.*#4
fabpot merged 1 commit into
stackphp:masterfrom
trust-abe:master

Conversation

@pascal-hofmann

Copy link
Copy Markdown
Contributor

Would be great to have this merged soon.

@fabpot

fabpot commented Feb 9, 2018

Copy link
Copy Markdown
Contributor

We need to be sure that this works on both 3.x and 4.0, so I would also change the .travis.yml file to be sure that we are running tests for at least 3.4 and 4.0. You can have a look at twigphp/twig for an example of how to make it work.

@pascal-hofmann pascal-hofmann changed the title Add compability with symfony 3.* and 4.* Add compatibility with symfony 3.* and 4.* Feb 12, 2018
@pascal-hofmann

Copy link
Copy Markdown
Contributor Author

@fabpot: Thanks for your feedback, I'll change .travis.ymlaccordingly.

I fail to see where multiple symfony versions are being used by travis in twigphp/twig, and instead will follow what's described here: https://symfony.com/doc/current/setup/bundles.html#update-the-travis-ci-configuration

Somehow the travis builds for this PR don't work. Travis just says "Abuse detected". I've reached out to the support to get this fixed.

@fabpot

fabpot commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

Indeed, Silex is a better example: https://github.com/silexphp/Silex/blob/master/.travis.yml

@pascal-hofmann

Copy link
Copy Markdown
Contributor Author

Closing and re-opening to trigger travis rebuild.

@pascal-hofmann pascal-hofmann force-pushed the master branch 5 times, most recently from 814465f to e4bb75d Compare February 16, 2018 08:13
@pascal-hofmann

Copy link
Copy Markdown
Contributor Author

I finally managed to get the travis "Abuse detected" problem solved and all travis builds passing.

@fabpot Is the PR ok like this?

@fabpot fabpot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me. I've made some minor comments.

Comment thread .travis.yml Outdated
- $HOME/.composer/cache/files

before_install:
# Circumvent a bug in the travis HHVM image - ships with incompatible PHPUnit.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would remove everything related to HHVM as we (Symfony) do not support HHVM anymore anyway.

Comment thread composer.json Outdated
"symfony/http-foundation": "~2.1",
"symfony/http-kernel": "~2.1"
"symfony/http-foundation": "~2.1|~3.0|~4.0",
"symfony/http-kernel": "~2.1|~3.0|~4.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about bumping ~2.1 to ~2.7? 2.1 is very old now, so I don't think we need to support it.

Comment thread tests/bootstrap.php Outdated
@@ -0,0 +1,6 @@
<?php
if (class_exists('PHPUnit_Framework_TestCase') === false && class_exists('PHPUnit\Framework\TestCase') === true) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Small thing, but can you add a blank line just after <?php? And add a new line at the end of the file?

Comment thread tests/bootstrap.php Outdated
@@ -0,0 +1,6 @@
<?php
if (class_exists('PHPUnit_Framework_TestCase') === false && class_exists('PHPUnit\Framework\TestCase') === true) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could be simplified to if (!class_exists('PHPUnit_Framework_TestCase') && class_exists('PHPUnit\Framework\TestCase')) {

@pascal-hofmann

Copy link
Copy Markdown
Contributor Author

@fabpot I changed the PR according to your suggestions.

@fabpot fabpot merged commit e5e5fcc into stackphp:master Feb 20, 2018
@fabpot fabpot mentioned this pull request Feb 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants