Make use of PHP 7.4 syntax #2470
Conversation
| @@ -21,7 +23,7 @@ public static function documentNotFound(string $className, $identifier): self | |||
| return new self(sprintf( | |||
| 'The "%s" document with identifier %s could not be found.', | |||
| $className, | |||
| json_encode($identifier) | |||
| json_encode($identifier, JSON_THROW_ON_ERROR) | |||
There was a problem hiding this comment.
This is something I'm not really sure if we should add here, as it can be considered a breaking change.
There was a problem hiding this comment.
You could catch the JsonException first if it occurs and concatenate it to the end of the message / set it as $previous exception. Then it should no longer be a BC break, as it would no longer change the type of an exception that might come out of it, while providing that little bit of extra information.
There was a problem hiding this comment.
$previous is the way to go IMO 👍
| @@ -25,6 +24,8 @@ class MemoryUsageTest extends BaseTest | |||
| { | |||
| /** | |||
| * Output for jwage "Memory increased by 14.09 kb" | |||
| * | |||
| * @doesNotPerformAssertions | |||
There was a problem hiding this comment.
Since you've found this test: I wonder if it's needed :D I mean nobody looks at tests that did not fail explicitly
There was a problem hiding this comment.
And this one won't be failing with an exception either
There was a problem hiding this comment.
Those tests are excluded from the test run in phpunit.xml.dist:
<groups>
<exclude>
<group>performance</group>
</exclude>
</groups>I think their main purpose was to quickly test the performance and memory usage before adding phpbench (the test is dated 2014).
There was a problem hiding this comment.
I think I we could keep these tests for now, for historical reasons and perhaps delete them in a separate PR if you don't mind. I'd like to keep the diff ass small as possible here.
| @@ -20,6 +20,8 @@ class HydrationPerformanceTest extends BaseTest | |||
| { | |||
| /** | |||
| * [jwage: 10000 objects in ~6 seconds] | |||
| * | |||
| * @doesNotPerformAssertions | |||
There was a problem hiding this comment.
Same here. Additionally phpbench is in use for Performance tests nowadays
|
Thanks @IonBazan! |
Summary
Migrates some of the old syntax to PHP 7.4 where applicable:
array_key_firstandis_iterableMigrates usage of PHPUnit API:
@doesNotPerformAssertionswhere no assertions are performed$this->createMock()shorthandLinked to #2464