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

QUERY is no longer supported as an argument to psgi_app() #7

Open
ichesnokov opened this issue May 21, 2019 · 6 comments
Open

QUERY is no longer supported as an argument to psgi_app() #7

ichesnokov opened this issue May 21, 2019 · 6 comments

Comments

@ichesnokov
Copy link

ichesnokov commented May 21, 2019

We cannot pass QUERY as an argument to psgi_app() anymore (after an adoption of code from markstos#17) because it will be overwritten by a new CGI::PSGI object.
There were some use cases where this was useful - e.g. when psgi_app() was used as a helper method for calling new() and run_as_psgi() on every request.

So I'd suggest to document it at least.
Documentation that shows usage of psgi_app() needs fixing as well.

As a side note, I don't completely see why QUERY parameter was chosen as a way to provide CGI::PSGI object to CGI::Application - because the more "natural" (and documented) way to do that is apparently overriding of cgiapp_get_query() method. It should be documented which of them has a precedence, if nothing else.

@MaxPerl
Copy link

MaxPerl commented Jun 8, 2019

Dear ichesnokov,
I agree with you, that the docs could be a little bit more clearer, although I could read out the (new) behaviour of CGI::App here. See my pull request for a try to make the docs clearer on this topic...

@ichesnokov
Copy link
Author

Hi @MaxPerl, thanks for the provided updates.
I checked your changes and added some comments - please take a look.

@ichesnokov
Copy link
Author

Additionally it has to be mentioned somewhere that cgiapp_get_query() doesn't work anymore for psgi_app() (oh, gosh).

@ichesnokov
Copy link
Author

ichesnokov commented Jun 10, 2019

Essentially I think that forced override of QUERY parameter is unacceptable because it brings too many incompatibilities with the existing code, behavior and docs and should be reverted. Instead, cgiapp_get_query() should be hacked to provide CGI::PSGI object on every request.

@MaxPerl
Copy link

MaxPerl commented Jun 10, 2019

Dear ichesnokov,
Now I am understanding your concerns. I totally agree with you. psgi_app and run_as_psgi should behave in the same manner (that means doesn't create a new query object on each request). I don't understand the bug report at markstos#17, too. I run my plack app from plackup, too, and have no problem, that the POST form data being the same each request. Perhaps a test script would be nice, to see, whether the problem really is a problem of CGI::App?

On the other hand, for me passing a custom query object through the QUERY parameter is more natural than ovveriding cgiapp_get_query. So both options should be supported also in a Plack environment.

@allter
Copy link

allter commented Jan 27, 2020

This patch was written long ago.
The problem was is that sub ( $env ) is meant to fire on each request, but the first time it's fired, it added QUERY to the $args_to new.

I would rewrite this now as follows:

        my $args_to_new2 = $args_to_new;
        if (not defined $args_to_new2->{QUERY}) {
            require CGI::PSGI;
            $args_to_new2->{QUERY} = CGI::PSGI->new($env);
        }

        my $webapp = $class->new($args_to_new2);

This way it both satisfies the docs and the people that worked around by using psgi_app as request handler.

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

No branches or pull requests

3 participants