Fix PHP 7.4 support #203

nicolas-grekas · 2019-07-18T15:05:16Z

Symfony is going to need this to have green CI on PHP 7.4 :)

nicolas-grekas · 2019-07-18T16:44:05Z

Ready.

egulias

Hi @nicolas-grekas , thanks for the update.
I have left some comments for you.

egulias · 2019-07-18T17:41:39Z

phpunit.xml.dist

 </filter>
+
+<listeners>
+  <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />


Would you mind explaining why is this listener needed?

because PHP 7.4 introduces new deprecations that affect phpunit, and the listener allows silencing them

egulias · 2019-07-18T20:12:22Z

EmailValidator/EmailLexer.php

@@ -77,10 +77,16 @@ class EmailLexer extends AbstractLexer

    protected $previous;

+    public function __construct()
+    {
+        $this->reset();


Calling reset on construction sounds a bit odd, reseting something 'new'. For what I've seen in the rest of the changes, this is needed mainly to do the equality evaluation (e.g https://github.com/egulias/EmailValidator/pull/203/files#diff-c759db088d5c729c1f537d85c2ea93d0R187).
I would suggest something like init() :

$this->hasInvalidTokens = false; $this->previous = $this->token = ['type' => null, 'value' => '', 'position' => 0];

and call that from both __contruct & reset.

the code is full of checks with $this->lexer->token['type'], when $this->lexer->token is null actually. PHP 7.4 is now refusing to access keys on null values, so that the affected properties must be set to an array representing the null-state. reset does it.

Thanks for the explanation, understood. My suggestion is moving both lines to a new method so they can call it having a more expresive meaning and avoiding coupling too much construction with re-use (reseting to analyse another string).

__construct() { $this->init(); } reset() { ... $this->init(); }

egulias · 2019-07-18T20:14:42Z

EmailValidator/EmailLexer.php

    public function reset()
    {
        $this->hasInvalidTokens = false;
        parent::reset();
+        $this->previous = $this->token = ['type' => null, 'value' => '', 'position' => 0];


I'd suggest to use EmailLexer::C_NUL for type instead.

this wouldn't be the same - we don't want to this be confused with the NUL char.

Agree. Should a new token be added?

I don't think so, null is really the best "const" for this to me.

egulias · 2019-07-18T20:18:37Z

Tests/EmailValidator/Validation/MultipleValidationWithAndTest.php

@@ -59,7 +60,7 @@ public function testAccumulatesWarnings()
        ];
        $expectedResult = array_merge($warnings1, $warnings2);

-        $lexer = $this->getMockBuilder("Egulias\\EmailValidator\\EmailLexer")->getMock();
+        $lexer = new EmailLexer();


egulias · 2019-07-18T20:48:40Z

EmailValidator/EmailLexer.php

@@ -122,8 +128,10 @@ public function getPrevious()
    public function moveNext()
    {
        $this->previous = $this->token;
+        $hasNext = parent::moveNext();
+        $this->token = $this->token ?: ['type' => null, 'value' => '', 'position' => 0];


Is this line because of the lexer reaching the last token and being set to null?
I'd suggest to express that case with something like

public function moveNext() { $this->previous = $this->token; $hasNext = parent::moveNext(); /* new stuff */ $this->setEndToken($hasNext); return $hasNext; } private function setFinalToken($nextToken) { $this->token = $this->token ?: ['type' => C_NUL, 'value' => '', 'position' => 0]; }

you're right about the reason: $this->token cannot be null.
not sure I get what the additional method would provide thought :)

To understood this line I had to review carefully the full PR and take my time, even with my knowledge of the library.
The additional method adds clarity on the reason behind.

I pushed a variant, does it solve the concern?

I think it can work.
Thanks!

SamMousa · 2019-07-22T15:21:15Z

Is this really the way to go about this @nicolas-grekas ??
It seems weird to me for 3rd party libraries to be required to include some dev package from symfony just so Symfony can have their tests pass...

nicolas-grekas · 2019-07-22T15:25:19Z

@SamMousa you must be confused, this added dev dep is not to make Symfony's tests pass, but about making this very package tests pass.

SamMousa · 2019-07-22T15:26:43Z

I have just removed it locally, the test listener and the dependency, and tests still pass.

Note: that's on PHUnit 6.

nicolas-grekas · 2019-07-22T15:27:54Z

Try again with PHP7.4 + ~~latest~~ phpunit 6 (as required by composer.json).

SamMousa · 2019-07-22T15:38:30Z

root@ba1517249f5f:/project# php --version
PHP 7.4.0alpha3 (cli) (built: Jul 12 2019 21:26:03) ( NTS )
Copyright (c) The PHP Group
Zend Engine v3.4.0-dev, Copyright (c) Zend Technologies

PHPUnit 6.5.14 by Sebastian Bergmann and contributors.

...............................................................  63 / 253 ( 24%)
............................................................... 126 / 253 ( 49%)
............................................................... 189 / 253 ( 74%)
...........................................................SSSS 252 / 253 ( 99%)
.                                                               253 / 253 (100%)

Time: 537 ms, Memory: 6.00MB

OK, but incomplete, skipped, or risky tests!
Tests: 253, Assertions: 351, Skipped: 4.

SamMousa · 2019-07-22T15:39:59Z

Works for me: ```bash root@ba1517249f5f:/project# php --version PHP 7.4.0alpha3 (cli) (built: Jul 12 2019 21:26:03) ( NTS ) Copyright (c) The PHP Group Zend Engine v3.4.0-dev, Copyright (c) Zend Technologies PHPUnit 6.5.14 by Sebastian Bergmann and contributors. ............................................................... 63 / 253 ( 24%) ............................................................... 126 / 253 ( 49%) ............................................................... 189 / 253 ( 74%) ...........................................................SSSS 252 / 253 ( 99%) . 253 / 253 (100%) Time: 537 ms, Memory: 6.00MB OK, but incomplete, skipped, or risky tests! Tests: 253, Assertions: 351, Skipped: 4. ```

…

On Mon, Jul 22, 2019 at 5:27 PM Nicolas Grekas ***@***.***> wrote: Try again with PHP7.4 + latest phpunit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#203?email_source=notifications&email_token=AAEFRTM3SLOTEO64T7MYOILQAXGXZA5CNFSM4IE4SWTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2QJO6A#issuecomment-513841016>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEFRTPHZMIPFMQUDOIOHT3QAXGXZANCNFSM4IE4SWTA> .

nicolas-grekas · 2019-07-22T15:45:39Z

Oh that's good news, maybe related to the removed mocks? I suppose I patched them after adding the bridge and didn't try removing it after. PR welcome I suppose.

mlocati · 2019-07-30T14:50:50Z

@egulias would you accept a similar pull request for version 1?

egulias · 2019-07-30T16:04:03Z

Hi @mlocati
Yes, happy do it. It is still in use from what Packagist says.

mlocati · 2019-07-31T06:40:04Z

Done: see #206

nicolas-grekas mentioned this pull request Jul 18, 2019

Allow Travis CI to build on PHP 7.4 symfony/symfony#32289

Merged

phansys approved these changes Jul 18, 2019

View reviewed changes

egulias requested changes Jul 18, 2019

View reviewed changes

Fix PHP 7.4 support

b9b2cf9

egulias approved these changes Jul 19, 2019

View reviewed changes

egulias merged commit a6c8d71 into egulias:master Jul 19, 2019

nicolas-grekas deleted the php74 branch July 22, 2019 15:24

SamMousa mentioned this pull request Jul 23, 2019

Removed bridge dependency and listener config #204

Closed

mlocati mentioned this pull request Jul 31, 2019

Fix PHP 7.4 support for 1.x #206

Merged

This was referenced Sep 2, 2019

E_NOTICE Array offset Laravel 5.8 #212

Closed

[6.x] PHP 7.4 support laravel/framework#29482

Merged

jacobsmith mentioned this pull request Jan 10, 2020

Issues with PHP 7.4 BookStackApp/BookStack#1834

Closed

sam-suresh mentioned this pull request May 14, 2020

Email not being sent freescout-help-desk/freescout#468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix PHP 7.4 support #203

Fix PHP 7.4 support #203

nicolas-grekas commented Jul 18, 2019

nicolas-grekas commented Jul 18, 2019

egulias left a comment

egulias Jul 18, 2019

nicolas-grekas Jul 18, 2019 •

edited

Loading

egulias Jul 18, 2019

nicolas-grekas Jul 18, 2019 •

edited

Loading

egulias Jul 18, 2019 •

edited

Loading

egulias Jul 18, 2019

nicolas-grekas Jul 18, 2019

egulias Jul 18, 2019

nicolas-grekas Jul 19, 2019

egulias Jul 18, 2019

egulias Jul 18, 2019

nicolas-grekas Jul 18, 2019

egulias Jul 18, 2019

nicolas-grekas Jul 19, 2019

egulias Jul 19, 2019

SamMousa commented Jul 22, 2019

nicolas-grekas commented Jul 22, 2019 •

edited

Loading

SamMousa commented Jul 22, 2019 •

edited

Loading

nicolas-grekas commented Jul 22, 2019 •

edited

Loading

SamMousa commented Jul 22, 2019

SamMousa commented Jul 22, 2019 via email

nicolas-grekas commented Jul 22, 2019 via email •

edited

Loading

mlocati commented Jul 30, 2019

egulias commented Jul 30, 2019

mlocati commented Jul 31, 2019

Fix PHP 7.4 support #203

Fix PHP 7.4 support #203

Conversation

nicolas-grekas commented Jul 18, 2019

nicolas-grekas commented Jul 18, 2019

egulias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

egulias Jul 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamMousa commented Jul 22, 2019

nicolas-grekas commented Jul 22, 2019 • edited Loading

SamMousa commented Jul 22, 2019 • edited Loading

nicolas-grekas commented Jul 22, 2019 • edited Loading

SamMousa commented Jul 22, 2019

SamMousa commented Jul 22, 2019 via email

nicolas-grekas commented Jul 22, 2019 via email • edited Loading

mlocati commented Jul 30, 2019

egulias commented Jul 30, 2019

mlocati commented Jul 31, 2019

nicolas-grekas Jul 18, 2019 •

edited

Loading

nicolas-grekas Jul 18, 2019 •

edited

Loading

egulias Jul 18, 2019 •

edited

Loading

nicolas-grekas commented Jul 22, 2019 •

edited

Loading

SamMousa commented Jul 22, 2019 •

edited

Loading

nicolas-grekas commented Jul 22, 2019 •

edited

Loading

nicolas-grekas commented Jul 22, 2019 via email •

edited

Loading