Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

consolidated getter/setter for userdata to just "data", also added deprecations #313

Closed
wants to merge 6 commits into from

6 participants

@jondavidjohn

http://codeigniter.uservoice.com/forums/40508-codeigniter-reactor/suggestions/2162587-consolidate-userdata-flashdata-getters-and-setters

example new usage for userdata


<?

// setting 
$this->session->data('key', 'information');

// set with array 
$array = array('key' => 'information') 
$this->session->data($array);

//getting 
$this->session->data('key');

// get all 
$this->session->data();
@jondavidjohn

Will add documentation and changelog info if approved.

@derekjones

How would you unset an element?

@jondavidjohn

good call, probably with $this->session->data('element', NULL); effectively setting it to null? would this interface make sense?

@derekjones

But $value defaults to NULL, so your getter would unset the variable in that case. A similar problem would occur is $value defaulted to an empty string or FALSE, since either of those could represent actual element values.

@jondavidjohn

I am aware of how it currently works ( in which I didn' t think of incorporating unset into the main getter/setter ).

I did not deprecate the unset_userdata function, currently thinking out how to wrap it in, suggestions are definetly welcome.

@derekjones

Socratic method blocked! ;)

There are different ways to handle it, one would be by taking an arbitrary number of arguments, checking the datatypes, and operating based on what's sent. The problem is that the various calls each need to be well documented, and it's very difficult with such agile methods for it to become second nature, i.e. not requiring a trip to the docs every single time a developer uses it.

So, my thoughts are that unsetting is a very different operation from setting or getting, in fact it's the very opposite of setting. Using a method to both do a certain action and its exact opposite is messy, and results in less readable code than having a specific method to "un" something.

One exception might be setting some bitwise constants to use as an optional third parameter to determine what should be done with the values, which would add code legibility, but still be needlessly complex.

@jondavidjohn

Undoubtedly trying to tie in the unset operation would create a level of complexity for both the readability of this function as well as the use by the end user.

I see nothing wrong with leaving unset_userdata in place or maybe renaming it to unset_data.

One creative option I was considering would be to set the default value of $value to an arbitrary value (maybe a random hex value?) and then operating knowing what the default value is instead of using NULL at the default value. Only downside to that would be if anyone ever hits that needle in a haystack and tries to set their session data to that value, it doesn't work.

then $this->session->data('key', null); would be possible and is in my mind the only intuitive interface for an operation like this.

@derekjones

Yes I think I agree, it's probably safer and more clear to just keep an unset method. I also concur on the naming, it seems simplest to just be unset_data(), since the opposing method is data(). The name is self-documenting as it's just applying a verb to an existing method.

@alexbilbie

Following discussion on Uservoice can I recommend we change the name of the functions to item() and unset_item(). Also in the interest of backwards compatibility are we going to keep the old functions and just proxy calls to the new functions?

@derekjones

Yes, the old versions should hook to the new ones, and a prominent deprecation notice should be made, and allowed to live for a few right of dot releases before being axed.

@jondavidjohn

I agree with item over data, should I consider this one approved, and start in on the docs and changelog ?

@gaker

Generally, I like the idea. However it does somewhat worry me on the impact it will have on the upgrade path for applications which rely on these heavily. I'd like to see your take on upgrade notes before I'm 100% comfy with this. I added a 2.1.0 upgrade notes file here:

053d67f

I'm also going to add a couple of notes in-line on the commits you have.

Thanks so much!

@jondavidjohn

@gaker


<h2> Update Session class Usage </h2>

<p>We have changed the way you retrieve and store session information.  Instead of having dedicated <code>$this->session->set_userdata($key, $value)</code> to store and <code>$this->session->userdata($key)</code> to retrieve, we have consolidated them to accept two different argument signatures and simplified the naming.  This single function is <code>$this->session->data()</code></p>

<p>This also applies to <code>flashdata</code> functions as well, making the same simplifications so that the single use function is <code>$this->session->flashdata()</code></p>

<h4>New Syntax</h4>

<textarea>
// store information 
$this->session->data('key', 'information');

// store with array useage
$array = array('key' => 'information') 
$this->session->data($array);

// retrieving 
$this->session->data('key');

// retrieve all 
$this->session->data();
</textarea>

<p> A simple find/replace, replacing ("$this->session->set_userdata", "$this->session->userdata") with ("$this->session->data") and ("$this->session->set_flashdata") with ("$this->session->flashdata") should be enough to cover most use cases, but you should thoroughly test and verify.</p>

<p class="important"><strong>Note:</strong>set_userdata, userdata, and set_flashdata will continue to operate normally but are marked deprecated so operation in future versions is not guaranteed</p>
@jondavidjohn

@gaker could you clarify what you mean by log_item() ?

@jondavidjohn

ok, I'm assuming it needs to be an informational message?

@onigoetz

Nice addition, will this once be merged ?

@jondavidjohn

Guess this guy got left behind?

@cryode

Yeah, this is obviously well before the Session class was turned into a driver. This PR would need to be resubmitted to follow the current driver's structure.

