DateIntervalType (negative support) resolves doctrine/dbal#2578 #2579
Conversation
|
If #2316 is merged before this, we'll need to refactor this PR. |
| @@ -56,9 +58,13 @@ public function convertToPHPValue($value, AbstractPlatform $platform) | |||
| } | |||
|
|
|||
| try { | |||
| return new \DateInterval($value); | |||
| $interval = new \DateInterval(substr($value, 1)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We should make sure that the type would still work for the "old" format.
Testing it too 😉
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is this really a BC break? This isn't on 2.5.
There was a problem hiding this comment.
@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 :)
|
Any update on this? Would you go ahead with the merge with the solution from #2579 (comment), @lcobucci? |
|
After looking at it with @sebastianfeldmann at the DPC code night, I'd say that this is NOT ready to be deployed. Instead, a new type is needed, because this kind of change would mean that users of the library would have half the data WITH the sign, and half the data without. In addition to that, binding with the sign would change behavior: queries using parameter binding would be broken. That's the way to go IMO, whereas the existing type would have to stay unchanged. |
|
Closing and pushing discussion back to #2578 |
|
Re-opening as per #2579 (comment) - sorry @galeaspablo! |
|
#2316 was merged. Through it, the database value format was changed. As I said before, this means that I had to refactor my PR. I've refactored my PR to work with the new master branch (i.e. handle negative support with the new format). |
|
@lcobucci as mentioned before, the fact that I released early is my fuckup (technical terminology). We discussed it earlier tonight, and while it is technically possible for us to introduce a quick This PR is now a BC break, and I don't think we can fix it without introducing a @galeaspablo I apologise, this will need some rework :-( |
|
Haha. Shit happens (technical terminology). So SignedDateIntervalType would get merged? |
|
#2578 is labeled as a We also need to keep in mind that new features will land just on |
|
Hi, shall we consider this a blocker for 2.7? If so, it needs a rebase & fixing the conflicts, otherwise we could push this back to 2.8/3.0 and decide later. |
No description provided.