-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[3.0] Bumped the minimum php version to 5.5 #12778
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
Conversation
@@ -16,7 +16,7 @@ | |||
} | |||
], | |||
"require": { | |||
"php": ">=5.3.3", | |||
"php": ">=5.5.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's the same as >=5.5
, isn't it?
Since there is no ~2.2.0
(line bellow), this is inconsistent and should be shorter as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TomasVotruba ~2.2.0
is a different version constraint to ~2.2
. They behave totally differently. Keeping the .0
in the all php version constraints seems consistent to me.
👍 |
@fabot I've just removed a load more old hacks. If you want those changes broken up into separate pull requests per component, give me a shout. |
Duplicate of #12522, maybe you can cherry pick my commit and put your additional work on top so we can keep only one PR, yours? |
@nicolas-grekas Sorry - I missed that pull. It looks like by changes are actually 100% different apart from the travis changes though. |
@fabpot Just rebased. |
Looking at the Travis output, some stuff look broken |
'TFoo' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
'CFoo' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
'Foo\\TBar' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
'Foo\\IBar' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
'Foo\\TFooBar' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
'Foo\\CBar' => __DIR__.'/Fixtures/php5.4/traits.php', | ||
)); | ||
} | ||
)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"," looks wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That's probably one of my errors. I'll go through this again later to check I haven't missed anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a problem IMHO. It's just an additional comma a the and of an array. The same comma as at line 104 of this file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comma is even required by the Symfony coding standards. It looks a bit weird in the diff. But when looking at the resulting file, everything seems to perfect here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh, we need trailing commas. I thought it was supposed to be a semicolon looking at the diff, but it's correct as a comma, yeh.
@GrahamCampbell can you please rebase/cherry-pick on my PR #12522 so I can close it? |
@@ -66,12 +66,6 @@ public function __construct($inputTimezone = null, $outputTimezone = null, $form | |||
|
|||
$this->generateFormat = $this->parseFormat = $format; | |||
|
|||
// The pipe in the parser pattern only works as of PHP 5.3.7 | |||
// See http://bugs.php.net/54316 | |||
$this->parseUsingPipe = null === $parseUsingPipe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be $this->parseUsingPipe = null === $parseUsingPipe ? true : $parseUsingPipe
or even better $this->parseUsingPipe = $parseUsingPipe
avec $parseUsingPipe = true
par defaut
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy-derusse your second suggestion would change the behavior by not accepting null
anymore (and actually changing the implicit support of null in this case because it is a falsy value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tought, the default null
value was a way for the developper to let symfony decide if the parameter should be true
or false
. Which only sets it to true
when the version of php is greater than 5.3.7
And given this is the last parameter of the list, I did not see why someone will explicitly pass null to this method.
BTW, We could keep this behaviours. It will just like weird in few year when someone will read
function foo($param = null) {
if ($param === null) $param = true;
}
Just rebased again, and fixed some issues. |
@@ -211,7 +185,7 @@ public function testSetSaveHandler54() | |||
/** | |||
* @expectedException \RuntimeException | |||
*/ | |||
public function testStartedOutside() | |||
public function testStarted() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. Forgot to fix this. :)
Please see #12778 @GrahamCampbell : I cherry-picked you work on top of mine (updating composer.json and rebasing) |
Moved to #12522. |
…amCampbell) This PR was merged into the 3.0-dev branch. Discussion ---------- [3.0] Update required PHP to 5.5.9 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13126, #12778 | License | MIT | Doc PR | - This PR upgrades composer.json files to php>=5.5.9 5.5.9 is the lowest version available on Travis and the version shipped with Ubuntu LTS 14.04. We can change the php version later when we'll agree on some other requirement, but currently, this is required to safely merge in master without having some users reporting issues because they have a `dev-master@dev` requirement. Commits ------- 35e0845 [3.0] Removed some old hacks fddcb86 [3.0] Update required PHP to 5.5.9
This pull request bumps the minimum php version of symfony 3.0 to php 5.5.
This also removes all the hacky workarounds for php 5.3 and 5.4.