Skip to content

Test against symfony versions #90

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

Merged
merged 27 commits into from
Nov 30, 2017
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,6 @@ cache:
directories:
- $HOME/.composer/cache/files

php:
- 5.4
- 5.5
- 5.6
- 7.0
- 7.1
- hhvm

env:
global:
- TEST_COMMAND="composer test"
Expand All @@ -27,11 +19,33 @@ branches:
matrix:
fast_finish: true
include:
# Minimum supported PHP and Symfony version
Copy link

Choose a reason for hiding this comment

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

Minimum supported PHP is confusing here

- php: 5.4
env: COMPOSER_FLAGS="--prefer-stable --prefer-lowest" COVERAGE=true TEST_COMMAND="composer test-ci"

env: DEPENDENCIES="minimun" COVERAGE=true TEST_COMMAND="composer test-ci"
Copy link
Contributor

Choose a reason for hiding this comment

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

minimum


# Test the latest stable release
- php: 7.1

# Test LTS versions
- php: 7.1
Copy link

Choose a reason for hiding this comment

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

Food for thought: test the LTS releases against the lowest common version they and this package supports. I'd say that people on LTS releases are less likely to always run on the latest PHP version, so testing against a lower version might be more comparable to real-world usage.

env: DEPENDENCIES="symfony/lts:v2"
Copy link
Contributor

Choose a reason for hiding this comment

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

clever use of the package, great discovery!

Choose a reason for hiding this comment

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

What if some bundle authors actively only want to support 2 Symfony versions at one time? What's what we're talking about in FOSUserBundle FriendsOfSymfony/FOSUserBundle#2639 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

This can still install a mixed set of 2.7 and 2.8 components. What about using symfony/symfony:v2.8.* which will force any symfony component to be in it's v2.8 version?

Choose a reason for hiding this comment

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

Sorry, I realize I was unclear! What I really mean was: in FOSUserBundle, they only want to support Symfony 3 and Symfony 4 (and then perhaps just do bug-fixes fro 2.7/2.8 branches).

In that situation, I guess they would just remove this one line and change the minimum dependencies matrix to 5.5 (from 5.4)?

Copy link

Choose a reason for hiding this comment

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

remove the Symfony 2 LTS job and bump your min requirement

Copy link

Choose a reason for hiding this comment

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

@weaverryan they in FOSUserBundle refers to only one maintainer of the bundle actually. I had not even seen this discussion yet (it was hidden in the middle of the thousand notifications I received during the hackday)

Copy link
Contributor

Choose a reason for hiding this comment

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

re requiring symfony/symfony: that hides problems if we miss depending on some symfony components. if we do that, we should require each component we use in the configured version instead. (which duplicates composer.json, so still not very nice)

Copy link
Contributor

Choose a reason for hiding this comment

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

We still have the "Test the latest stable release" job which does not alter the composer.json.

- php: 7.1
env: DEPENDENCIES="symfony/lts:v3"

# Latest beta release
Copy link

Choose a reason for hiding this comment

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

I suggest testing against dev rather than beta (especially if you allow them to fail):

  • if something breaks in an early Symfony 4.1 version, you may have 5 months to report it or adapt, instead of a few weeks/days
  • most of your other deps don't release beta at all, so you will not catch anything until it reaches other jobs too

- php: 7.1
env: DEPENDENCIES="beta"

allow_failures:
# Latest beta is allowed to fail.
Copy link

Choose a reason for hiding this comment

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

wrong comment

- php: 7.1
Copy link

Choose a reason for hiding this comment

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

no need to specify the PHP version here. It will avoid having to keep it in sync when changing the version used by DEPENDENCIES="dev"

env: DEPENDENCIES="beta"

Choose a reason for hiding this comment

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

I think the latest beta should not be allowed to fail. Probably we really should allow it to fail, but I'm afraid people will not "notice" the failure. I'd rather push them to be a bit more proactive by noticing it :)


before_install:
- if [[ $COVERAGE != true ]]; then phpenv config-rm xdebug.ini || true; fi
- if [ "$DEPENDENCIES" = "minimun" ]; then COMPOSER_FLAGS="--prefer-stable --prefer-lowest"; fi;
- if [ "$DEPENDENCIES" = "beta" ]; then composer config minimum-stability beta; fi;
- if [[ $DEPENDENCIES == *"/"* ]]; then composer require --no-update $DEPENDENCIES; fi;
Copy link
Member

Choose a reason for hiding this comment

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

and then just test for $DEPENDENCIES not being empty here


install:
# To be removed when this issue will be resolved: https://github.com/composer/composer/issues/5355
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"php-http/httplug": "^1.1",
"php-http/message-factory": "^1.0",
"php-http/message": "^1.6",
"symfony/options-resolver": "^2.6 || ^3.0 || ^4.0"
"symfony/options-resolver": "^2.6 || ^3.0"
},
"require-dev": {
"phpspec/phpspec": "^2.4",
Expand Down
4 changes: 2 additions & 2 deletions src/Plugin/AddHostPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ final class AddHostPlugin implements Plugin
*/
public function __construct(UriInterface $host, array $config = [])
{
if ($host->getHost() === '') {
if ('' === $host->getHost()) {
throw new \LogicException('Host can not be empty');
}

Expand All @@ -51,7 +51,7 @@ public function __construct(UriInterface $host, array $config = [])
*/
public function handleRequest(RequestInterface $request, callable $next, callable $first)
{
if ($this->replace || $request->getUri()->getHost() === '') {
if ($this->replace || '' === $request->getUri()->getHost()) {
$uri = $request->getUri()
->withHost($this->host->getHost())
->withScheme($this->host->getScheme())
Expand Down
4 changes: 2 additions & 2 deletions src/Plugin/AddPathPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,11 @@ final class AddPathPlugin implements Plugin
*/
public function __construct(UriInterface $uri)
{
if ($uri->getPath() === '') {
if ('' === $uri->getPath()) {
throw new \LogicException('URI path cannot be empty');
}

if (substr($uri->getPath(), -1) === '/') {
if ('/' === substr($uri->getPath(), -1)) {
throw new \LogicException('URI path cannot end with a slash.');
}

Expand Down
2 changes: 1 addition & 1 deletion src/Plugin/ContentTypePlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ private function isJson($stream)

json_decode($stream->getContents());

return json_last_error() == JSON_ERROR_NONE;
return JSON_ERROR_NONE == json_last_error();
Copy link

Choose a reason for hiding this comment

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

strict comparison ?

}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/Plugin/CookiePlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
continue;
}

if ($cookie->isSecure() && ($request->getUri()->getScheme() !== 'https')) {
if ($cookie->isSecure() && ('https' !== $request->getUri()->getScheme())) {
continue;
}

Expand Down
6 changes: 3 additions & 3 deletions src/Plugin/DecoderPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,15 +123,15 @@ private function decodeOnEncodingHeader($headerName, ResponseInterface $response
*/
private function decorateStream($encoding, StreamInterface $stream)
{
if (strtolower($encoding) == 'chunked') {
if ('chunked' == strtolower($encoding)) {
return new Encoding\DechunkStream($stream);
}

if (strtolower($encoding) == 'deflate') {
if ('deflate' == strtolower($encoding)) {
return new Encoding\DecompressStream($stream);
}

if (strtolower($encoding) == 'gzip') {
if ('gzip' == strtolower($encoding)) {
return new Encoding\GzipDecodeStream($stream);
}

Expand Down