Skip to content
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

allow null setting for domain #65

Merged
merged 4 commits into from
May 4, 2015
Merged

allow null setting for domain #65

merged 4 commits into from
May 4, 2015

Conversation

mshick
Copy link
Contributor

@mshick mshick commented Apr 20, 2015

Per #64 allows domains to be set to null an allowed value for server.state.

@hueniverse hueniverse added the feature New functionality or improvement label Apr 20, 2015
@gansbrest
Copy link

yep, +1 )

@@ -57,7 +57,7 @@ internals.implementation = function (server, options) {
cookieOptions.ttl = settings.ttl;
}

if (settings.domain) {
if (settings.hasOwnProperty('domain')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since Statehood sets domain to null via defaults, this isn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, that makes sense. It's worth pointing out though, that with the validation happening first there's no way to trigger that branch of logic -- no falsey value will pass validation for domain, presently.

@jaw187
Copy link
Contributor

jaw187 commented Apr 30, 2015

@mshick include changes for redirectTo as well and any other property which you need to send falsey values for.

@jaw187 jaw187 added this to the 2.1.1 milestone Apr 30, 2015
@jaw187
Copy link
Contributor

jaw187 commented Apr 30, 2015

@gansbrest Do you have an specific property and falsey value which you need support for?

@mshick
Copy link
Contributor Author

mshick commented Apr 30, 2015

Okay, will do. I’ll push up the validation change in a sec, along with a test.

I don’t mean to badger you. If redirectTo isn’t a change you want to make I understand.

On Apr 30, 2015, at 10:04 AM, James Weston notifications@github.com wrote:

@mshick https://github.com/mshick include changes for redirectTo as well and any other property which you need to send falsey values for.


Reply to this email directly or view it on GitHub #65 (comment).

@hofan41
Copy link
Contributor

hofan41 commented Apr 30, 2015

Should domain also be changed to allow false?

@mshick
Copy link
Contributor Author

mshick commented Apr 30, 2015

Null is allowed by statehood, false is not. That's the rationale -- consistency.

On Apr 30, 2015, at 1:49 PM, Ho-Fan Kang notifications@github.com wrote:

Should domain also be changed to allow false?


Reply to this email directly or view it on GitHub.

path: Joi.string().default('/'),
clearInvalid: Joi.boolean().default(false),
keepAlive: Joi.boolean().default(false),
isSecure: Joi.boolean().default(true),
isHttpOnly: Joi.boolean().default(true),
redirectTo: Joi.string(),
redirectTo: Joi.string().allow(false),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be allow('')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used false to be consistent with the docs for redirectTo at the route level:

 To set an individual route to use or disable redirections, use the route plugins config ({ config: { plugins: { 'hapi-auth-cookie': { redirectTo: false } } } }). Defaults to no redirection.

In light of that, would you still like me to change it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, the allow value will override the data type.

> Joi.validate(false, Joi.string().allow(false))
{ error: null, value: false }

jaw187 added a commit that referenced this pull request May 4, 2015
allow null setting for domain
@jaw187 jaw187 merged commit bc76bb4 into hapijs:master May 4, 2015
@lock
Copy link

lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature New functionality or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants