327914 - Make Bugzilla::Util::perform_substs() go away

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Marc Schumann [:Wurblzap]]

When creating e-mails from their template params, variable substitution crashes badly if the variable to be substituted is neither a pre-defined variable nor a Bugzilla param, saying that an internal error occurred in Bugzilla::Config::Param. This may be all right for calls to Bugzilla::Config::Param() from "normal" code, but it is misleading for e-mail template params. For e-mail template params, the real cause will usually be a typo in the param's text (or similar error).

The reason is that Bugzilla::Util::perform_substs calls Bugzilla::Config::Param without validating the substitution variable names, *and* Bugzilla::Config::Param die()s if the param name it's passed doesn't exist.

Suggestion:
Firstly, add an optional second param to Bugzilla::Config::Param, which, when defined, causes it not to die() but return undef when it can't find a param. Secondly, make Bugzilla::Util::perform_substs use the new optional second param, and fall back to the original variable substitution string if param substitution fails.

In 2.18 and 2.20, the sub is called PerformSubsts and lives in Bugzilla::BugMail.

Comment 1

[Marc Schumann [:Wurblzap]]

This is a FAQ which I think should be gone with 3.0.

Comment 2

[Frédéric Buclin]

perform_substs() is no longer used by email templates. The single place where it is still used is showdependencygraph.cgi. So I don't see how this could be a blocker, especially now that Param() has been replaced by Bugzilla->params->{} which never fails.

Comment 3

[Frédéric Buclin]

Unless I miss something, perform_substs() can go away. It's only used here:

my $webdotbase = Bugzilla->params->{'webdotbase'};

if ($webdotbase =~ /^https?:/) {
     # Remote dot server
     my $url = perform_substs($webdotbase) . $filename;


We should write:

    my $url = Bugzilla->params->{'webdotbase'} . $filename;

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> Unless I miss something, perform_substs() can go away. It's only used here:
> 
> We should write:
> 
>     my $url = Bugzilla->params->{'webdotbase'} . $filename;

  You have to replace the %urlbase% in the default webdotbase. But yes, other than that, we can remove perform_substs.

Comment 5

[Marc Schumann [:Wurblzap]]

Cool! Not a blocker then. Should we morph this bug into "make perform_substs go away" or something?

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> Should we morph this bug into "make perform_substs go
> away" or something?

Yes!

Comment 7

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 8

[Frédéric Buclin]

Attachment: patch, v1

Comment 9

[Max Kanat-Alexander]

Comment on attachment 272487 [details] [diff] [review]
patch, v1

r=mkanat by inspection

Comment 10

[Frédéric Buclin]

Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.60; previous revision: 1.59
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.58; previous revision: 1.57
done