904568 - emails generated by jobqueue.pl unable to reference custom fields

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[:glob ✱]

since bug 877078, any email templates which reference custom fields throw errors if the jobqueue is enabled.

this is because the new_from_list constructor doesn't trigger one of the spots that Bugzilla::Bug uses to call _create_cf_accessors.

Comment 1

[:glob ✱]

Attachment: 904568_1.patch

Comment 2

[Frédéric Buclin]

Comment on attachment 789553 [details] [diff] [review]
904568_1.patch

>=== modified file 'Bugzilla/Object.pm'

> sub new {

>+    if (my $cache_key = $class->cache_key($param)) {
>+        ...
>+        return $object;
>+    }
>+    else {
>+        # no caching
>+        return $class->new_from_hash($class->_load_from_db($param));
>+    }
>     my $object   = $class->_init(@_);
>     bless($object, $class) if $object;
>     return $object;

The last 3 lines are never reached due to the if-else block. Either that's not intentional and that's a bug, or these lines must go away.

Note that the else {} block is useless.

if () {
}
return ...

works too.

Comment 3

[:glob ✱]

Attachment: 904568_2.patch

Comment 4

[Frédéric Buclin]

Comment on attachment 789561 [details] [diff] [review]
904568_2.patch

>=== modified file 'Bugzilla/Object.pm'

> sub new {

>+    if (my $cache_key = $class->cache_key($param)) {
>+        # cached objects
>+        my $object = $class->_cache_get($param);
>+        return $object if $object;
>+
>+        my $object_data = $class->_load_from_db($param)
>+            or return;
>+        $object = $class->new_from_hash($object_data)
>+            or return;
>+        $class->_cache_set($param, $object);
>+
>+        return $object;
>+    }
>+    else {
>+        # no caching
>+        return $class->new_from_hash($class->_load_from_db($param));
>+    }

You are duplicating code and also waste some cycles by calling $class->cache_key() 3 times in a row (explicitly here, then again in _cache_get() and in _cache_set()). It would IMO be cleaner to simply write:

  my $object = $class->_cache_get($param);
  return $object if $object;

  $object = $class->new_from_hash($class->_load_from_db($param));
  $class->_cache_set($param, $object);

  return $object;


No need for a if-else block as _cache_get() already calls cache_key() for you and does nothing if there is no such key. Same for _cache_set(). Also, new_from_hash() does nothing if $object_data is undefined so you can combine both as you did in the else block.

Note that I didn't test your patch at all as I don't use jobqueue. This review was only based on the code itself.

Comment 5

[Simon Green]

Comment on attachment 789561 [details] [diff] [review]
904568_2.patch

What Frédéric said.

Comment 6

[:glob ✱]

Attachment: 904568_3.patch

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #6)
> Created attachment 792005 [details] [diff] [review]
> 904568_3.patch

Simon, it would be nice to this into the 4.5.1 release we would like to do soon. Will you be able to look at this soon?

Thanks
dkl

Comment 8

[Simon Green]

Comment on attachment 792005 [details] [diff] [review]
904568_3.patch

nit: line 93/4 in Bugzilla::Object needs to be changed, since the name of the sub changed
          || ThrowCodeError('param_must_be_numeric',
                            {function => $class . '::_init'});

can be fixed on checkin

Comment 9

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #7)
> (In reply to Byron Jones ‹:glob› from comment #6)
> > Created attachment 792005 [details] [diff] [review]
> > 904568_3.patch
> 
> Simon, it would be nice to this into the 4.5.1 release we would like to do
> soon. Will you be able to look at this soon?

Did it earlier this evening. I try and look at all items in my review queue each with a week, as time permits. Your B/W/Bug.pm page is the only thing remaining in the queue.

Comment 10

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified Bugzilla/Object.pm
Committed revision 8712.