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

Invalid examples in documentation. Misleading exception message for intersect() #33

Closed
joshribakoff opened this Issue Apr 1, 2016 · 11 comments

Comments

Projects
None yet
3 participants
@joshribakoff

joshribakoff commented Apr 1, 2016

     */
    public function intersect(Period $period)
    {
        if ($this->abuts($period)) {
            throw new LogicException('Both object should not abuts');
        }

        return new self(
            (1 === self::compareDate($period->startDate, $this->startDate)) ? $period->startDate : $this->startDate,
            (-1 === self::compareDate($period->endDate, $this->endDate)) ? $period->endDate : $this->endDate
        );
    }

Its as if its meant to swap start/end date automatically, but it doesn't work correctly because whether the ternary is true or false, its doing the same thing, rendering the conditional pointless. I can't get it to work, even with this example from the documentation:

$period        = Period::createFromDuration(2012-01-01, '2 MONTHS');
    $anotherPeriod = Period::createFromDuration(2012-01-15, '3 MONTHS');
    $intersectPeriod = $period->intersect($anotherPeriod);

I always get this error:

PHP Fatal error:  Uncaught exception 'LogicException' with message 'The ending datepoint must be greater or equal to the starting datepoint
@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 1, 2016

In addition to the swapping logic being broken, most of the examples in the documentation found here are broken - http://period.thephpleague.com/api/comparing/

$period        = Period::createFromDuration(2012-01-01, '2 MONTHS');

This will subtract 1 from 2012, twice, and pass the resulting integer. It appears its meant to be shown passing a string, not doing subtraction.

Even after quoting the datetime string being passed, intersect still often throws the exception, like for these two periods:

League\Period\Period Object
(
    [startDate:protected] => DateTimeImmutable Object
        (
            [date] => 2015-11-11 00:00:00.000000
            [timezone_type] => 2
            [timezone] => Z
        )

    [endDate:protected] => DateTimeImmutable Object
        (
            [date] => 2015-12-12 23:59:59.000000
            [timezone_type] => 2
            [timezone] => Z
        )

)
League\Period\Period Object
(
    [startDate:protected] => DateTimeImmutable Object
        (
            [date] => 2016-04-19 00:00:00.000000
            [timezone_type] => 2
            [timezone] => Z
        )

    [endDate:protected] => DateTimeImmutable Object
        (
            [date] => 2016-04-26 23:59:59.000000
            [timezone_type] => 2
            [timezone] => Z
        )

)

I don't see any swapped start/end dates, so it should not be complaining.

joshribakoff commented Apr 1, 2016

In addition to the swapping logic being broken, most of the examples in the documentation found here are broken - http://period.thephpleague.com/api/comparing/

$period        = Period::createFromDuration(2012-01-01, '2 MONTHS');

This will subtract 1 from 2012, twice, and pass the resulting integer. It appears its meant to be shown passing a string, not doing subtraction.

Even after quoting the datetime string being passed, intersect still often throws the exception, like for these two periods:

League\Period\Period Object
(
    [startDate:protected] => DateTimeImmutable Object
        (
            [date] => 2015-11-11 00:00:00.000000
            [timezone_type] => 2
            [timezone] => Z
        )

    [endDate:protected] => DateTimeImmutable Object
        (
            [date] => 2015-12-12 23:59:59.000000
            [timezone_type] => 2
            [timezone] => Z
        )

)
League\Period\Period Object
(
    [startDate:protected] => DateTimeImmutable Object
        (
            [date] => 2016-04-19 00:00:00.000000
            [timezone_type] => 2
            [timezone] => Z
        )

    [endDate:protected] => DateTimeImmutable Object
        (
            [date] => 2016-04-26 23:59:59.000000
            [timezone_type] => 2
            [timezone] => Z
        )

)

I don't see any swapped start/end dates, so it should not be complaining.

@GodsBoss

This comment has been minimized.

Show comment
Hide comment
@GodsBoss

