-
-
Notifications
You must be signed in to change notification settings - Fork 328
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
Provide compatibility with PHP 7.2 #706
Conversation
hboomsma
commented
Mar 22, 2017
- Allow building phpnightly on travis
- Allow installing on PHP ^7.2.0-dev
- Fix type errors
@@ -56,7 +56,7 @@ public function __construct(ChainableTokenParserInterface $functionTokenParser, | |||
/** | |||
* @inheritdoc | |||
*/ | |||
public function withParser(ParserInterface $parser): self | |||
public function withParser(ParserInterface $parser): parent |
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 think this change should be reverted. The parent signature is public function withParser(ParserInterface $parser): static
so self
is acceptable besides being more flexible than parent
. I didn't know you could use parent
though so nice to learn :)
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.
When building with PHP 7.2(.0-dev) you get:
Fatal error: Declaration of Nelmio\Alice\FixtureBuilder\ExpressionLanguage\Parser\TokenParser\Chainable\TolerantFunctionTokenParser::withParser(Nelmio\Alice\FixtureBuilder\ExpressionLanguage\ParserInterface $parser):
Nelmio\Alice\FixtureBuilder\ExpressionLanguage\Parser\TokenParser\Chainable\TolerantFunctionTokenParser
must be compatible with
Nelmio\Alice\FixtureBuilder\ExpressionLanguage\Parser\TokenParser\Chainable\AbstractChainableParserAwareParser::withParser(Nelmio\Alice\FixtureBuilder\ExpressionLanguage\ParserInterface $parser): Nelmio\Alice\FixtureBuilder\ExpressionLanguage\Parser\TokenParser\Chainable\AbstractChainableParserAwareParser
in
/home/hboomsma/projects/alice/src/FixtureBuilder/ExpressionLanguage/Parser/TokenParser/Chainable/TolerantFunctionTokenParser.php on line 31
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 would remove the : self
in the AbstractChainableParserAwareParser
then
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.
Works for me 👍 , I'll update the request.
@@ -244,7 +244,7 @@ public function testThrowsAnExceptionIfFixtureClassDoesNotMatchObjectClass() | |||
|
|||
/** | |||
* @expectedException \Nelmio\Alice\Throwable\Exception\Generator\Instantiator\InstantiationException | |||
* @expectedExceptionMessage Instantiated fixture was expected to be an instance of "Nelmio\Alice\Entity\Instantiator\DummyWithFakeNamedConstructor". Got "Nelmio\Alice\Throwable\Exception\Generator\Instantiator\InstantiationExceptionFactory" instead. | |||
* @expectedExceptionMessage Instantiated fixture was expected to be an instance of "Nelmio\Alice\Entity\Instantiator\DummyWithFakeNamedConstructor". Got "null" instead. |
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 like a dumb copy/paste
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.
actually not, in PHP <7.2 get_class(null)
would return the current class. Since PHP 7.2 it throws a TypeError
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.
which makes perfect sense, I think I didn't read the message properly when I copy/pasted it there :)
composer.json
Outdated
@@ -20,7 +20,7 @@ | |||
], | |||
|
|||
"require": { | |||
"php": "7.0 - 7.1", | |||
"php": "7.0 - 7.2 ||7.2.*@dev", |
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.
Doesn't the --ignore-platform-reqs
option of Composer allows to get around that?
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.
Yes it sure does! I'll put that one in the .travis.yml
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.
Already there... 👍
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.
so this is not needed right? 😛
* Allow building phpnightly on travis * Allow installing on PHP ^7.2.0-dev * Fix type errors
Thanks :) |