search

cartalyst / sentinel

A framework agnostic authentication & authorization system.
BSD 3-Clause "New" or "Revised" License
1.51k stars 238 forks source link

Fix Native Cookie forget issue + Add more tests #483

Closed 8633brown closed 5 years ago

8633brown commented 5 years ago

currently one test failing as forget does not set the max age of the cookie. https://github.com/cartalyst/sentinel/blob/c396b0363f0ffb8d010ea7830578ac7b42af5d49/src/Cookies/NativeCookie.php#L73-L76 I'm guessing it should call NativeCookie::setcookie directly instead of calling put. https://github.com/cartalyst/sentinel/blob/c396b0363f0ffb8d010ea7830578ac7b42af5d49/src/Cookies/NativeCookie.php#L57-L60

also php seems to only allow a max age of 0 so thats why im testing for

8633brown commented 5 years ago

should be all good now.

brunogaspar commented 5 years ago

Thanks!

There's one test still failing, but i'm not sure about using the xdebug_get_headers due to the fact that, i sometimes run the tests without Xdebug so it's faster to run, but i don't have any suggestion at the moment.

8633brown commented 5 years ago

Thats completely fair. i was assuming you always ran it with xdebug and it wouldnt be a big deal as the phpunit.xml always produces code coverage which i believe you need xdebug for. anyway ive removed the use of xdebug now. however the methods to remove xdebug are not all that pretty.

we can either mock NativeCookies::setCookie which is mocking the SUT or we can mock the global function setcookie. mocking the global involves fiddling namespaces to effectively hide the global function behind the namespaced function which is what ive done in the last commit. the only real difference is codecoverage in this case. if youd rather we just mocked the NativeCookie just let me know.

I havent fixed the test failing as i believe the tests are correct to fail here. see my initial comment. as i said the NativeCookie::forget method seems as though it should be setting a negative max age. however it calls the `::put method with the negative parameter and put then passes the opposite parameter to ::setCookie so i'm not sure what you want to do for this case. This should also be a sepperate PR no?

brunogaspar commented 5 years ago

Thats completely fair. i was assuming you always ran it with xdebug and it wouldnt be a big deal as the phpunit.xml always produces code coverage which i believe you need xdebug for. anyway ive removed the use of xdebug now. however the methods to remove xdebug are not all that pretty.

heh it depends, i usually run without it, unless i want to analyse the coverage :)

In regards to the test, i don't mind having it fixed on this pull request, it's all good.

Thanks for your work, really appreciate it!

brunogaspar commented 5 years ago

Thanks, will do a review later today.

@8633brown Do you have an email address i can use to contact you?

Thanks!

8633brown commented 5 years ago

Sorry, hope you dont mind. been adding some extra tests and some refactoring to a few more tests.