140629 - SyncAnyPendingShadowChanges() should not be called from a template

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Dave Miller [:justdave]]

SyncAnyPendingShadowChanges() recently got moved out of PutFooter() into the
footer template.  Personally I think this is a Bad Idea because this is a code
process issue and not a user interface issue, and we shouldn't be depending on a
template to do this.  This should be in the Perl code.

Comment 1

[Bradley Baetz (:bbaetz)]

The problem is that there is no requirement for PutFooter to be called. Besides,
this will vanish once we get rid of the shadowlog.

ccing people from bug 140121.

Comment 2

[Gervase Markham [:gerv]]

What other code is executed at the end of each CGI apart from PutFooter()? The 
only other option is to use a general "end of CGI - cleanup" callout, and call 
SyncBlah from that, but that doesn't really change things.

And the shadowdb is going away, as bbaetz said. I think this is a reasonable 
hack for 2.16

Actually, we might want to do

[% CALL SyncAnyPendingShadowChanges IF SyncAnyPendingShadowChanges %]

that way, we won't need a dummy function to avoid custom templates breaking when 
the function goes away.

Gerv

Comment 3

[Matthew Tuck [:CodeMachine]]

sub EmitTemplateToBrowser ($$) {
    my ($contenttype, $templatepath) = @_;
    print "Content-type: $contenttype\n\n";
    $template->process($templatepath, $vars)
      || ThrowTemplateError($template->error());
    SyncAnyPendingShadowChanges();
}

Comment 4

[Bradley Baetz (:bbaetz)]

That's probably not a bad idea anyway - I know that I've been thinking we should
centralise all these things.

ProcessTemplate($$), maybe, to match $template->process ?

Comment 5

[Gervase Markham [:gerv]]

That won't be right, because SyncAnyPendingShadowChanges should not be called
whenever we do a template. It should only be called at the end of a CGI, as it
is in the current solution. Myk actually thinks this is too often, but he
doesn't want to change this because the shadowdb is about to go away.

We could only use this new sub to process the template when we actually wanted
to call SyncAnyPendingShadowChanges, but that would mean some templates are
invoked one way and some are invoked the other.

The shadowdb is about to go away, and we should do whichever solution means it
goes away with least pain and code rearrangement, and doesn't leave code
redundancy. Unless there are other good reason to have the subroutine you mention, 
[% CALL SyncAnyPendingShadowChanges IF SyncAnyPendingShadowChanges %]
is the solution that best fits this criteria. Because when
SyncAnyPendingShadowChanges goes away, nothing else needs changing.

Gerv

Comment 6

[Dave Miller [:justdave]]

I have no argument against leaving this in for 2.16, as long as it's going away
eventually.  I just thought it set a bad precident since it's an obvious
violation of the current developer guidelines.

Comment 7

[Gervase Markham [:gerv]]

I agree it's certainly not ideal; however IMO it's the cleanest solution to an
annoying problem.

Let's push this out to 2.18 - the correct fix for this bug is to remove the
shadowdb. Is there a bug on that we can make this one have a relationship with?

Gerv

Comment 8

[Bradley Baetz (:bbaetz)]

bug 124589 is replication. Note that if someone else actually uses the shadowdb,
then we'll probably still need to support it for 2.18. If you know of a large
installation which may do so, comment in that bug and I'll contact them and
check if they are.

Bradley

Comment 9

[Matthew Tuck [:CodeMachine]]

I would prefer fixing this somehow for 2.16 because this would involve requiring
people to change their templates in future versions.

Comment 10

[Bradley Baetz (:bbaetz)]

Why don't we call it from quitly_check_login, as a tmeporary work arround? call
it one in every 'n' times, or if the userid is divisiable by 3, or something?
bmo is planning to use replication soon, so myk can tweak those numbers manually
until then

Comment 11

[Gervase Markham [:gerv]]

[% CALL SyncAnyPendingShadowChanges IF SyncAnyPendingShadowChanges %]
would not require people to change their templates in future versions. If this
function no longer exists, this template line does nothing.

Calling it from quietly_check_login would mean it's called at the top of CGIs
instead of the bottom. If this doesn't make a difference, we could do that.

Gerv

Comment 12

[Myk Melez [:myk] [@mykmelez]]

>Calling it from quietly_check_login would mean it's called at the top of CGIs
>instead of the bottom. If this doesn't make a difference, we could do that.

It does make a difference.  In some cases, Bugzilla pauses for a while in this
function.  That's only a minor problem at the end of a script/template (because
the user still gets the whole page, even though the browser tells them it isn't
finished), but it would be a major problem at the beginning.

We're trying to ship 2.16, so it's a bad idea to make riskier changes than are
absolutely necessary.  Gerv's solution is the right one for now; we can remove
this function in 2.18 or 2.20 when we remove shadow database functionality.

Comment 13

[Dave Miller [:justdave]]

I'm with Myk...  the suggested line would work great.  As long as it's temporary
(which it is) I care not. :-)

Comment 14

[Gervase Markham [:gerv]]

The fix suggested is being implemented over in bug 140435.

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

This is fixed in my patch for bug 180870.

Removing the funciton ahs to be the best solution, right? ;)

Comment 16

[Bradley Baetz (:bbaetz)]

FIXED cause it isn't called from teh footer anymore - in fact, it isn't called
from anywhere, now that MySQL handles replication for us