Problem/Motivation
There are too many issues that are not actionable by the original issue participants or a Core Maintainer.
You can see the gigantic list at - https://drupal.org/project/issues/drupal?status=8 - when this issue was first opened the count was 2388.
It's near impossible to get a timely review for most issues outside of IRC.
It's not unusual for issues that might only need trivial feedback to be waiting in the "Needs review" list for multiple years.
A large number of stagnant CNR issues represents a lot of wasted volunteer effort.
Active, Postponed and "Needs work" patches usually have some known further action that can be taken by the thread participants to progress the issue, whether it's discussion or code. "Needs review" can be hard for a patch to break through as often a commitment is required from a community member who had no prior engagement with the issue.
Proposed Resolution
Adapt a simplified version of the "PAReview Bonus" Program from the Project Application queue to the Core issue queue to promote and focus the patch review process.
You can read all about the PAReview Bonus project at https://drupal.org/node/1975228.
Proposed core review bonus process:
Choosing an issue:
Important: Only choose an issue that you feel you can do a good job reviewing. Don't feel obliged to be pushed way outside your comfort zone.
We want to prioritise reviewing issues that other people have tagged, so start by trying to pick an issue from the Core review bonus list (you can ignore issues you tagged yourself).
If the bonus list is empty (or you can't find anything there you feel comfortable reviewing), pick any issue you like from the normal "needs review" list.
If there are no issues in the normal review list, come back tomorrow!
Are you new to contributing? Don't worry, that's what the "Novice" tag is for!
Novice, Core Review Bonus issues can be found here: https://drupal.org/project/issues/search/drupal?status%5B%5D=8&issue_tag...
Normal Novice issues that need review can be found here: https://drupal.org/project/issues/search/drupal?status%5B%5D=8&issue_tag...
Performing a good review:
Here's a collection of links to existing docs to help you understand the Core development process.
- Reviewing patches.
- Contributor tasks: Review.
- Core Gates.
- Writing secure code and subpages.
- Drupal Coding Standards.
- Issue summary template standards.
A review does not have to be long but it should be as thoughtful as possible, within your personal ability.
A good quality review will usually take at least 15 minutes and could take as long as an hour or more if the patch is very large or involves a complex testing process.
The "Core Review Bonus" tag
Once you've left a comment on an issue, if the issue has made some progress (see below) then you've earned a tag and can remove the "Core Review Bonus" tag from the issue you just reviewed (if it's there).
Other people need to be able to see what is going on, so this is a two-step process. Leave a note on or just under your review comment like this:
#2094585: [policy, no patch] Core review bonus for [#12345]
Then go to your issue #12345 and tag it with "Core Review Bonus", with a link back to your review comment at the same time. This way your issue will turn up in the Core Review Bonus issue queue automatically and other people will be able to see why it was tagged.
Because other people participating in the Core Review Bonus are working through tagged issues first, this ensures that the issues you need feedback for are given a little more attention by the community than other issues of the same priority.
"Progress" on an issue could be:
- The issue has moved to a new status (most often back to "Needs Work" with your feedback)
- The issue summary has been updated to match the standard template (linked to above), or the documentation for the patch has been improved.
- Performance profiling has been done
- Manual testing has been performed and steps to reproduce have been provided in the issue summary for others to verify the testing results
Automated reviews are useful (keep doing them until #1299710: [meta] Automate the coding-standards part of patch review lands!), but people appreciate it most when you take the effort to do a manual review alongside a scripted one. A review that is purely automated and no human has read over the patch or tried to understand the problem that the patch solves, while useful, should not be used as part of the Core Review Bonus - See comment #30 in this thread.
Remaining tasks
Review some patchesApply some tags- Refine this process until people like using it
- Trial using this process at a Core sprint
User interface changes
Adding a new "Core Review Bonus" tag for core issues.
API Changes
None.
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedComment #1.0
thedavidmeister CreditAttribution: thedavidmeister commentedadding linke to "Core Review Bonus" search page
Comment #1.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.6
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.7
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.8
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.9
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.10
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.11
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.12
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.13
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.14
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.15
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.16
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.17
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.18
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.19
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.20
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.21
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.22
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #1.23
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commentedComment #2.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2.3
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2.4
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #2.5
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #3
markhalliwellUm... so we get points for what exactly? What do I get to do with them, brag that "I have more core bonus points than you"? How is that an "incentive"? I have never liked the PAReview Bonus "program". It serves as yet another "gate" to prevent contribution and in reality doesn't help us with the over all issue: commit rejection (in some for or another). A lot of these "rejections" are for trivial coding (or general) standards. These could be automated, if we really want to put forth the effort... but no one seems to. Sad. We already have enough GK for core, why are we adding yet another one?
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commented@Mark Carver, please read the issue summary again, individuals don't get points, issues do, and issues with more points are reviewed first - see step 1 of "choosing an issue".
If you think that's how this works, I dare say you haven't actually participated in the PAReview Bonus? Neither system leads to an accrual of persistent "points" for a user. Any points that you "earn" by reviewing one issue have to be immediately put towards raising the visibility of another issue for potentially expedited review.
I'm 100% in support for automating coding standards reviews (you can see that I've left comments on the issue that you linked, supporting it back in July, no I haven't submitted a patch!), this issue isn't about changing the way we ask people to do reviews, it's an alternate way to help people doing reviews already select what to review next.
@Mark Carver, I don't think you fully understand the system proposed here (which is fine, ping me in chat to discuss if you want), nor its intention which is:
- Improve visibility of issues that need review, but are at risk of "slipping through the cracks"
- Encourage contributors to be more broad in what they review and consider reviewing other people's patches before they complain that nobody will review theirs
- Provide an alternate channel for contributors who want their patch reviewed to solicit a review outside IRC
- Provide some extra community spirit and feel-good vibes for people doing reviews
- Try to keep issues actionable by the main "stake holders" who have already put in valuable time and effort into getting the issue to CNR and just need some "outside" help to get it over the line
Regardless, the whole thing is entirely opt-in and can be safely ignored by anyone who doesn't want to participate, there's no plans at all to try and turn this into a Gate or any other kind of barrier to getting something over the line.
Comment #5
markhalliwellFair enough and you're right, I haven't participated in PAReview bonuses (just have heard poor experiences from them because it is used there as a gate). It still just seems like a lot of overhead in general though and there has to be a better solution than this.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented#5 yeah, there's a bit of copying and pasting and some basic math involved, but compared to the overall amount of work that's involved in doing a proper review, it's pretty minor. The whole thing is super DIY, experiment-y atm - if it ever really took off i'm sure it could be "baked in" to the issue tracker (or Dreditor, more likely), but seriously, right now it's just me tagging issues and doing some reviews that I probably would have done anyway. I feel like it's waaaay too early to be talking about optimising the labour involved in a completely unproven process.
I had a great experience with the PAReview bonus, I know other people don't like it, I guess in many ways it comes down to personal expectations and experiences.
Bit of a downer that the first feedback I got was decidedly negative, but I want to keep going because I think it's too early for anyone to pass judgement objectively. I've done both, and Core reviews are a rather different beast to Project Application reviews, so it will be an interesting experiment I think :)
Comment #7
markhalliwellSorry :( Just re-read my comment and can see how it might come off as "negative". I really wasn't trying to be, just more confused than anything is all I guess. I am actually happy that at least someone is taking this challenge on though :) I'll definitely keep this in mind and will probably revisit this again soon to really read through and digest what it is you're trying to do and make suggestions for improvements. Keep up the great work @thedavidmeister!!
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commented@Mark Carver, might I suggest that you actually work through the steps at least once, and have a look at how I've been doing it too for an example.
I think it will make more sense to you if you actually physically go through the process at least once, rather than trying to imagine it all.
Comment #9
markhalliwellTime, especially to read all at a deeper comprehension level. I'll get to when I can.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedno rush :)
Comment #11
thedavidmeister CreditAttribution: thedavidmeister commentedI think I found something that could be improved.
Currently, you need to change the state of an issue for a review to "count", but really, looking at something like #1987410: [meta] system.module - Convert theme_ functions to Twig right now, one person doing everything required to change the state is a lot to ask:
- 30k patch, not huge, but not tiny
- Needs issue summary update
- Needs manual testing (no testing steps are in the summary)
- Needs profiling
So, probably, the requirement should be that you change the state of the issue OR do a review that removes a review related tag, if there are multiple review related tags on that issue (if you remove the last review related tag, you should also change the state of the issue).
I'll update the summary here, now.
Comment #11.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #11.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary as per #11
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedI updated the issue summary, to hopefully make it easier to read, lowered the amount of work required to bump off a "Core Review Bonus" tag (#11) and also linked to the automated code sniff issue (#3).
Comment #12.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12.1
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #12.2
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #13
jwilson3Here's an idea, what if we *add* tags for issues that are *well done* in addition to adding tags for when things need assistance. (Eg, when you remove the "Needs issue summary" tag, you automatically add the "Has issue summary" tag). This will help people to start actively looking for issues that are in "needs review" and "has issue summary". (A small step, and only tangentially related to this but might prove helpful if core contributors were to get on board with it).'
I do like this idea -- having a couple core patches that I've worked on lingering in Needs review myself, and feeling the frustration mentioned in the issue summary.
Comment #14
thedavidmeister CreditAttribution: thedavidmeister commented@jwilson3, the change to tags sounds interesting. You should open up a meta for discussion, anything with potential to help get the number of CNR issues down would be great. The total number of CNR issues has actually *increased* since I opened this issue to 2427, we're going backwards at a rate of more than 1 unattended CNR issue per day :(
I managed to get a couple more issues tagged for review bonus today (https://drupal.org/project/issues/search/drupal?status%5B%5D=8&issue_tag...), so if you could help get the ball rolling over there with a review or two and maybe even get one of your own issues that you mentioned tagged, that would be amazing.
Comment #14.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #15
thedavidmeister CreditAttribution: thedavidmeister commentedOk, so I've been shopping this idea around casually with people I know through work and the usual reaction is to run away from the wall of text that is the issue summary - this issue needs a healthy dose of KISS if it's ever going to get anywhere.
Comment #16
thedavidmeister CreditAttribution: thedavidmeister commentedComment #17
thedavidmeister CreditAttribution: thedavidmeister commentedComment #18
thedavidmeister CreditAttribution: thedavidmeister commentedComment #19
thedavidmeister CreditAttribution: thedavidmeister commentedComment #20
thedavidmeister CreditAttribution: thedavidmeister commentedComment #21
thedavidmeister CreditAttribution: thedavidmeister commentedComment #22
thedavidmeister CreditAttribution: thedavidmeister commentedComment #23
thedavidmeister CreditAttribution: thedavidmeister commentedComment #24
thedavidmeister CreditAttribution: thedavidmeister commentedComment #25
thedavidmeister CreditAttribution: thedavidmeister commentedComment #26
thedavidmeister CreditAttribution: thedavidmeister commentedComment #27
thedavidmeister CreditAttribution: thedavidmeister commentedComment #28
thedavidmeister CreditAttribution: thedavidmeister commentedComment #29
thedavidmeister CreditAttribution: thedavidmeister commentedOk, I think the issue summary is much easier to read now, and the whole process has been simplified.
Gone is the concept of "points" and whatever. What we have now couldn't really be simpler I don't think:
You leave a review for someone else (prioritising issues with the tag), you reference the issue you want reviewed, you tag the issue you want reviewed and reference the review that you did yourself.
What does everyone think of that?
Comment #30
xjmInteresting proposal!
One thing in the summary seems misleading to me:
I'd phrase this differently. We don't need people to be computers -- we have computers for that. We need people to be people. :) Each patch that gets committed to core needs to be read by human beings for code review, and tested by them manually because no automated test suite can cover everything.
So, if you're reviewing a patch and wanting others to return the favor, you should really be thoughtful about it. Read the issue, read the code, apply the patch and try it, do research to understand it better. Not everyone will do all these things for every issue, of course, but simply running an automated tool against someone's patch and not focusing on the actual issue tends to cause conflict and disappointment. More thoughts on this matter in my review guide blog post from a couple years ago. (One of these days I need to post a followup to all that... or distill it onto d.o so people don't reference my blog all the time.) ;)
Aside from that. I'd suggest removing the link to https://drupal.org/community-initiatives/drupal-core from the summary since it's not that relevant, and adding some to some of the relevant contributor task documents where appropriate:
https://drupal.org/contributor-tasks/review
https://drupal.org/contributor-tasks/manual-testing
https://drupal.org/contributor-tasks/improve-patch-standards
https://drupal.org/contributor-tasks/write-issue-summary
And, one of these days, we also need to make https://drupal.org/patch/review a whole lot better too. :)
Thanks so much for all your work on this proposal. This is still the #1 biggest hurdle for participating in core development and the biggest need core has.
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated the issue summary to say this.
Removed the initiatives link. Added the suggested links.
@xjm, Thanks for the kind words :) would you consider trialling the system and tagging an issue next time you review a patch?
Comment #40
andypostComment #44
quietone CreditAttribution: quietone at PreviousNext commentedChanging to a policy to determine if the community agrees with this proposal.
Although there has been no activity here for 9 years it is worth checking if anyone is interested. I will ask in #contirbute.
Comment #45
quietone CreditAttribution: quietone at PreviousNext commentedI asked about this issue in #contribute, both larowlan and catch replied. It was pointed out that the work done in the #needs-review-queue-initiative has made significant progress on reducing the number of issues waiting for review. As I write this there are 76 core issues at NR. Another point was that the proposal doesn't seem workable for core. Core has many topics where there reviewer needs to know about that topic to make a meaningful review. So, even you have done the work and have tagged an issue 'core review bonus' there may not be anyone participating in the program that can review it.
Given, those two points and the fact that there has been no activity here for 9 years, I am closing this as outdated.
To everyone who worked on this, thank you for the proposal and working to improve Drupal!
Comment #46
quietone CreditAttribution: quietone at PreviousNext commented