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

Fix hash ordering bug tickled by perl 5.18.0 #1

Closed
wants to merge 6 commits into from

Conversation

frioux
Copy link

@frioux frioux commented Jul 23, 2013

No description provided.

@anazawa
Copy link

anazawa commented Oct 30, 2013

Your patch doesn't solve the problem. A hashref should be passed to CGI.pm's cookie method:

my $newcookie = $self->query->cookie(%options);

To be surprised, the following code:

$query->cookie( 'foo', 'bar', 'baz', 'qux' );

is equivalent to:

$query->cookie( -name => 'foo', -value => 'baz', -path => 'baz', -domain => 'qux' );

An initial dash is important.

If you pass a hashref to CGI#cookie, the method will work as you expect.

@anazawa
Copy link

anazawa commented Oct 30, 2013

Sorry, i misunderstood. Your patch solves the problem anyway because keys are sorted.
But we don't have to sort keys if we pass a hashref ;)

@frioux
Copy link
Author

frioux commented Oct 30, 2013

Hashref would be fine with me if it will get merged, but I've pretty much given up hope at this point and am looking forward to ditching CGI::Application

@anazawa
Copy link

anazawa commented Oct 30, 2013

I just found your patch here: https://rt.cpan.org/Public/Bug/Display.html?id=81611
and wanted to introduce a better way to fix your problem ;)
I hope your patch will be merged anyway :)

@frioux
Copy link
Author

frioux commented Oct 30, 2013

oh yeah, yours is certainly better, I will admit that :-D

@dsteinbrunner
Copy link

I noticed you got maintainership on CGI-Application-Plugin-DBH to fix issues there. Any chance of doing the same on this module?

@frioux
Copy link
Author

frioux commented Nov 13, 2013

I'd love to, but cees hasn't responded to my emails. Maybe I'll petition for ownership :/

@frioux
Copy link
Author

frioux commented Nov 14, 2013

Ok, I got comaint. I will release this shortly. I switched to a hashref as @anazawa recommended. I'll also grab any low hanging RT's I see.

@frioux frioux closed this Nov 14, 2013
@frioux frioux reopened this Nov 14, 2013
@frioux
Copy link
Author

frioux commented Nov 14, 2013

Arg, ironically neither my patch nor @anazawa 's works on anything OLDER than 5.18. I'm trying to find a real solution other than checking the perl version. Ideas welcome.

@frioux
Copy link
Author

frioux commented Nov 14, 2013

ok fixed for real now. Was a bug in Devel::Cover

@frioux frioux closed this Nov 14, 2013
@anazawa
Copy link

anazawa commented Nov 14, 2013

Thanks for fixing bugs.

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.

None yet

4 participants