Review parameter names in ext/session #6239
Conversation
ext/session/session.stub.php
Outdated
|
|
||
| /** @return int|bool */ | ||
| public function gc(int $maxlifetime); | ||
| public function gc(int $lifetime); |
There was a problem hiding this comment.
Does it still have the same meaning? Could we use TTL? I think lifetime has too widespread usage to change it.
There was a problem hiding this comment.
I like the TTL idea, but there's also the consideration that this is the value specified by session.gc_maxlifetime.
There was a problem hiding this comment.
I left this as $lifetime, but I'm ok to revert this to $maxlifetime or $max_lifetime if you prefer these.
There was a problem hiding this comment.
Mild preference for $max_lifetime from my side.
There was a problem hiding this comment.
OK, seems good for me as well. And I've just pushed a commit with this.
| callable $gc = UNKNOWN, | ||
| callable $create_session_id = UNKNOWN, | ||
| callable $validate_session_id = UNKNOWN, | ||
| callable $update_timestamp = UNKNOWN |
There was a problem hiding this comment.
These are called create_sid(), validateId() and updateTimestamp() in the SessionHandler interface -- yes, complete with inconsistent name casing. I was going to suggest that we should match those names here, but given the casing issue I'm not sure.
There was a problem hiding this comment.
No idea here either. I can revert to using the original names here until we decide it.
ext/session/session.stub.php
Outdated
|
|
||
| /** @return bool */ | ||
| public function write(string $key, string $val); | ||
| public function write(string $id, string $value); |
There was a problem hiding this comment.
| public function write(string $id, string $value); | |
| public function write(string $id, string $data); |
Possibly, as you use $data in session_decode.
There was a problem hiding this comment.
Good idea! We could also use $content maybe (although I prefer data here), as asked in #6238 (comment). Can we then standardize on using $data as a name for similar params? :)
There was a problem hiding this comment.
Yes, I think $data is better than $content. I think you need to change updateTimestamp() to use it as well. At least based on docs, the argument there seems to have the same meaning.
ext/session/session.stub.php
Outdated
|
|
||
| /** @return int|bool */ | ||
| public function gc(int $maxlifetime); | ||
| public function gc(int $lifetime); |
There was a problem hiding this comment.
I like the TTL idea, but there's also the consideration that this is the value specified by session.gc_maxlifetime.
No description provided.