Refactor user agent validator - #145
Conversation
|
Uploaded an initial commit removing the Should the data still be attached to the storage metadata going forward? |
e59a027 to
ecaea56
Compare
| if ($data !== null) { | ||
| /** @var Environment $currentEnvironment */ | ||
| $currentEnvironment = unserialize($data); | ||
| } else { | ||
| $currentEnvironment = Environment::fromGlobals($_SERVER); | ||
| } |
There was a problem hiding this comment.
This block should just be replaced with $currentEnvironment = Environment::fromGlobals($_SERVER);
We are initialising new validators and we should only care what the environment is right now, not any kind of stale or already serialised data
| public static function getServerOption(string $name, ?array $superglobal = null): mixed | ||
| { | ||
| if ($superglobal === null) { | ||
| $superglobal = $_SERVER; | ||
| } | ||
|
|
||
| return $superglobal[$name] ?? null; | ||
| } |
There was a problem hiding this comment.
Why is this method here?
I cannot see any use case for this. This value object should be a wrapper around common environment related/request variables so that we have a predictable API for retrieving the data, therefore any/all data retrieval should be via public properties.
| if ($remoteAddr === null || (isset($options['use_proxy']) && $options['use_proxy'])) { | ||
| $remoteAddr = RemoteAddr::getIpAddress($options, $remoteAddr); | ||
| } |
There was a problem hiding this comment.
Accessing static methods on a validator here is the wrong thing to do - you are making this value object dependent on the validator - RemoteAddr needs the X-ForwardedFor, maybe X-ForwardedProto or whatever - these should be made public properties and simply copied from the given array.
| if ($remoteAddr === null || (isset($options['use_proxy']) && $options['use_proxy'])) { | |
| $remoteAddr = RemoteAddr::getIpAddress($options, $remoteAddr); | |
| } | |
| $forwardedFor = isset($server['X_FORWARDED_FOR']) && is_string($server['X_FORWARDED_FOR']) | |
| ? $server['X_FORWARDED_FOR'] | |
| : null; |
The $options argument should also be dropped
| $validators = $storage->getMetadata('_VALID'); | ||
| $environment = (string) $storage->getMetadata('environment'); | ||
| if ($validators) { | ||
| /** | ||
| * @var class-string<ValidatorInterface> $validator | ||
| */ | ||
| foreach ($validators as $validator => $data) { | ||
| $this->attachValidator('session.validate', [new $validator($data), 'isValid'], 1); | ||
| $currentEnvironment = $data instanceof Environment ? $data : Environment::fromGlobals($_SERVER); | ||
| $this->attachValidator( | ||
| 'session.validate', | ||
| [new $validator(unserialize($environment), $currentEnvironment), 'isValid'], | ||
| 1 | ||
| ); |
There was a problem hiding this comment.
Correct me if I am wrong, but it looks to me like all of this work is already done in SessionManager. Why repeat it?
| @@ -69,11 +81,9 @@ private function attachValidator($event, $callback, $priority) | |||
| array_unshift($callback, $test); | |||
| } | |||
| if ($context instanceof ValidatorInterface) { | |||
There was a problem hiding this comment.
Because the validator chain is specific to this lib, why not change this method signature to attachValidator(Event $event, ValidatorInterface $validator, int $priority) for better type inference? (Possibly better in another patch)
| $data = $context->getData(); | ||
| $name = $context->getName(); | ||
| $this->getStorage()->setMetadata('_VALID', [$name => $data]); | ||
| $this->getStorage()->setMetadata('_VALID', [$name => serialize($context->current)]); |
There was a problem hiding this comment.
Can you check - does setMetadata merge or replace data? If it's a replace operation (which its name suggests) then there could only ever be 1 validator in a re-constituted chain.
I don't really understand why the validators are persisted to metadata at all, as they are constructed inside SessionManager from a list of configuration items right?
| public function getData(): ?string | ||
| { | ||
| return $this->id; | ||
| return (bool) preg_match($pattern, $this->initial->sessionId); |
There was a problem hiding this comment.
Surely you'd want to validate the "current" session id?
|
Based on this comment i've change de flow to only allow attaching validators via config file, the benefit of this is allowing to drop the unused
However the old version allowed users to manually attach validators in other places during the request, if i understand correctly this should no longer be the case, correct? |
|
@alexmerlin Can you too review this one ? We are a bit behind with this package |
|
@SergiuBota1 Let's keep things straightforward and allow adding validators only via the configs. |
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
Signed-off-by: bota <Bota@dotkernel.com>
65542d5 to
b16cc9f
Compare
Unless I'm mistaken this seems to be the case in the current version of this PR |
|
@gsteel I am about to merge this PR , we are quite late with this :-) |
|
@SergiuBota1 LGTM |
5 participants
Description
Pull request based on #133
I closed the previous pull request #136 and opened this one to keep it clean (and a failed rebase). Based on what was discussed I replaced the use of the super-global variable with an object populated from outside.
Do we want the properties in the object to be mutable?
Based on @gsteel comment should we no longer allow
$datato be null?