3
\$\begingroup\$

I have this code

//TRIGGER MULTIPLE VIEWS, WALL AND FORUM
if(isset($_COOKIE["projects-sorting"])) {
    $_SESSION['projectsDisplayMode'] = $_COOKIE["projects-sorting"];
}
if(isSet($_GET['mode'])) {
    $_SESSION['projectsDisplayMode'] = $_GET['mode'];
}
if(!isSet($_SESSION['projectsDisplayMode'])) {
    $_SESSION['projectsDisplayMode'] = "wall"; //default is wall mode
}
$mode = $_SESSION['projectsDisplayMode'];
setcookie("projects-sorting", $mode, time() + (86400 * 30), "/"); // 86400 = 1 day

Which to me looks overly complicated, but it does work.

What it does:

  • Checks if a specific cookie exists, if it does, set session variable projectsDisplayMode to whatever that cookie's content is.
  • Check if _GET variable mode exists, if it does, set projectsDisplayMode to whatever that variable is (Would overwrite the last variable set if both exist)
  • Last check if the session variable is set at all, if it's not set it to "wall", as that is the default.
  • Now set $mode (A specific page variable) to be processed a bit more to show the page properly.

I'm not concerned with the afterwards processing, I just want to see if there is a better way that I can do the first bit that may be faster or more standard, or more pleasant to look at.

\$\endgroup\$
0
2
\$\begingroup\$

You only really need one if. IMO something like this would be easier to read.

if(!empty($_GET['mode'])){
    $mode = $_GET['mode'];
}else{ 
    $mode = !empty($_COOKIE["projects-sorting"]) ? $_COOKIE["projects-sorting"] : "wall"; 
}
setcookie("projects-sorting", $mode, time() + (86400 * 30), "/");
$_SESSION['projectsDisplayMode'] = $mode;

At the end of this, $_SESSION['projectsDisplayMode'] and $_COOKIE["projects-sorting"] will always be the same value, so I would omit the former. You really shouldn't use session to store intermediate values anyway. You should be setting session to the value of $mode, not vice versa.

\$\endgroup\$
5
  • \$\begingroup\$ Your right, looking at this now I'm not really sure why I used sessions in the first place. \$\endgroup\$ Jun 23 '17 at 18:43
  • \$\begingroup\$ So the problem with your code and why it won't work for me, is there is no way to override the current display settings. The way it was done before was the GET variable, it literally just changed it between wall and forum, but with the c ode that you submitted there is no way to take it off of wall mode. But this is easily fixed by checking if the GET variable isset when I check for the cookie, e.g if((!empty($_COOKIE["myticketroll-projects-sorting"])) && (!isSet($_GET['mode']))){ \$\endgroup\$ Jun 23 '17 at 18:57
  • \$\begingroup\$ @GrumpyCrouton - You're right. I swapped it to check GET first. Please try updated code. \$\endgroup\$ Jun 23 '17 at 19:18
  • \$\begingroup\$ What's the difference between checking if the variable is empty vs checking if it's set? isset() vs empty()? \$\endgroup\$ Jun 23 '17 at 20:54
  • \$\begingroup\$ empty will return true and not throw an error if the variable is undefined. it will also return true if the variable is defined and set to 0 or empty string or null. \$\endgroup\$ Jun 23 '17 at 20:58
2
\$\begingroup\$

First I think that you need to reconcile your use of session vs. cookie. In your answer you explain technically what is happening in your code, but not what the overall user experience requirement is. You need to match your usage of cookies vs. sessions vs. HTML local storage (though not used here), which are all means to convey application state to what the actual user experience needs of the application are.

Does the cookie represent a default user preference? If so, then should you be overriding it when the user passes parameters or otherwise changes mode setting in a session only context?

Why use session at all if you are always going to set a new "mode" cookie which overrides whatever is in session and there is not a session-specific override context? Most likely you are propagating session ID by cookie anyway, so if user deletes cookies, you are going to lose state anyway.

If for example you remove session from the equation altogether, you may get something like:

$mode = 'wall';
if(!empty($_COOKIE['projects-sorting'])) {
    $mode = $_COOKIE['projects-sorting'];
}
if(!empty($_GET['mode'])) {
    $mode = $_GET['mode'];
}
if($mode !== $_COOKIE['projects-sorting']) {
    setcookie("projects-sorting", $mode, time() + (86400 * 30), "/");
}

You are doing nothing in code shown here to validate/sanitize the inputs. Both $_COOKIE and $_GET are use input and should be validated/sanitized accordingly before passing these values further into the application (or especially placing back into user cookie).

Maybe something as simple as validateMode() function.

function validateMode($array, $key) {
    if(empty($array[$key]) || !is_string($array[$key])) {
        return false;
    }
    switch($array[$key]) {
        case 'wall':
        case 'forum':
            return true;
        default:
            return false;
    }
}

And usage with my suggestion like:

$mode = 'wall';
if(validateMode($_COOKIE, 'projects-sorting')) {
    $mode = $_COOKIE['projects-sorting'];
}
if(validateMode($_GET, 'mode')) {
    $mode = $_GET['mode'];
}
if($mode !== $_COOKIE['projects-sorting']) {
    setcookie("projects-sorting", $mode, time() + (86400 * 30), "/");
}
\$\endgroup\$
5
  • \$\begingroup\$ missing a closing brace and a closing quote in your function. and that function doesn't make a lot of sense anyway because the empty check needs to be done before you pass an undefined index of $_COOKIE to a function, causing php to throw a notice. i would also consider the impact on readability of creating a function that will only be used once. that's unnecessary scrolling for future devs. \$\endgroup\$ Jun 23 '17 at 17:40
  • \$\begingroup\$ @Iwrestledabearonce. Thanks for pointing out syntax issues. Not doing this in IDE when you making these responses, so these things get missed from time to time :) WRT validateMode() implementation, I guess this really depends on how one would intend to use such a method and how one typically works with input validation in their application. The usage I present here is perhaps more similar to how one would use popular PHP functions like filter_var(), filter_input(), filter_input_array() where the filter/.validation function is the first point of ingestion of the variable. \$\endgroup\$
    – Mike Brant
    Jun 23 '17 at 18:04
  • \$\begingroup\$ yeah, bummer php doesn't handle undefined like javascript does. imo though, this seems like a perfectly safe and reasonable case for the stfu operator :P \$\endgroup\$ Jun 23 '17 at 18:19
  • \$\begingroup\$ @Iwrestledabearonce. PHP does have error suppression operator (which one should be heavily biased against using) but you are right that this sample could generate undefined index notice. This would probably be best implemented as validateMode($array, $key) (with usage of validateMode($_COOKIE, 'projects-sorting') to work around that issue (this is in essence how filter_input() works. I have updated my example to reflect this change. Of course at point, one might just want to use filter_input() with regex or custom callback validation. \$\endgroup\$
    – Mike Brant
    Jun 23 '17 at 20:05
  • \$\begingroup\$ you're right. that's a better idea. \$\endgroup\$ Jun 23 '17 at 21:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.