1313766 - Bugzilla::Bug->send_changes() should not output HTML directly

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Dylan Hardison [:dylan] (he/him)]

process_bug.cgi relies on the fact that send_changes() will output messages regarding how many email updates were sent. This means headers have to be sent early. Throw{Code,Template,User}Error all expect this, but if for some reason ThrowPageError() is called there will be headers in the message body.

The sheer number of other problems this could cause, the number of edge cases... This just needs to be fixed.

Comment 1

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1313766_1.patch

* API CHANGE: bug->send_changes() 
** eturn value is now an arrayref of hashrefs:
** ->{params} (which populate template toolkit vars) and
*** ->{sent_bugmail}. This is only used in bugmail.html.tmpl, and only there for its size.
** None of the callers of this method care about its return value, except process_bug.cgi

* improve program flow in process_bug.cgi
** In most of the code, we send $cgi->header() right before calling $template->process(). I have changed process_bug.cgi to follow this pattern.
** moved sanity check for post_bug_submit_action up to top. Previously, if this code ever was called (e.g. a new value was added to post_bug_submit_action and we forgot to update this script) the result would be part of the normal process_bug.cgi output, followed by the error page output. 
** this still leaves open bug 1314201, but fixing that shouldn't block review of this change at least.
** a minor nit, but somehow template/en/default/bug/process/bugmail.html.tmpl
was using the Template::Plugin::CGI module. I am surprised that this worked, but nevertheless replaced it with [% USE Bugzilla %] and Bugzilla.cgi.


There are still weirdnesses I'd like to fix with this code,
ideally every CGI calls one root template and the template controls its display logic, but that can be done later. This is enough to make CSP work without horrible hacks.

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 8806211 [details] [diff] [review]
1313766_1.patch

Review of attachment 8806211 [details] [diff] [review]:
-----------------------------------------------------------------

r=dkl

Comment 3

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   b15c80d..35671f4  master -> master

Comment 4

[Dylan Hardison [:dylan] (he/him)]

Backing out.

To [e-mail]:mozilla-bteam/bmo.git
   2938094..04e3d20  master -> master

Comment 5

[David Lawrence [:dkl]]

Mainly this is due to print $cgi->header being moved in process_bug.cgi. When verifying a product change using the classic show_bug.cgi, the verification screen is output by Bugzilla::Bug::_set_product and does not print the header itself. If we leave cgi->header near the top of process_bug.cgi as before, or add some sort of check such as 'print $cgi->header if !$cgi->header_done' then this should work along with the other changes.

Note to self: Make sure to QA using classic form as well as modal form :)

dkl

Comment 6

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1313766_2.patch

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 8810452 [details] [diff] [review]
1313766_2.patch

Review of attachment 8810452 [details] [diff] [review]:
-----------------------------------------------------------------

t/002goodperl.t ........... 1/3732 
#   Failed test 'Bugzilla/Error/Template.pm DOES NOT use strict --WARNING'
#   at t/002goodperl.t line 106.

#   Failed test 'Bugzilla/Error/Template.pm DOES NOT use warnings --WARNING'
#   at t/002goodperl.t line 112.
t/002goodperl.t ........... 1224/3732 # Looks like you failed 2 tests of 3732.
t/002goodperl.t ........... Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/3732 subtests

r=dkl with minor error fixed on commit.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   9f141e8..490499f  master -> master

Comment 9

[Dylan Hardison [:dylan] (he/him)]

More test failures, backing out

To github.com:mozilla-bteam/bmo.git
   50fe01e3e..e0040665d  master -> master

Comment 10

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1313766_5.patch

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 8819435 [details] [diff] [review]
1313766_5.patch

r=dkl

Comment 12

[Dylan Hardison [:dylan] (he/him)]

To [e-mail]:mozilla-bteam/bmo.git
   e004066..574643a  master -> master