-
Notifications
You must be signed in to change notification settings - Fork 50
validate type specific options #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
public static function validateAuthenticationType($expected, $actual, $authName) | ||
{ | ||
unset($actual['type']); | ||
if ($expected == $actual) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make a comment that you actually intend to do == instead of ===.
Cool. I like it. =) |
if (empty($config['username']) || empty($config['password'])) { | ||
throw new InvalidConfigurationException('Authentication "basic" requires both "username" and "password".'); | ||
} | ||
Configuration::validateAuthenticationType(['username', 'password'], array_keys($config), 'basic'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be self::validateAuthenticationType
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stupid question: since it is a closure, why can't it be $this->validateAuthenticationType
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using $this in a closure has been supported since php 5.4. I just tried this: https://3v4l.org/gvIgS
6ee6e4c
to
875dd51
Compare
fixed to use $this, and added the missing tests for the new configuration (and plugin configuration in general) |
Cool. Thanks! |
@Nyholm trying to do this validation thing