Bug 1446236 - BugmailFilter: Divorce from TrackingFlags extension

[CyberShadow]

Allow BugmailFilter extension to work without TrackingFlags.

[mohawk2]

Why not instead have $tracking_flag_names which is an array-ref, and in boolean terms could be @$tracking_flag_names? That would be even more efficient.

Otherwise I approve this change.

[CyberShadow]

Bugzilla::has_extension is now in git, so I'll update this to use it instead.

I think we want to eventually refactor all uses of has_extension out of core and instead provide hooks or equivalent functionality to allow extending the core in an extension-agnostic way. Using has_extension thus creates an implicit TODO, but we could use it alongside some other method, like a comment, instead.

I'm also a bit nervous about the idea of enabling execution of some non-trivial logic (which happens to be a no-op when the array is empty) by a boolean condition with dual meaning (disabled whether the array is empty or doesn't exist, if I understood you correctly).

[mohawk2]

I'm not sure I see any difference between a boolean array, and a boolean flag. Your logic above is setting a boolean flag based on whether anything is in the array, then repeatedly getting the contents of such an array on every call?

[CyberShadow]

Your logic above is setting a boolean flag based on whether anything is in the array, then repeatedly getting the contents of such an array on every call?

Sorry, could you please clarify what this is referring to specifically?

[mohawk2]

Literally the line we're commenting on. To copy-paste it:

state $have_tracking_flags = any { $_->NAME eq 'TrackingFlags' } @{ Bugzilla->extensions };

That is a boolean flag. Its truth depends on whether the array in Bugzilla->extensions contains any whose name is TrackingFlags. Does that return a different list from Bugzilla->tracking_flag_names?

[CyberShadow]

Thanks for clarifying.

That is a boolean flag. Its truth depends on whether the array in Bugzilla->extensions contains any whose name is TrackingFlags.

Yep, the plan is to remove that line, and replace if ($have_tracking_flags) with if (Bugzilla->has_extension('TrackingFlags')).

Does that return a different list from Bugzilla->tracking_flag_names?

Well, that depends. Can Bugzilla->tracking_flag_names ever return an empty array? If yes, then the change becomes more than a simple refactoring, and the semantic meaning of the check is changed: it is now overloaded to mean both "Bugzilla->tracking_flag_names doesn't exist" and "Bugzilla->tracking_flag_names exists, but returned an empty array".

I hope I understood the situation correctly.

[mohawk2]

I would have thought having the array-ref be empty (and still using whether it has content for the boolean) so you can use it in that array-boolean rather than reference-boolean, which this sort of is.