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.

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

Fix issue 414, add tests + add php8 to travis (but not composer) #415

Merged
merged 5 commits into from
Aug 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
/build
.idea
.php_cs.cache
.phpunit.result.cache
nbproject
composer.lock
docs/html
6 changes: 6 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ matrix:
- php: 7.4
env:
- DEPENDENCIES="--prefer-lowest --prefer-stable"
- php: 8.0
env:
- DEPENDENCIES="--ignore-platform-req=php"
- php: 8.0
env:
- DEPENDENCIES="--ignore-platform-req=php --prefer-lowest --prefer-stable"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should ignore these here. Better fix these issues properly. Not doing so makes this either not installable for normal users or it forces them to use this flag on their project too.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, makes sense, but without the flag, we cannot run the tests for php 8 until all dependencies supports php 8. What if we don't add php8 to composer.json, that way we don't officially support php8, but can still run tests.

The tests that fail (in addition to dependencies not supporting php8 yet) are related to the vimeo/psalm version.

  • Psalm 3.11.2 is successful
  • Psalm 3.18.2 fails. Seems that marking a class with @psalm-immutable conflicts with @psalm-pure somehow. Maybe this can be resolved by specifying a version constraint, but I guess this depends on how psalm develops, or removing all @psalm-pure annotations in classes annotated with @psalm-immutable


cache:
directories:
Expand Down
2 changes: 1 addition & 1 deletion src/Internal/DateTimeStringBugWorkaround.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public static function fixDateTimeString(string $dateTimeString): string
{
$micros = \substr($dateTimeString, 20, -1);

if (false === $micros) {
if (false === $micros || '' === $micros) {
// no microseconds given
$dateTimeString = \substr($dateTimeString, 0, 19) . '.';
$micros = '';
Expand Down
49 changes: 49 additions & 0 deletions tests/Internal/DateTimeStringBugWorkaroundTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/**
* This file is part of prooph/event-store.
* (c) 2014-2020 Alexander Miertsch <[email protected]>
* (c) 2015-2020 Sascha-Oliver Prolic <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace ProophTest\EventStore\Internal;

use PHPUnit\Framework\TestCase;
use Prooph\EventStore\Internal\DateTimeStringBugWorkaround;

class DateTimeStringBugWorkaroundTest extends TestCase
grymmare marked this conversation as resolved.
Show resolved Hide resolved
{
public function provider(): array
{
return [
[
'2020-12-14T12:55:57Z',
'2020-12-14T12:55:57.000000Z',
],
[
'2020-12-14T12:55:57.1234567Z',
'2020-12-14T12:55:57.123456Z',
],
[
'2020-12-14T12:55:57.1234Z',
'2020-12-14T12:55:57.123400Z',
],
];
}

/**
* @param string $date
* @param string $expectedConvertedDate
* @dataProvider provider
*/
public function test_it_correctly_converts_datetime(string $date, string $expectedConvertedDate): void
{
$actualConvertedDate = DateTimeStringBugWorkaround::fixDateTimeString($date);
$this->assertEquals($expectedConvertedDate, $actualConvertedDate);
}
}