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

Fixed empty HTTP_REFERER calls. #505

Closed
wants to merge 3 commits into from
Closed

Fixed empty HTTP_REFERER calls. #505

wants to merge 3 commits into from

Conversation

b10m
Copy link
Contributor

@b10m b10m commented Nov 13, 2013

When HTTP_REFERER is not set (which is a quite likely scenario), dancer
would die with:
isa check for "referer" failed: undef is not a string!

This forces request->referer to return an empty string, rather than
undef (which is not a Str).

When HTTP_REFERER is not set (which is a quite likely scenario), dancer
would die with:
    isa check for "referer" failed: undef is not a string!

This forces request->referer to return an empty string, rather than
undef (which is not a Str).
@veryrusty
Copy link
Member

@b10m nice pickup on the behaviour of those request object attributes when their corresponding request header doesn't exist!

I'm wondering why you elected to return an empty string vs altering the data type on those accessors?
( i.e. define them as isa => Maybe[Str] to allow for the possibility of undef values )

@b10m
Copy link
Contributor Author

b10m commented Nov 18, 2013

I needed a quick hack and didn't know about Maybe[Str]. I don't really have a strong preference either way, as long as the bug gets killed.

@veryrusty
Copy link
Member

My preference would be for changing the type on those attributes. That way Request->referer and Request->headers->header('Referer') are consistent. @b10m do you want to do another pull request?

    When HTTP_REFERER is not set (which is a quite likely scenario), dancer
    would die with:
        isa check for "referer" failed: undef is not a string!
@b10m
Copy link
Contributor Author

b10m commented Nov 19, 2013

You make a solid point. Code patched and pushed.

@veryrusty
Copy link
Member

Looks good. It's a 👍 from me..

@xsawyerx
Copy link
Member

xsawyerx commented Dec 7, 2013

👍 Good work!

@xsawyerx
Copy link
Member

xsawyerx commented Dec 7, 2013

Merged, thanks! :)

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