304653 - remove 'use Bugzilla::Error' from Util.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

Util.pm should never return an error. About validation routines, it should
return 0 or 1 depending whether the check is successful or not and let the
caller display an error:

check_something(@args) || ThrowUserError();

Comment 1

[Frédéric Buclin]

Attachment: patch, v1

Comment 2

[Dave Miller [:justdave]]

For reference, in case you didn't pick it up from the dependency, this is
solving a dependency loop by fixing this.  The other side effect is it breaks
the assumption by Bugzilla::Util that it's in a CGI environment, effectively
letting the caller decide how to deal with the error in a way that's appropriate
for the environment (so that Bugzilla::Util can be used from command-line
scripts without fear of spewing HTML a the user, for example :)

Comment 3

[Max Kanat-Alexander]

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

>Index: Bugzilla/Util.pm

> sub check_email_syntax {

  Let's just move this to Bugzilla::User instead. check_ functions are supposed
to throw errors. (At least, that's how I was trying to keep things different
between check_ and validate_).

>+sub validate_date {

  And this part looks fine, though I haven't tested it.

Comment 4

[Frédéric Buclin]

(In reply to comment #3)
> > sub check_email_syntax {
> 
>   Let's just move this to Bugzilla::User instead. check_ functions are supposed
> to throw errors. (At least, that's how I was trying to keep things different
> between check_ and validate_).

Sorry, no. I'm opposed to move this function into User.pm. Some files such as
editflagtypes.cgi only want to check that some email address is valid but
neither use a user object nor use any of user properties. And I can easily
imagine a dependency loop such as 'Flag -> FlagType -> User* -> Bug ->
Attachment -> Flag' in the future by doing so. Replacing User* by Util in the
sequence above would break the loop and make sure there is no problem, as we do
actually.

I can rename this function as validate_email_syntax() if this can make you happy. ;)

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

rename check_email_syntax as validate_email_syntax.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 192727 [details] [diff] [review]
patch, v2

r=mkanat on inspection and trusting LpSolit's patch-testing abilities. (He said
he tested it, when I asked.)

Comment 7

[Frédéric Buclin]

Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.44; previous revision: 1.43
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.25; previous revision: 1.24
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.97; previous revision: 1.96
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.123; previous revision: 1.122
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.276; previous revision: 1.275
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.35; previous revision: 1.34
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.107; previous revision: 1.106
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.74; previous revision: 1.73
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.38; previous revision: 1.37
done