Skip to content

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

Merged
merged 1 commit into from
Feb 28, 2016
Merged

validate type specific options #48

merged 1 commit into from
Feb 28, 2016

Conversation

dbu
Copy link
Collaborator

@dbu dbu commented Feb 26, 2016

@Nyholm trying to do this validation thing

public static function validateAuthenticationType($expected, $actual, $authName)
{
unset($actual['type']);
if ($expected == $actual) {
Copy link
Member

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 ===.

@Nyholm
Copy link
Member

Nyholm commented Feb 26, 2016

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');
Copy link
Member

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?

Copy link
Member

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?

Copy link
Collaborator Author

@dbu dbu Feb 27, 2016 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

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

@dbu dbu force-pushed the dbu-patch-1 branch 3 times, most recently from 6ee6e4c to 875dd51 Compare February 27, 2016 23:10
@dbu
Copy link
Collaborator Author

dbu commented Feb 27, 2016

fixed to use $this, and added the missing tests for the new configuration (and plugin configuration in general)

@Nyholm
Copy link
Member

Nyholm commented Feb 28, 2016

Cool. Thanks!

Nyholm added a commit that referenced this pull request Feb 28, 2016
validate type specific options
@Nyholm Nyholm merged commit f4c051e into authentication Feb 28, 2016
@dbu dbu deleted the dbu-patch-1 branch February 28, 2016 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants