Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Added the delete_cookie() function in order to help with the removal of ... #1120

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
3 participants

jc1arke commented Mar 4, 2012

...cookies that might have been set. Although one can make use of the set_cookie() function and pass an 0 via the expire field, this is a simpler method

@jc1arke jc1arke Added the delete_cookie() function in order to help with the removal …
…of cookies that might have been set. Although one can make use of the set_cookie() function and pass an 0 via the expire field, this is a simpler method
386c3ec
Contributor

it-can commented Mar 4, 2012

jc1arke commented Mar 4, 2012

That is in the helper. So without having to load the helper since you are already working with the cookies in the controller side of things. So for instance, if you have CI_Cookie loaded you don't have to load the helper as well in order to perform this action.

@it-can it-can commented on an outdated diff Mar 5, 2012

system/core/Input.php
@@ -226,6 +226,27 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if( isset($name) && !empty($name) && $this -> cookie( $name ) ) // Check if the cookie exists, just a failsafe, one never knows.... :)
@it-can

it-can Mar 5, 2012

Contributor

should be: if (isset($name) && ! empty($name) && $this->cookie($name))

@it-can it-can commented on an outdated diff Mar 5, 2012

system/core/Input.php
@@ -226,6 +226,27 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if( isset($name) && !empty($name) && $this -> cookie( $name ) ) // Check if the cookie exists, just a failsafe, one never knows.... :)
+ {
+ $this -> set_cookie( $name, '', 0 );
@it-can

it-can Mar 5, 2012

Contributor

should be: $this->set_cookie($name, '', 0);

@it-can it-can commented on an outdated diff Mar 5, 2012

system/core/Input.php
@@ -226,6 +226,27 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if( isset($name) && !empty($name) && $this -> cookie( $name ) ) // Check if the cookie exists, just a failsafe, one never knows.... :)
+ {
+ $this -> set_cookie( $name, '', 0 );
+ return true; // Let the calling function know that it succeeded
@it-can

it-can Mar 5, 2012

Contributor

TRUE

@it-can it-can commented on an outdated diff Mar 5, 2012

system/core/Input.php
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if( isset($name) && !empty($name) && $this -> cookie( $name ) ) // Check if the cookie exists, just a failsafe, one never knows.... :)
+ {
+ $this -> set_cookie( $name, '', 0 );
+ return true; // Let the calling function know that it succeeded
+ }
+
+ return false; // Could not find the cookie, or no name provided
@it-can

it-can Mar 5, 2012

Contributor

FALSE

Contributor

it-can commented Mar 5, 2012

Is it also an idea to change the cookie_helper, delete function to use the input class so we don't have multiple functions doing the same thing, but still keep backwards compatiblilty?

@jc1arke jc1arke Changed the format of the core Input class to conform to CI standards…
…, as well as changed the cookie_helper to make use of the new delete_cookie function in the core Input controller
079c419

jc1arke commented Mar 5, 2012

Thanks for all the notes :) I have just pushed the update for the formatting as suggested and also setup the delete_cookie helper function to make use of the delete_cookie function in the core CI_Input controller

@narfbg narfbg commented on an outdated diff Mar 5, 2012

system/core/Input.php
@@ -226,6 +226,27 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if(isset($name) && !empty($name) && $this->cookie($name)) // Check if the cookie exists, just a failsafe, one never knows.... :)
@narfbg

narfbg Mar 5, 2012

Contributor

isset($name) will always return TRUE, unless you pass NULL as it's value and ! empty($name) basically evaluates as isset($name) && $name != '' anyway ... you don't need isset() here.

Also you should have a space on each side of the exclamation mark and one after if as well.
Might be a good idea to move the comment on the previous line too, I doubt that anybody in our days would be reading that with a 480x320 resolution, but it still causes a need to use the scrollbar in e.g. this view we're currently looking at. :)

@narfbg narfbg commented on an outdated diff Mar 5, 2012

system/core/Input.php
@@ -226,6 +226,27 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ if(isset($name) && !empty($name) && $this->cookie($name)) // Check if the cookie exists, just a failsafe, one never knows.... :)
+ {
+ $this->set_cookie($name, '', 0);
+ return TRUE; // Let the calling function know that it succeeded
@narfbg

narfbg Mar 5, 2012

Contributor

It's good to have code commented, but ... well, we know what return TRUE/FALSE does in a 10-line method. :)

Contributor

narfbg commented Mar 5, 2012

I like the idea of the cookie helper utilizing the new method instead, but if it's going to work this way - one of the two should change its behavior. The delete_cookie() function used to accept 4 parameters ... now it's only 1 that's actually used - doesn't make much sense.

Another thing is (although I'm probably just giving too much thought on this) - do we need that to return boolean instead of void? I really can't think of a case where such check would be needed and even if there is, just because we've sent an empty cookie, we're not guaranteed that the browser will delete it - that's out of our control.

... just some thoughts, not saying that it must be like I suggest.

jc1arke commented Mar 6, 2012

Thanks mate for the comments. I have updated the code as you can see from the latest 2 commits. Also, returning boolean seems like a safer option. One doesn't have to check for output, such as

delete_cookie('thisIsMyCookie'); // No need to check output, just perform the action

All depends on your usage I suppose, but still a good point though

@narfbg narfbg commented on an outdated diff Mar 6, 2012

system/core/Input.php
@@ -226,6 +226,28 @@ public function cookie($index = '', $xss_clean = FALSE)
// ------------------------------------------------------------------------
/**
+ * Remove cookie
+ *
+ * Accepts the name of the cookie and expires it accordingly
+ *
+ * @param string the name of the cookie
+ * @return bool
+ */
+ public function delete_cookie($name = '')
+ {
+ // Check if the cookie exists, just a failsafe, one never knows.... :)
+ if (! empty($name) && $this->cookie($name))
@narfbg

narfbg Mar 6, 2012

Contributor

Still need to put that space - if ( ! empty($name) ...

Contributor

narfbg commented Mar 6, 2012

I'm still concerned with backwards compatibility of the helper's function.

Contributor

it-can commented Mar 6, 2012

Yes it should be full backwards compatible...

jc1arke commented Mar 6, 2012

Okay lads, you need to make a decision here now. I removed the extra 3 parameters from the helper based on your comment that there is no need for them anymore. I can place them back for backward compatibility, but if we keep going "needs to be backward compatible" we will end up with another IE6 monster where we keep saying "but this needs to work in it" :)

Suggestions?

Contributor

narfbg commented Mar 6, 2012

Actually, nobody said that there's no need for it. I said that it doesn't make sense, because they're not used.

There are 2 problems with this one:

  • It does need to be fully backwards compatible. I understand what you mean with the IE6 analogy, but this is not the same case. We're talking about removing useful functionality here, not switching to more modern solutions. :)
  • It causes too much discussion for something that we can already do with the set_cookie() method.

I'd like to hear some other opinions before I ask you to do yet another change on it ... @philsturgeon @ericbarnes @katzgrau @seejohnrun ?

Here's what I personally think - revert the changes to the cookie helper function and stick to just adding a method to the Input library. For obvious reasons, it's indeed best to reuse code when possible, but in this case it's not the same logic to be executed, so that rule doesn't fit.

Contributor

it-can commented Mar 6, 2012

@narfbg Think you're right... Leave the cookie helper like it was and add delete_cookie to input class...

Contributor

narfbg commented Oct 5, 2012

It's been awhile ... now looking at the setcookie() manual - you can pretty much do the same thing by just calling setcookie(<cookiename>);, so we don't need this.

@narfbg narfbg closed this Oct 5, 2012

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