The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.this is a multi-value metric that stores total issue counts per project. it splits the issues into two main groups by state, 'open' and 'fixed/closed'. counts of each are provided for each category, and a total across categories is also provided.
Comments
Comment #1
dwwMostly looks great, thanks! Here's what I found reading through the patch:
A) Although it's not new here, we're perpetuating a pattern we should remove. The PHPDoc explaining how class/interface functions work should only happen in the interface (or base class) definition, not in all the implementations of classes and subclasses.
B) I'd rather just call this "open vs closed", s/fixed_closed/closed/, etc. Not every site necessarily has our slightly weird and non-standard recently fixed -> closed distinction. I think it's simpler and more general if we just consider our fixed issues "closed" for this purpose.
C) I was assuming this metric was counting all kinds of closed, not just what's now known as "closed (fixed)" and "fixed". What about won't fix, by design, duplicate, etc? I'd consider those closed. They're all called "closed (xxx)" now, too.
See also #214347: Differentiate the kinds of open issues. Maybe we really want 3 counts per category: "Open", "Waiting" and "Closed". Or, "Open" (what we currently have as 'Open issues' in the issue queue filters), vs. "Outstanding" (the subset of "open" that's waiting for the maintainer) vs "Closed" (everything else).
D) It's too bad we're still using those status int constants. :( We're trying to kill those at #27865: Remove hard-coded status options. Probably out of scope to actually resolve this inside this issue, but we should at the very least add a /// @todo in the code pointing to #27865, or perhaps open a separate issue specifically about this one.
E) Ditto the hard-coded categories. See #115553: Ability to define own categories. Can't we at least define everything programatically using project_issue_category() instead of hard-coding?
F) Seems pointless to be passing a reference to $values around to helper functions inside the same class if we're just doing this in the end, anyway:
I'd rather see buildSampleResults() just add its values directly to $this->currentSample->values -- that'll save a lot of RAM, too.
G) I'm happy the metric itself is relatively simple thank to not caring about timestamps. However, should we open a separate issue to try to write a script that could populate these historically with some nastier (or lots more) queries and/or a lot of PHP logic? Or is the idea that we just roll this out ASAP and only gather/chart things from here forward?
Comment #2
hunmonk commenteda) #914808: move PHPDoc to interface (or base class) definition only for all the rest of the occurances, i've removed them for this patch as well.
b) fixed
c) & d) fixed, with a TODO referencing the status issue. re #214347: Differentiate the kinds of open issues, that's still under debate, and i'd prefer to stick with something simple for this first run. if we *really* want this later, we can do some magic to fill those values in, same as we're doing for the historical stuff.
e) this is a big ol' can of worms. the problem is that we're depending on what we have in the metric to build the schema to store it (which makes perfect sense to me), and project_issue wants these to be configurable. i see no practical way to magically handle schema changes if a site admin decides to add/remove an issue/category. all this makes me think that, unfortunately, maybe this metric needs to live in drupalorg module, since we're optimizing it for drupal.org's project configuration. i'll wait for your feedback on this.
f) fixed
g) i think i'd prefer the 'script' to be implemented as a metric, or included in this metric as an option -- neither one of those would be harder than writing a straight script, and then the code would be available to use through sampler for things like missed weeks, etc. i'll wait for your feedback on this before opening an issue to tackle it.
Comment #3
dwwCool, thanks! Haven't looked at the new patch, but based on your comment above:
E) Yeah, I know it's worms, but I wanted to mention it for consideration. Given that the categories are *not* currently flexible, I think it's okay to leave the metric in project_issue, not drupalorg. Maybe just add a TODO about this pointing to #115553...
G) Yeah, by "script" I meant "code" -- doing it as an option to this metric would be fine and then we can trigger it via drush. But yeah, definitely another issue.
H) Given that the metric doesn't currently handle timestamps and always counts the current totals, should there be a sanity check inside computeSample() to bail if the current sample is in the past?
Comment #4
hunmonk commentede) but status's ARE currently configurable, so we still have the same problem. any status besides 1 can be deleted by the admin. given that, i really think it's best to move this to drupalorg module.
g) #914894: extend opened_vs_closed_by_category metric to build historical data
h) i don't think this is necessary. the periodic method handler looks for the last sample time by default to start building its sample set. so, if it's not time for the next sample, there will be no sample set.
Comment #5
dwwre: h) I mean for cases where you manually specify a --startstamp via drush, even if accidentally.
I'm going to just hash out e in IRC with you, since I think that'll be faster. ;)
Comment #6
hunmonk commentedh) seems like overkill to me. if you're specifying a startstamp via drush, you should probably know what you're doing ;)
e) i see more clearly now -- we're only grouping by category, which is still hard-coded. if status values are added/missing, things won't barf, it will just skew the resulting metrics a bit. so i've added a TODO referencing #115553: Ability to define own categories for now.
Comment #7
dwwH) it's easy to accidentally use the wrong args for drush. The party line on sampler API is that drush is the *only* way to drive this stuff. So, you can't then argue "if you're using drush, you know what you're doing". ;) That said, it's not urgent to fix this, maybe I'll just open a lower prio separate issue about it.
C + D) eek. Just looked at the patch. You can instead do this:
Although, that's going to count 'fixed' as 'open' until #214347: Differentiate the kinds of open issues is done. If we were going to have a hard-coded hack, I'd use something like the above but unset PROJECT_ISSUE_STATE_FIXED from the array before array_keys() (and leave a comment pointing to #214347).
F)
+ $values = array();is now dead code, too.I) Typo:
+ // Don't comput if we're on the first sample in a sample set.Comment #8
hunmonk commentedh) part of the problem is you can't skip if it's in the past, the periodic method by default assumes that you don't want any sample points after the current time, and we want to respect that -- so all sample times will be in the past. sounds like this might best be solved by having the API by default not overwrite any samples if they already exist, and allowing that to be overridden in cases where you want to re-take a sample to correct an error, for example. that fix will take some time though...
c & d) fixed
f) fixed
i) fixed
Comment #9
hunmonk commentedComment #10
dwwLooks great other than the fact that this comment is no longer true:
I haven't tested at all, but assuming this actually works, you can remove that comment and commit.
Thanks!
-Derek
Comment #11
hunmonk commentedremoved comment, retested, working beautifully, committed -- whoot!