GodsBoss Apr 4, 2016

Contributor

Your first example works if you add the missing quotes:

$period = Period::createFromDuration('2012-01-01', '2 MONTHS'); // 2012-01-01 till 2012-03-01
$anotherPeriod = Period::createFromDuration('2012-01-15', '3 MONTHS'); // 2012-01-15 till 2012-04-15
$intersectPeriod = $period->intersect($anotherPeriod); // 2012-01-15 till 2012-03-01

The example in your second comment works as documented - the periods do not overlap, in this case Period::intersect throws an exception (http://period.thephpleague.com/api/comparing/#periodintersect):

If they do not overlap or abut, then an Exception is thrown.

Contributor

GodsBoss commented Apr 4, 2016

Your first example works if you add the missing quotes:

$period = Period::createFromDuration('2012-01-01', '2 MONTHS'); // 2012-01-01 till 2012-03-01
$anotherPeriod = Period::createFromDuration('2012-01-15', '3 MONTHS'); // 2012-01-15 till 2012-04-15
$intersectPeriod = $period->intersect($anotherPeriod); // 2012-01-15 till 2012-03-01

The example in your second comment works as documented - the periods do not overlap, in this case Period::intersect throws an exception (http://period.thephpleague.com/api/comparing/#periodintersect):

If they do not overlap or abut, then an Exception is thrown.

@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 4, 2016

Shouldn't the exception say something along the lines of

These periods do not overlap, and cannot be intersected

That would make sense, but instead it says:

The ending datepoint must be greater or equal to the starting datepoint

Bottom line is the documentation examples don't work as-is, and the error messages are stating I swapped a start/end date, when this is objectively not true. Most users would just assume the library doesn't work. You're welcome for reporting the issue(s) & debugging it :) Great library other than these issues.

PS > You never acknowledged the fact your ternary operator does nothing. Regardless of if the comparison evaluates true/false, it always returns start state first, when it appears it was meant to swap them. You might want to double check that part (implementation of intersect()).

The code is like this, currently:

$result = (bool)$a ? true : true;

If there is no bug, it should be simplified to:

$result = true;

But I suspect its another bug.

joshribakoff commented Apr 4, 2016

Shouldn't the exception say something along the lines of

These periods do not overlap, and cannot be intersected

That would make sense, but instead it says:

The ending datepoint must be greater or equal to the starting datepoint

Bottom line is the documentation examples don't work as-is, and the error messages are stating I swapped a start/end date, when this is objectively not true. Most users would just assume the library doesn't work. You're welcome for reporting the issue(s) & debugging it :) Great library other than these issues.

PS > You never acknowledged the fact your ternary operator does nothing. Regardless of if the comparison evaluates true/false, it always returns start state first, when it appears it was meant to swap them. You might want to double check that part (implementation of intersect()).

The code is like this, currently:

$result = (bool)$a ? true : true;

If there is no bug, it should be simplified to:

$result = true;

But I suspect its another bug.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Apr 4, 2016

Member

@joshribakoff

as stated in the documentation:

Before getting the intersection, make sure the Period objects, at least, overlap each other.

which can be translated by the following code

<?php

if ($period1->overlaps($period2)) {
    $interesct = $period1->intersect($period2);
}

If you don't use the code above then Period::intersect will only check if the objects do not abuts. The rest of the check is done when instantiating the possible intersect Period object because of

  • the way the intersection is computed.
  • the way Period objects are validated.

TL;DR: The exception you get is emitted by the constructor not by the method per se.

Out of curiosity which version of League\Period are you using as I am sure neither version 3.x nor 2.x stable versions as any compareDate methods. If I recall correctly this was a beta or an alpha version from the 3.X version that was never released. If you look at the most recent stable release 3.1.1 you'll see that the intersect method code is different.

Hope these explanations will help.

Member

nyamsprod commented Apr 4, 2016

@joshribakoff

as stated in the documentation:

Before getting the intersection, make sure the Period objects, at least, overlap each other.

which can be translated by the following code

<?php

if ($period1->overlaps($period2)) {
    $interesct = $period1->intersect($period2);
}

If you don't use the code above then Period::intersect will only check if the objects do not abuts. The rest of the check is done when instantiating the possible intersect Period object because of

  • the way the intersection is computed.
  • the way Period objects are validated.

TL;DR: The exception you get is emitted by the constructor not by the method per se.

Out of curiosity which version of League\Period are you using as I am sure neither version 3.x nor 2.x stable versions as any compareDate methods. If I recall correctly this was a beta or an alpha version from the 3.X version that was never released. If you look at the most recent stable release 3.1.1 you'll see that the intersect method code is different.

Hope these explanations will help.

@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 4, 2016

I tested with 3.0.0, and I also tested master. The bug exists in both:

 PHP Fatal error:  Uncaught exception 'LogicException' with message 'The ending datepoint must be greater or equal to the starting datepoint' in /home/josh/www/isite/server/vendor/league/period/src/Period.php:69
Stack trace:
#0 /home/josh/www/isite/server/vendor/league/period/src/Period.php(834): League\\Period\\Period->__construct(Object(DateTimeImmutable), Object(DateTimeImmutable))

As you can see, its due to the erroneous ternary logic I've mentioned above, it does throw an exception as documented, but its not the expected exception.

Wouldn't the fix be to basically take this code:

return new self(
            (1 === self::compareDate($period->startDate, $this->startDate)) ? $period->startDate : $this->startDate,
            (-1 === self::compareDate($period->endDate, $this->endDate)) ? $period->endDate : $this->endDate
        );

And change it to something more like this?:

return new self(
            (1 === self::compareDate($period->startDate, $this->startDate)) ? $period->startDate : $this->endDate,
            (-1 === self::compareDate($period->endDate, $this->endDate)) ? $period->endDate : $this->startDate
        );

joshribakoff commented Apr 4, 2016

I tested with 3.0.0, and I also tested master. The bug exists in both:

 PHP Fatal error:  Uncaught exception 'LogicException' with message 'The ending datepoint must be greater or equal to the starting datepoint' in /home/josh/www/isite/server/vendor/league/period/src/Period.php:69
Stack trace:
#0 /home/josh/www/isite/server/vendor/league/period/src/Period.php(834): League\\Period\\Period->__construct(Object(DateTimeImmutable), Object(DateTimeImmutable))

As you can see, its due to the erroneous ternary logic I've mentioned above, it does throw an exception as documented, but its not the expected exception.

Wouldn't the fix be to basically take this code:

return new self(
            (1 === self::compareDate($period->startDate, $this->startDate)) ? $period->startDate : $this->startDate,
            (-1 === self::compareDate($period->endDate, $this->endDate)) ? $period->endDate : $this->endDate
        );

And change it to something more like this?:

return new self(
            (1 === self::compareDate($period->startDate, $this->startDate)) ? $period->startDate : $this->endDate,
            (-1 === self::compareDate($period->endDate, $this->endDate)) ? $period->endDate : $this->startDate
        );
@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Apr 4, 2016

Member
1 === self::compareDate($period->startDate, $this->startDate))

This code does not exist on the master branch or on any stable release of the package
here's a link to the current intersect method https://github.com/thephpleague/period/blob/master/src/Period.php#L814

Member

nyamsprod commented Apr 4, 2016

1 === self::compareDate($period->startDate, $this->startDate))

