Closed Bug 1090493 Opened 5 years ago Closed 5 years ago

Allow ComponentWatching extension to work on either bmo/4.2 or upstream 5.0+

Categories

(bugzilla.mozilla.org :: Extensions, defect)

Production
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dkl, Assigned: dkl)

References

Details

Attachments

(1 file, 2 obsolete files)

Currently if you install the component watching extension on upstream trunk (5.0+) it will fail during a new database configuration due to components.id changing to a MEDIUMSERIAL instead of a SMALLSERIAL.

We need to make the extension look to see what size components.id is and adjust accordingly.

dkl
Attached patch 1090493_1.patch (obsolete) — Splinter Review
Will not work on upstream 4.2 or 4.4 as it still relies on the fix from bug 1082106 which only went into 5.0/master.

dkl
Attachment #8512970 - Flags: review?(glob)
Duplicate of this bug: 1088107
Comment on attachment 8512970 [details] [diff] [review]
1090493_1.patch

Review of attachment 8512970 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/ComponentWatching/Extension.pm
@@ +39,5 @@
> +        if ($field eq 'id') {
> +            $component_id_type = 'INT3' if $def->{TYPE} eq 'MEDIUMSERIAL';
> +            last;
> +        }
> +    }

dcloning to process the list in pairs is messy.
let's use List::Util's pairfirst instead:

my (undef, $def) = pairfirst { $a eq 'id' } @{ $schema->{components}->{FIELDS} };
my $component_id_type = $def->{TYPE} eq 'MEDIUMSERIAL' ? 'INT3' : 'INT2';
Attachment #8512970 - Flags: review?(glob) → review-
Attached patch 1090493_2.patch (obsolete) — Splinter Review
Yeah much cleaner.

dkl
Attachment #8512970 - Attachment is obsolete: true
Attachment #8579648 - Flags: review?(glob)
Comment on attachment 8579648 [details] [diff] [review]
1090493_2.patch

Review of attachment 8579648 [details] [diff] [review]:
-----------------------------------------------------------------

gah, sorry, it looks like the version of List::Util we have installed is only 1.21, and pairfirst doesn't exist until 1.30.

there's some other nice things in recent List::Util, so i wouldn't be against bumping our minimum version to 1.41 (and filing an associated infra bug to get it deployed).

or you could rewrite it in a way that doesn't require a dclone or pairfirst.
Attachment #8579648 - Flags: review?(glob) → review-
Attached patch 1090493_3.patchSplinter Review
And here it is. Not pretty, but works :)

dkl
Attachment #8579648 - Attachment is obsolete: true
Attachment #8584675 - Flags: review?(glob)
Comment on attachment 8584675 [details] [diff] [review]
1090493_3.patch

Review of attachment 8584675 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob with changes on commit

::: extensions/ComponentWatching/Extension.pm
@@ +34,5 @@
> +    # Bugzilla 5.0+, the components.id type
> +    # is INT3, while earlier versions used INT2
> +    my $component_id_type = 'INT2';
> +    my $len = scalar @{ $args->{schema}->{components}->{FIELDS} };
> +    for (my $i = 0; $i < $len; $i++) {

that should be |$i < $len - 1| because you're accessing $i+1
and since they are pairs, use $i += 2.
Attachment #8584675 - Flags: review?(glob) → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   426d9ce..634f31d  master -> master
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Component: Extensions: ComponentWatching → Extensions
You need to log in before you can comment on or make changes to this bug.