SyncAnyPendingShadowChanges() should not be called from a template

RESOLVED FIXED in Bugzilla 2.18

Status

()

RESOLVED FIXED
17 years ago
6 years ago

People

(Reporter: justdave, Assigned: bbaetz)

Tracking

2.15
Bugzilla 2.18

Details

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.
Target Milestone: --- → Bugzilla 2.16
(Assignee)

Comment 1

17 years ago
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.
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
sub EmitTemplateToBrowser ($$) {
    my ($contenttype, $templatepath) = @_;
    print "Content-type: $contenttype\n\n";
    $template->process($templatepath, $vars)
      || ThrowTemplateError($template->error());
    SyncAnyPendingShadowChanges();
}
(Assignee)

Comment 4

17 years ago
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 ?
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
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.
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
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
(Assignee)

Comment 8

17 years ago
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
Depends on: 124589
I would prefer fixing this somehow for 2.16 because this would involve requiring
people to change their templates in future versions.
(Assignee)

Comment 10

17 years ago
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
[% 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
>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.
I'm with Myk...  the suggested line would work great.  As long as it's temporary
(which it is) I care not. :-)
The fix suggested is being implemented over in bug 140435.

Gerv
(Assignee)

Updated

16 years ago
Depends on: 180870
(Assignee)

Updated

16 years ago
No longer depends on: 124589
(Assignee)

Comment 15

16 years ago
This is fixed in my patch for bug 180870.

Removing the funciton ahs to be the best solution, right? ;)
Assignee: justdave → bbaetz
(Assignee)

Comment 16

16 years ago
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
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.