This code does not exist on the master branch or on any stable release of the package
here's a link to the current intersect method https://github.com/thephpleague/period/blob/master/src/Period.php#L814

@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 4, 2016

It came from 3.0.0 which was installed via bower. The same logic is present on master on line 820. If this is expected behavior, then the error message is just confusing.

joshribakoff commented Apr 4, 2016

It came from 3.0.0 which was installed via bower. The same logic is present on master on line 820. If this is expected behavior, then the error message is just confusing.

@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 4, 2016

After

if ($this->abuts($period)) {
            throw new LogicException('Both object should not abuts');
        }

Is there a reason not to add:

if(!$this->overlaps($period)) {
  throw new LogicException('Both object should overlap')
}

That would have prevented the confusion on my end.

joshribakoff commented Apr 4, 2016

After

if ($this->abuts($period)) {
            throw new LogicException('Both object should not abuts');
        }

Is there a reason not to add:

if(!$this->overlaps($period)) {
  throw new LogicException('Both object should overlap')
}

That would have prevented the confusion on my end.

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Apr 4, 2016

Member

It came from 3.0.0 which was installed via bower.

Well you should not install PHP packages with Bower as the system seems to be out of sync with the github/packagist version which are the only official release channels that are supported.

but the conditional is still doing the same thing regardless of if the express evaluates to true or false.

