briannesbitt / Carbon

A simple PHP API extension for DateTime.
https://carbon.nesbot.com/
MIT License
16.57k stars 1.28k forks source link

CarbonPeriod toIso8601String without date interval #1999

Closed erikgaal closed 9 months ago

erikgaal commented 4 years ago

Hello,

I encountered an issue with the following code:

$isoString = '2007-03-01T13:00:00Z/2008-05-11T15:30:00Z';
$period = CarbonPeriod::createFromIso($isoString);

echo $period->toIso8601String();

Carbon version: 2.29.1

PHP version: 7.4.1

I expected to get:

2007-03-01T13:00:00Z/2008-05-11T15:30:00Z

But I actually get:

2007-03-01T13:00:00Z/P1D/2008-05-11T15:30:00Z

This is both a bug report and a feature request. By default, a CarbonInterval of 1 day is added to the CarbonPeriod, even though the period is parsed from ISO that doesn't have an interval. There is currently also no way to generate an ISO8601 string without interval.

I would be open to making a PR for this, however, we must discuss what the default behaviour for the dateInterval would be when creating a CarbonPeriod from an ISO string, and from the constructor.

kylekatarnls commented 4 years ago

There is no relation between the ISO you passed to createFromIso and the output of toIso8601String as there is many ways to create periods and toIso8601String must be available and uniform no matter how the period was created (so it can be parsed easily by other library/API). This point is not going to change, so what you get for the precise code example you gave is the expected behavior and won't change.

Periods can be used in many ways, and a very usual case is iteration, so we need a default interval. It would be too annoying having to specify a date interval for such a simple use as:

foreach (CarbonPeriod::create('today', 'next Monday') as $day) {
}

So this sounds to me like a need to solve with a macro:

CarbonPeriod::macro('toIso8601Ends', static function () {
  [$start, , $end] = explode('/', self::this()->toIso8601String());

  return "$start/$end";
});

Usage:

$isoString = '2007-03-01T13:00:00Z/2008-05-11T15:30:00Z';
$period = CarbonPeriod::createFromIso($isoString);

echo $period->toIso8601Ends(); // 2007-03-01T13:00:00Z/2008-05-11T15:30:00Z

Wouldn't it be fine?

lancechentw commented 11 months ago

How about updating the API description for CarbonPeriod::toIso8601String() with something like

Format the date period into iso8601 datetime/iso8601 duration, or iso8601 datetime/iso8601 duration/iso8601 datetime if a end time is given explicitly.

I understand that CarbonPeriod::toIso8601String() is not going to get a change, as it would be a breaking change and we can just create a macro to achieve what we want.

But I guess the pitfall here is that most developers would expect a toIso8601String() method to return a string that conform to the ISO8601 spec, and they could just pass it into a createFromIso8601() method from another library/language. However, the start/duration/end format does not seem to follow the spec (from what is listed on Wikipedia/ISO8601), and Luxon's Interval does not take it.

kylekatarnls commented 11 months ago

The thing is that a period is not a simple interval, it's a repeated interval. And ISO 8601 spec for repeated interval only allow recurrences to be defined as a number, if it's set this way, then, the toIso8601String() will return a result that matchs specs for repeated interval:

echo CarbonPeriod::create(3, '1 day', '2000-01-01 UTC')->toIso8601String();

Outputs:

R3/2000-01-01T00:00:00+00:00/P1D

Which is valid ISO8601 Repeating interval so Luxon should accept it.

Changing the method would be a breaking change but we can think about another method or a parameter to make it return only start and end, maybe toTimeIntervalIso8601String

kylekatarnls commented 10 months ago

Similarly, if you have neither end nor recurrence, you'll get the same ISO in and out:

echo CarbonPeriod::createFromIso('2000-01-01T00:00:00+00:00/PT12H')->toIso8601String();

=>

2000-01-01T00:00:00+00:00/PT12H
kylekatarnls commented 10 months ago

This behavior won't change for 2.x for backward-compatibility but we can have it in 3.0. #2909