Fixed expires

[Ragazzo]

Follow up zendframework/zendframework#7557

RFC

[Martin-P]

Just as a reminder: please add unittests.

[Ragazzo]

Afaik, empty expires means that cookie is only for current user session after which it should be deleted. Need to review if this fix is correct according this statetment.

[Martin-P]

Where is the actual test if the expires is null? You are now testing nothing.

[Ragazzo]

i test condition if expires is empty string, remove fix to code and exception will be thrown

[Ragazzo]

or you are referring to missing ->isNull on getExpires ?

[Martin-P]

There is no assertion, so nothing is tested. You can use a try/catch block to test if an exception is thrown. Also please update the method name so it reflects what you are actually testing.

You can do it like this:

public function testEmptyExpiresDoesNotThrowException()
{
    $cookieString = 'Set-Cookie:path=/; Expires=; HttpOnly;';

    try {
        SetCookie::fromString($cookie);
        $this->pass('No exception was thrown for empty "expires" value');
    } catch (\Exception $exception) {
        $this->fail('Empty "expires" value should not throw an exception');
    }   
}

[Ragazzo]

dont need to add try catch, you can use @expectedException, overall i will only update code with assertIsNull, your fix is not correct. Tests should not always have assertions, if test has passed without exceptions than it is ok, pass or fail

[Martin-P]

you can use @expectedException

If you expect an exception you can use that, but in this case you expecting the opposite: no exception. You can not use annotations for that, because there is none for "not expecting an exception".

Tests should not always have assertions, if test has passed without exceptions than it is ok, pass or fail

I agree it is not mandatory, but IMO it is better to be explicit when testing.

[Maks3w]

Please assert getExpires returns the correct value.

[Ragazzo]

Ok

[Martin-P]

Please assert getExpires returns the correct value.

I think that is a test on its own. One test to see there is no exception thrown and another test to see if getExpires returns null.

[Ragazzo]

@Maks3w done, ready for review

[Maks3w]

Should 0 to be handle as null too?

[Martin-P]

No, it seems 0 is not allowed. See: zendframework/zendframework#4684 (comment)

[Maks3w]

@Martin-P that thread is about Expires HEADER

If set to 0, or omitted, the cookie will expire at the end of the session (when the browser closes).
http://php.net/manual/en/function.setcookie.php

[Ragazzo]

not sure, i've read rfc, but it is still unclear for me about this situation http://tools.ietf.org/html/rfc6265#section-4.1.2.1

yet, without this fix exception will be thrown that does not feel right

[Martin-P]

@Maks3w I know, but a cookie is just a header. Also the class name confirms this: Zend\Http\Header\SetCookie.

Edit: The PHP function setcookie is not used in this class, so I doubt if that applies here.

[Maks3w]

@Martin-P Expires header and Set-Cookie headers are defined by different rules.

It's wrong assume a constraint for Proxy header affects to Authorization header. The same is applicable for Expires and Set-Cookie

[Martin-P]

@Ragazzo Can you provide a link to the specifications where empty expires is mentioned?

[Ragazzo]

@Martin-P do let me know if other things need to be done, as for rfc as i said i am not sure about correct behavior in this case, but throwing exception on valid header attribute does not sound right too

[weierophinney]

@Ragazzo — per @Martin-P , can you provide the link to the expires specification in the Cookie header RFC so (a) others coming to the issue know where to look, and (b) we can validate the behavior is correct per the RFC?

[Ragazzo]

@weierophinney updated description, yet as i said it is still unclean for me about this situation in RFC

[ezimuel]

I close this PR due to inactivity.