If something like this would be implemented, I'd like to see aliased method names, or essentially, add-on any new methods but leave the originals. That would keep backwards compatibility, but still offer the new functionality. The original methods could then be deprecated and eventually removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 100 additions and 16 deletions.
  1. +100 −16 system/libraries/Session.php
View
116 system/libraries/Session.php 100644 → 100755
@@ -418,14 +418,51 @@ function sess_destroy()
// --------------------------------------------------------------------
/**
+ * Get or Set specific item in the session array
+ *
+ * @access public
+ * @param string
+ * @param string
+ * @return string
+ */
+ public function data($item = NULL, $value = NULL)
+ {
+ if (is_null($value) && ! is_array($item)) // getting
+ {
+ if (is_null($item)) // get all
+ {
+ return $this->userdata;
+ }
+ return ( ! isset($this->userdata[$item])) ? FALSE : $this->userdata[$item];
+ }
+ else // setting
+ {
+ if (is_array($item))
+ {
+ foreach ($item as $key => $val)
+ {
+ $this->userdata[$key] = $val;
+ }
+ }
+ else
+ {
+ $this->userdata[$item] = $value;
+ }
+ $this->sess_write();
+ }
+ }
+
+ /**
* Fetch a specific item from the session array
*
+ * @deprecated 2.1.0
* @access public
* @param string
* @return string
*/
function userdata($item)
{
+ log_message('info','userdata DEPRECATED as of 2.1.0');
return ( ! isset($this->userdata[$item])) ? FALSE : $this->userdata[$item];
}
@@ -434,11 +471,13 @@ function userdata($item)
/**
* Fetch all session data
*
- * @access public
- * @return array
+ * @deprecated 2.1.0
+ * @access public
+ * @return array
*/
function all_userdata()
{
+ log_message('info','all_userdata DEPRECATED as of 2.1.0');
return $this->userdata;
}
@@ -447,18 +486,19 @@ function all_userdata()
/**
* Add or change data in the "userdata" array
*
- * @access public
- * @param mixed
- * @param string
- * @return void
+ * @deprecated 2.1.0
+ * @access public
+ * @param mixed
+ * @param string
+ * @return void
*/
function set_userdata($newdata = array(), $newval = '')
{
+ log_message('info','set_userdata DEPRECATED as of 2.1.0');
if (is_string($newdata))
{
$newdata = array($newdata => $newval);
}
-
if (count($newdata) > 0)
{
foreach ($newdata as $key => $val)
@@ -466,7 +506,6 @@ function set_userdata($newdata = array(), $newval = '')
$this->userdata[$key] = $val;
}
}
-
$this->sess_write();
}
@@ -475,11 +514,13 @@ function set_userdata($newdata = array(), $newval = '')
/**
* Delete a session variable from the "userdata" array
*
+ * @deprecated 2.1.0
* @access array
* @return void
*/
function unset_userdata($newdata = array())
{
+ log_message('info','unset_userdata DEPRECATED as of 2.1.0');
if (is_string($newdata))
{
$newdata = array($newdata => '');
@@ -496,19 +537,42 @@ function unset_userdata($newdata = array())
$this->sess_write();
}
+ /**
+ * Delete a session variable from the "userdata" array
+ *
+ * @access array
+ * @return void
+ */
+ function unset_data($data = array())
+ {
+ if (is_string($newdata))
+ {
+ $newdata = array($newdata => '');
+ }
+
+ foreach ($newdata as $key => $val)
+ {
+ unset($this->userdata[$key]);
+ }
+
+ $this->sess_write();
+ }
+
// ------------------------------------------------------------------------
/**
* Add or change flashdata, only available
* until the next request
*
- * @access public
- * @param mixed
- * @param string
- * @return void
+ * @deprecated 2.1.0
+ * @access public
+ * @param mixed
+ * @param string
+ * @return void
*/
function set_flashdata($newdata = array(), $newval = '')
{
+ log_message('info','set_flashdata DEPRECATED as of 2.1.0');
if (is_string($newdata))
{
$newdata = array($newdata => $newval);
@@ -549,16 +613,36 @@ function keep_flashdata($key)
// ------------------------------------------------------------------------
/**
- * Fetch a specific flashdata item from the session array
+ * Add or Retrieve flashdata, only available until the next request
*
* @access public
* @param string
+ * @param string
* @return string
*/
- function flashdata($key)
+ function flashdata($key, $value = NULL)
{
- $flashdata_key = $this->flashdata_key.':old:'.$key;
- return $this->userdata($flashdata_key);
+ if (is_null($value) && ! is_array($key)) // getting
+ {
+ $flashdata_key = $this->flashdata_key.':old:'.$key;
+ return $this->userdata($flashdata_key);
+ }
+ else // setting
+ {
+ if (is_array($key))
+ {
+ foreach ($key as $key => $val)
+ {
+ $flashdata_key = $this->flashdata_key.':new:'.$key;
+ $this->userdata($flashdata_key, $val);
+ }
+ }
+ else
+ {
+ $flashdata_key = $this->flashdata_key.':new:'.$key;
+ $this->userdata($flashdata_key, $value);
+ }
+ }
}
// ------------------------------------------------------------------------
Something went wrong with that request. Please try again.