Are you sure this is because of the Redis Store? I don't see anything suggesting that in the backtrace. I'm having a hard time digging up what $this->redis->get returns on error, false or null. It seems like it shouldn't return either on error, but rather one or the other consistently. Thoughts?

$this->redis->get should return false on error and null on 'key not found'..

Regarding the commit:
Bear in mind that return unserialize(null); would return false and that other code may actually depend on this behaviour...

Ahh, thanks for the context. This PR makes sense to me, then, as I believe that's the way this function was originally intended to behave:

@return mixed|null The value associated with that key, or null if there's no such key.

As for backwards compatibility, ugh, we've put ourselves in the unfortunate situation of reversing the error case return values. On error this function returns null and when the key isn't found it returns false because of unserialize!

Another possible solution would be to update the documentation to include the case where false is returned. However, I think I'd prefer to accept this PR at the cost of backwards incompatibility. That way we maintain the interface contract in the Store class in lib/SimpleSAML/Store.php of @return mixed|null The value. Changing all implementations to include the possibility of returning false seems like a much more heavy-handed solution with more backwards incompatible implications.

Thoughts?

I think you're mixing it up...
Consider the return-value of $this->redis->get returning false on error and null on key not found:

Old situation:
On error (false) this function returns null (because of if-statement)
On key n/f (null) this function returns false (because of unserialize)

New situation:
Both cases (null/false) would return null

Since support for redis is relatively new, I think it's worth checking out if any code relies on \Redis\:get() returning false and if we need to change any tests.

I spent some time looking at the other store types (memcache, sql) and they do not appear to return false, so if the Redis plugin was to return false it would be inconsistent with the other types.

Great, thanks @matt-catalyst! One suggestion I have, though, would be to make getMocked in RedisTest look like this instead:

public function getMocked($key)
{
    return array_key_exists($key, $this->config) ? $this->config[$key] : null;
}

I think this is more true to the actual API, where get should return null when the key doesn't exist, and false on error. Testing both the null and false error cases seems like more trouble than it's worth for now.

Thanks for the suggestion @mschwager - I have updated the pull request.

This LGTM! Can we pull this in @tvdijen?

Fine by me!

Ahh, I was under the impression that @tvdijen had committer's access, but maybe that's not the case? If not, can we pull this in @jaimeperez or @thijskh?

Thanks a lot for reporting and fixing this, @matt-catalyst!

As for the lack of write access of @tvdijen, I've just fixed that too 😉