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.

Sign up for GitHub

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

Jump to bottom

Add support for PHP 8 #1064

Merged
merged 14 commits into from
Jan 13, 2021
Merged

Add support for PHP 8 #1064

merged 14 commits into from
Jan 13, 2021

Conversation

Yozhef
Copy link
Contributor

@Yozhef Yozhef commented Dec 9, 2020

No description provided.

@Yozhef Yozhef changed the title Allow php 8.0 Add support for PHP 8 Dec 9, 2020
@Yozhef
Copy link
Contributor Author

Yozhef commented Dec 9, 2020 •
edited

Error in build PHP 8.0:
theofidry/composer-inheritance-plugin#8

Blocked by:
wikimedia/composer-merge-plugin#189

@alexislefebvre alexislefebvre mentioned this pull request Dec 10, 2020
Merged
@simPod
Copy link
Contributor

simPod commented Jan 7, 2021 •
edited

alice/src/Throwable/Exception/FixtureBuilder/Denormalizer/DenormalizerExceptionFactory.php

Line 33 in 25e127f

public static function createForUnparsableValue(string $value, int $code = 0, Throwable $previous): UnexpectedValueException
needs to add = null to throwable so required param is not after optional one.

Edit: #1068

@theofidry
Copy link
Member

Thanks for the PR @Yozhef, but I think it will require a migration to GitHub Actions as Travis CI kinda gave up on OSS support :(

@theofidry theofidry mentioned this pull request Jan 7, 2021
Closed
@Yozhef
Copy link
Contributor Author

Yozhef commented Jan 7, 2021 •
edited

@theofidry ok I will do it
#1069

@Yozhef Yozhef force-pushed the php8 branch 4 times, most recently from fd357d1 to 2af9929 Compare January 8, 2021 19:24
@Yozhef
Copy link
Contributor Author

Yozhef commented Jan 8, 2021

@theofidry
A little need for your consultation:
Changing the function will cause what needs to be changed or updated for this structure?

File -> SimpleReferenceTokenParserTest.php:56 -> SimpleReferenceTokenParser -> FixtureReferenceValue

In PHP 7.4
var_dump(substr('', 1)); // false

In PHP 8.0
var_dump(substr('', 1)); // ""

theofidry
theofidry reviewed Jan 8, 2021
View reviewed changes
tests/FixtureBuilder/ExpressionLanguage/TokenTypeTest.php Outdated
@@ -48,7 +48,7 @@ public function testStaticValues(): void

$reflProp = $reflClass->getProperty('values');
$reflProp->setAccessible(true);
$values = $reflProp->getValue(TokenType::class);
$values = $reflProp->getValue((object) TokenType::class);
Copy link
Member

Choose a reason for hiding this comment

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

??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If run in PHP 8

1) Nelmio\Alice\FixtureBuilder\ExpressionLanguage\TokenTypeTest::testStaticValues
TypeError: ReflectionProperty::getValue(): Argument #1 ($object) must be of type ?object, string given

/home/runner/work/alice/alice/tests/FixtureBuilder/ExpressionLanguage/TokenTypeTest.php:50

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the reflection API changed for a static value, but the change does not look correct, (object) TokenType::class === stdClass{ #scalar: TokenType::class } which is most likely not what we want

@theofidry
Copy link
Member

@Yozhef sorry I'm not following, what's the issue?

@Yozhef Yozhef force-pushed the php8 branch 2 times, most recently from b5f0b42 to 145d319 Compare January 12, 2021 16:24
@Yozhef Yozhef requested a review from theofidry January 12, 2021 16:47
@alexislefebvre
Copy link
Contributor

@Yozhef Congrats on making the CI green. 😃

theofidry
theofidry approved these changes Jan 12, 2021
View reviewed changes
Copy link
Member

@theofidry theofidry left a comment

Choose a reason for hiding this comment

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

👍 looks good to me overall aside a few notes; thanks a lot for the work

...ixtureBuilder/ExpressionLanguage/Parser/TokenParser/Chainable/SimpleReferenceTokenParser.php Outdated
@@ -41,7 +41,9 @@ public function parse(Token $token): FixtureReferenceValue
$value = $token->getValue();

try {
return new FixtureReferenceValue(substr($value, 1));
$elements = empty($value) ? false : substr($value, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? IIRC this parser can only parse SIMPLE_REFERENCE_TYPE tokens which have the form @.+, i.e. $value is a non-empty string here and starts with @. Anything else IMO should go with the exception.

Looking at it again though it might be clearer to throw the exception if $value is not a string and $elements an empty string rather than relying on an exception to be thrown and catch it:

$value = $token->getValue();

if (!is_string($value) || '' === $value) {
    throw ExpressionLanguageExceptionFactory::createForUnparsableToken($token);
}

return new FixtureReferenceValue(substr($value, 1));

src/FixtureBuilder/ExpressionLanguage/Parser/TokenParser/Chainable/VariableTokenParser.php Outdated
@@ -45,7 +45,9 @@ public function canParse(Token $token): bool
*/
public function parse(Token $token)
{
$variable = substr($token->getValue(), 1);
$variable = empty($token->getValue())
Copy link
Member

Choose a reason for hiding this comment

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

likewise here; also general note empty() should be avoided at all cost because is unreliable: empty('0') = true

...ixtureBuilder/ExpressionLanguage/Parser/TokenParser/Chainable/StringArrayTokenParserTest.php
static::fail('Expected exception to be thrown.');

if (PHP_VERSION_ID < 80000) {
static::fail('Expected exception to be thrown.');
Copy link
Member

Choose a reason for hiding this comment

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

why is it no longer the case here in PHP8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because PHP8.0 function substr return only string -therefore, the parameter error typehint will never fall.

@Yozhef Yozhef requested a review from theofidry January 13, 2021 09:49
@theofidry theofidry merged commit c019cf4 into nelmio:master Jan 13, 2021
@theofidry
Copy link
Member

👌

@alexislefebvre
Copy link
Contributor

@theofidry

Thanks for the PR @Yozhef, but I think it will require a migration to GitHub Actions as Travis CI kinda gave up on OSS support :(

Did they added some limitations for open source? I found this article and it's not very clear: https://devclass.com/2020/11/25/travis-ci-open-source-engagement/

@theofidry
Copy link
Member

they change of ownership and the direction is not clear. In the short run it's effectively very limited support and it's unlikely to change. They claim to still be able to offer support for popular OSS projects but I heard they won't decide anything there before another 2-3 months.

TL:DR; Travis is indeed dead for OSS

@alexislefebvre
Copy link
Contributor

Ok, thanks for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexislefebvre alexislefebvre alexislefebvre approved these changes

@theofidry theofidry Awaiting requested review from theofidry

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Yozhef @simPod @theofidry @alexislefebvre