Skip to content

DateIntervalType (negative support) resolves doctrine/dbal#2578 #2579

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

Merged
merged 2 commits into from
Mar 29, 2018
Merged

DateIntervalType (negative support) resolves doctrine/dbal#2578 #2579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff36931
DateIntervalType (negative support) resolves doctrine/dbal#2578
galeaspablo Jul 2, 2017
7c97593
Add type declarations to DateIntervalTest
lcobucci Jul 23, 2017
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
15 changes: 12 additions & 3 deletions lib/Doctrine/DBAL/Types/DateIntervalType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@
namespace Doctrine\DBAL\Types;

use Doctrine\DBAL\Platforms\AbstractPlatform;
use function substr;

/**
* Type that maps interval string to a PHP DateInterval Object.
*/
class DateIntervalType extends Type
{
public const FORMAT = '%RP%YY%MM%DDT%HH%IM%SS';

/**
* {@inheritdoc}
*/
Expand Down Expand Up @@ -38,7 +41,7 @@ public function convertToDatabaseValue($value, AbstractPlatform $platform)
}

if ($value instanceof \DateInterval) {
return $value->format('P%YY%MM%DDT%HH%IM%SS');
return $value->format(self::FORMAT);
}

throw ConversionException::conversionFailedInvalidType($value, $this->getName(), ['null', 'DateInterval']);
Expand All @@ -54,9 +57,15 @@ public function convertToPHPValue($value, AbstractPlatform $platform)
}

try {
return new \DateInterval($value);
$interval = new \DateInterval(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.

With this you assume that $value always have the sign, which is not the case for existing data.
We should make sure that the type would still work for the "old" format.

Copy link
Member

Choose a reason for hiding this comment

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

We should make sure that the type would still work for the "old" format.

Testing it too 😉

Copy link
Contributor Author

@galeaspablo galeaspablo Dec 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think \DateInterval accepts a leading + or -.
How about?

try {
            $firstCharacter = substr($value, 0, 1);
            if ($firstCharacter !== '+' && $firstCharacter !== '-') {
                  return new \DateInterval($value);
            }
            $interval = new \DateInterval(substr($value, 1));
            if ($firstCharacter === '-') {
                 $interval->invert = 1;
             }
             return $interval;
}

Edit Also, just realized. This change is for BC, inside the development branch. I don't think we should be doing this. That's a big can of worms to open. For instance, there's another pull request that might change the whole format #2316

Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents on this. As already pointed out, this is a BC break that cause schema changes and format changes, which we cannot accept for a minor release. We could however introduce a new type like SignedDateIntervalType to keep BC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this really a BC break? This isn't on 2.5.

Copy link
Member

Choose a reason for hiding this comment

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

@galeaspablo oh you are right I thought it was part of 2.5 already. Thanks for pointing that out, then we won't have a problem with BC indeeed :)


if (substr($value, 0, 1) === '-') {
$interval->invert = 1;
}

return $interval;
} catch (\Exception $exception) {
throw ConversionException::conversionFailedFormat($value, $this->getName(), 'P%YY%MM%DDT%HH%IM%SS', $exception);
throw ConversionException::conversionFailedFormat($value, $this->getName(), self::FORMAT, $exception);
}
}

Expand Down
57 changes: 39 additions & 18 deletions tests/Doctrine/Tests/DBAL/Types/DateIntervalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@

namespace Doctrine\Tests\DBAL\Types;

use Doctrine\DBAL\Types\ConversionException;
use Doctrine\DBAL\Types\DateIntervalType;
use Doctrine\DBAL\Types\Type;
use Doctrine\Tests\DBAL\Mocks\MockPlatform;
use Doctrine\Tests\DbalTestCase;

class DateIntervalTest extends \Doctrine\Tests\DbalTestCase
final class DateIntervalTest extends DbalTestCase
{
/**
* @var MockPlatform
Expand All @@ -20,66 +23,84 @@ class DateIntervalTest extends \Doctrine\Tests\DbalTestCase
/**
* {@inheritDoc}
*/
protected function setUp()
protected function setUp() : void
{
$this->platform = new MockPlatform();
$this->type = Type::getType('dateinterval');

self::assertInstanceOf('Doctrine\DBAL\Types\DateIntervalType', $this->type);
self::assertInstanceOf(DateIntervalType::class, $this->type);
}

public function testDateIntervalConvertsToDatabaseValue()
public function testDateIntervalConvertsToDatabaseValue() : void
{
$interval = new \DateInterval('P2Y1DT1H2M3S');

$expected = 'P02Y00M01DT01H02M03S';
$expected = '+P02Y00M01DT01H02M03S';
$actual = $this->type->convertToDatabaseValue($interval, $this->platform);

self::assertEquals($expected, $actual);
}

public function testDateIntervalConvertsToPHPValue()
public function testDateIntervalConvertsToPHPValue() : void
{
$date = $this->type->convertToPHPValue('P02Y00M01DT01H02M03S', $this->platform);
self::assertInstanceOf('DateInterval', $date);
self::assertEquals('P02Y00M01DT01H02M03S', $date->format('P%YY%MM%DDT%HH%IM%SS'));
$interval = $this->type->convertToPHPValue('+P02Y00M01DT01H02M03S', $this->platform);

self::assertInstanceOf(\DateInterval::class, $interval);
self::assertEquals('+P02Y00M01DT01H02M03S', $interval->format(DateIntervalType::FORMAT));
}

public function testNegativeDateIntervalConvertsToDatabaseValue() : void
{
$interval = new \DateInterval('P2Y1DT1H2M3S');
$interval->invert = 1;

$actual = $this->type->convertToDatabaseValue($interval, $this->platform);

self::assertEquals('-P02Y00M01DT01H02M03S', $actual);
}

public function testNegativeDateIntervalConvertsToPHPValue() : void
{
$interval = $this->type->convertToPHPValue('-P02Y00M01DT01H02M03S', $this->platform);

self::assertInstanceOf(\DateInterval::class, $interval);
self::assertEquals('-P02Y00M01DT01H02M03S', $interval->format(DateIntervalType::FORMAT));
}

public function testInvalidDateIntervalFormatConversion()
public function testInvalidDateIntervalFormatConversion() : void
{
$this->expectException('Doctrine\DBAL\Types\ConversionException');
$this->expectException(ConversionException::class);

$this->type->convertToPHPValue('abcdefg', $this->platform);
}

public function testDateIntervalNullConversion()
public function testDateIntervalNullConversion() : void
{
self::assertNull($this->type->convertToPHPValue(null, $this->platform));
}

/**
* @group DBAL-1288
*/
public function testRequiresSQLCommentHint()
public function testRequiresSQLCommentHint() : void
{
self::assertTrue($this->type->requiresSQLCommentHint($this->platform));
}

/**
* @dataProvider invalidPHPValuesProvider
*
* @param mixed $value
*/
public function testInvalidTypeConversionToDatabaseValue($value)
public function testInvalidTypeConversionToDatabaseValue($value) : void
{
$this->expectException('Doctrine\DBAL\Types\ConversionException');
$this->expectException(ConversionException::class);

$this->type->convertToDatabaseValue($value, $this->platform);
}

/**
* @return mixed[][]
*/
public function invalidPHPValuesProvider()
public function invalidPHPValuesProvider() : array
{
return [
[0],
Expand Down