No, you are wrong, please review ternary usage in PHP

Is there a reason not to add the overlaps check

Fundamentally no, but, this check is not required because of the way periods are defined (http://period.thephpleague.com/definitions/#definitions). If the periods do not overlaps the call to the constructor will always emit a LogicException.

Member

nyamsprod commented Apr 4, 2016

It came from 3.0.0 which was installed via bower.

Well you should not install PHP packages with Bower as the system seems to be out of sync with the github/packagist version which are the only official release channels that are supported.

but the conditional is still doing the same thing regardless of if the express evaluates to true or false.

No, you are wrong, please review ternary usage in PHP

Is there a reason not to add the overlaps check

Fundamentally no, but, this check is not required because of the way periods are defined (http://period.thephpleague.com/definitions/#definitions). If the periods do not overlaps the call to the constructor will always emit a LogicException.

@joshribakoff

This comment has been minimized.

Show comment
Hide comment
@joshribakoff

joshribakoff Apr 4, 2016

you should not install PHP packages with Bower

I meant composer. Freudian slip.

No, you are wrong, please review ternary usage in PHP

Yeah, I'm wrong. I thought the ternary was meant to swap incorrect start/end dates, part of the reason I thought this was because the error message was alluding to swapped dates.

this check is not required

k. It would have avoided confusion on my end though. I'd highly recommend adding that check, defensive programming is usually a good thing... especially when you have users like me to defend against ;)

The current exception it throws is misleading, because it complains about swapped start/end dates instead of complaining the periods do not overlap. Its throwing an error message because of invalid internal calls, as a result of "missing" validation. Having that extra validation would have been nice, especially since the examples in the documentation were invalid [fixed now].

Anyways, I was able to get my code working, so thanks.

joshribakoff commented Apr 4, 2016

you should not install PHP packages with Bower

I meant composer. Freudian slip.

No, you are wrong, please review ternary usage in PHP

Yeah, I'm wrong. I thought the ternary was meant to swap incorrect start/end dates, part of the reason I thought this was because the error message was alluding to swapped dates.

this check is not required

k. It would have avoided confusion on my end though. I'd highly recommend adding that check, defensive programming is usually a good thing... especially when you have users like me to defend against ;)

The current exception it throws is misleading, because it complains about swapped start/end dates instead of complaining the periods do not overlap. Its throwing an error message because of invalid internal calls, as a result of "missing" validation. Having that extra validation would have been nice, especially since the examples in the documentation were invalid [fixed now].

Anyways, I was able to get my code working, so thanks.

@joshribakoff joshribakoff changed the title from intersect never works to Invalid examples in documentation. Misleading exception message for intersect() Apr 4, 2016

@nyamsprod

This comment has been minimized.

Show comment
Hide comment
@nyamsprod

nyamsprod Apr 15, 2016

Member

The exception message has been improved in the master branch.

Member

nyamsprod commented Apr 15, 2016

The exception message has been improved in the master branch.

@nyamsprod nyamsprod closed this Apr 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment