Use Plack::Request instead of CGI::PSGI

[bschmalhofer]

Just a small step toward a more standard usage of Plack and PSGI. Kernel::System::Web::Request currently wraps CGI::PSGI. Switch that over to Plack::Request. There are a couple of instances were a CGI object is passed via the parameter WebRequest into the constructor of Kernel::System::Web::Request. These instances would have to be adapted as well. In future, test scripts should pass in HTTP::Request objects via the parameter HTTPRequest.

[bschmalhofer]

Putting this into the wishlist as moving to Plack::Request requires more effort than expected. By design Plack::Request is immutable. But Kernel::System::Web::Request::LoadFormDraft() wants to modify the underlying query object.

[bschmalhofer]

Did some some experimenting for this issue. Plack::Request is not fully compatible with CGI, so there is a bit of work to do.

• [x] Use the correct methods of Plack::Requestin Kernel::System::Web::Request
• [x] Decide whether the merged parameters can be used, see https://metacpan.org/dist/CGI/view/lib/CGI.pod#Mixing-post-and-url-parameters
• [x] Create a new issue concerning large file uploads, formerly checked by $CGI::POST_MAX
• [x] Fix the support for drafts
• [x] Expose the method Plack::Request::content() as Kernel::System::Web::Request::Content
• [x] Correctly support POSTDATA, PUTDATA, PATCHDATA as a legacy interface, see https://metacpan.org/dist/CGI/view/lib/CGI.pod#Handling-non-urlencoded-arguments
• [x] Look at the case where a modfied draft is stored as a copy
• [x] Fix support for uploads
• [x] Resolve the TODO comments before merging the changes
• [x] fix scripts/test/GenericInterface/Transport/HTTP/SOAP/ContentCharset.t
• [x] Resolve failures in the changed test scripts
• [x] add test for GetParamNames() with a file upload field, reproducing the behaviour from rel-10_1
• [x] add a method SetParam()

[bschmalhofer]

Reopening, as I still like the idea.

[bschmalhofer]

Here is the current strategy for eliminating CGI.pm:

• Plack::Request replaces CGI.pm as the internal object used in Kernel::System::Web::Request
• use HTTP::Request objects for setting up $ParamObject in test scripts
  • HTTP::Message::PSGI::req_to_psgi() then creates a PSGI env from the request
  • SCRIPT_NAME and PATH_INFO are set up like in otobo.psgi
• GET('/') is used as the fallback HTTP::Request object
• divergence from rel-10_1: URL and body parameters are always mixed, URL params come before body params
• Body parameters are only considered when Content-Type is set accordingly, not sure how CGI.pm does it
• For single value parameters the first value is taken, like in CGI.pm, divergence from rel-10_1: body params are not guaranteed to win -GetParam() with POSTDATA, PUTDATA, and PATCHDATA is still supported
• new method Kernel::System::Web::Request::Content() for getting the request body
• GetParam() still returns the file name for file upload fields
• For supporting drafts, $Env->{'plack.request.merged'} is meddled with, more or less like setting values with CGI::param()

[bschmalhofer]

The fallout from the test suite is now down to three failures:

Test Summary Report
-------------------
/opt/otobo/scripts/test/Compile.t                                                                         (Wstat: 0 Tests: 1775 Failed: 0)
  TODO passed:   1137, 1234
/opt/otobo/scripts/test/Selenium/Agent/Admin/ProcessManagement/AdminProcessManagementTransitionAction.t   (Wstat: 768 (exited 3) Tests: 87 Failed: 3)
  Failed tests:  81, 84, 87
  Non-zero exit status: 3
/opt/otobo/scripts/test/Selenium/Agent/AgentTicketService.t                                               (Wstat: 256 (exited 1) Tests: 42 Failed: 1)
  Failed test:  42
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/Output/Ticket/OverviewMedium.t                                           (Wstat: 256 (exited 1) Tests: 29 Failed: 1)
  Failed test:  29
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/Output/Ticket/OverviewPreview.t                                          (Wstat: 256 (exited 1) Tests: 35 Failed: 1)
  Failed test:  35
  Non-zero exit status: 1
/opt/otobo/scripts/test/Selenium/TestingMethods.t                                                         (Wstat: 0 Tests: 36 Failed: 0)
  TODO passed:   21, 24, 28
Files=959, Tests=80558, 5543 wallclock secs (26.94 usr  3.11 sys + 1521.04 cusr 214.30 csys = 1765.39 CPU)
Result: FAIL
ran tests for product OTOBO 11.0.x on host d05d0f2bdf0d .

I investigated what the failure of AgentTicketService.t is about. The cause is a hidden parameter with a redirect URL: <input type="hidden" name="RedirectURL" value="Action=AgentTicketService;Filter=Unlocked;View=Medium;ServiceID=2;SortBy=Age;OrderBy=Up;View=Small;"/> That the parameter View is given twice, seems to be an accident. But the point is that the code apparently relies on that the first declaration View=Medium takes precedence over the second declaration View=Small. So let's take the extra step and let GetParam() return the first value when there are multiple values.

[bschmalhofer]

I discussed this with @svenoe and we decided to go ahead with these changes. The first PR is merged. Some TODOs are still open.

[bschmalhofer]

It is not obvious how the SysConfig Setting WebMaxFileUpload should be enforced on the server side. Plack::Request used HTTP::Entity::Parser which seem to honor the Content-Length header but does not support a hard limit. Maybe a PSGI Middleware can help here. See #2103 for details.

[bschmalhofer]

The failures in scripts/test/GenericInterface/Transport/HTTP/SOAP/ContentCharset.t seem to be an artefact of the test script. In #320, from me @bschmalhofer, a nasty trick was introduced for making the test script pass.

In the script there used to be:

    # Fake STDIN and fill it with the request.
    local *STDIN;
    open STDIN, '<:encoding(UTF-8)', \$Request;    ## no critic qw(OTOBO::ProhibitOpen)

This forced CGI.pm to decode the content as it was read STDIN. Thus the test script always got a decoded string for comparison and the comparison with the Perl strings from the script worked. But in the real world UTF-8 encoded strings are sent over the wire and the processing has no charset information that the content is UTF-8 encoded. Thus the processing has no reason to decode the content. Thus the comparison must be done with the encoded strings. The conversion to HTTP::Request removed the nasty workaround and there were failures again. The new version is hopefully more correct.

[bschmalhofer]

PR is merged. Tests look fine now. The open issues have been adressed. Closing this issue.

Test Summary Report
-------------------
/opt/otobo/scripts/test/Compile.t                                                                         (Wstat: 0 Tests: 1775 Failed: 0)
  TODO passed:   1137, 1234
/opt/otobo/scripts/test/Selenium/Agent/Admin/ProcessManagement/AdminProcessManagementTransitionAction.t   (Wstat: 768 (exited 3) Tests: 87 Failed: 3)
  Failed tests:  81, 84, 87
  Non-zero exit status: 3
/opt/otobo/scripts/test/Selenium/TestingMethods.t                                                         (Wstat: 0 Tests: 36 Failed: 0)
  TODO passed:   21, 24, 28
Files=959, Tests=80732, 5878 wallclock secs (28.15 usr  2.99 sys + 1603.95 cusr 225.94 csys = 1861.03 CPU)
Result: FAIL
ran tests for product OTOBO 11.0.x on host c86ad2058931 .