[Policy, no patch] Add special tag to identify issues ready for a high level only review

It has been 7 years since there was discussion here and I see there is no proposed resolution in the issue summary.

I read the comments and understand that that is working to address the sometimes negative impact of coding standards reviews. But since this issue was created coding standard checks have become automated. And as the sniffs get enabled in core there are less reasons for these types of reviews.

Still, it is true that not all the rules for the Drupal coding standards are enabled. Issues still do get set back to Needs Work when they don't meet the Minimum requirements for in-code documentation → . Is that still causing a problem for some?

I do think that in the 7 years since this issue became quiet that the community practice has improved in this area and that this can be closed.

I think the overall problem is still very much there - critical bugfixes often get stalled on something like documentation wording for the test coverage - I know this because I sometimes mark the issues needs work for that, obviously in the hope it's a quick fix and I can commit it when it's back at RTBC soon, but that's not always the case. Sometimes I try to fix things on commit, and sometimes reviewers fix things 'on-review' (i.e. upload a patch/commit to the MR with their remarks addressed), which is a good compromise between deliberately ignoring problems to get something committed vs. stalling an issue on something minor.

But sometimes I find a major bug that's about two years old that got marked work for a very minor issue, and then never got revived and just sat there, and it's very sad when that happens - especially if it's an issue affecting real sites - and then I think 'was the grammar of that code comment really worth the two year delay?'

There are these issues to remove boilerplate documentation that we don't really need any more:
#3238192: Allow omitting @var for strictly typed class properties →
#2140961: Omit first line documentation for __construct() and other magic methods →

There's also this issue to only add tests we actually need, and/or sometimes in follow-ups 🌱 [policy, no patch] Better scoping for bug fix test coverage RTBC .

These would help to remove the 'surface' of things that need review at least, but there are probably more, similar changes that might help too.

PHPStan, cspell, and also bugsmash and the needs review queue initiative are all definitely helping the overall situation, since at least you get much quicker feedback vs. waiting three months for a review then getting your issue marked needs work for a typo. PHPstan and cspell however are also reasons I wouldn't want to punt too much to follow-ups, since we already have massive number of coding quality issues via the process of fixing phpstan levels - we don't want every medium-priority issue to span 5 minor follow-ups.

@catch, Thanks. I really appreciate your ability to summarize the history and to provide examples.

The three issues linked are definitely part of improving the review process. And they are on my list to work on, if that helps.

For this issue, I have updated the Issue Summary with the suggestions from the comments. I also added one from a relevant Slack discussion that catch started in #needs-review-queue-initiative.

I suggest the next step is to get consensus on an Option.

I like option A - but alternatively we might be able to use this to solve two problems we have at the moment, the other one being a lot of poor re-rolls and contributions that only address these types of nitpicks and don't do anything to resolve open issues.

So perhaps we could keep it as needs review, and
* if you have nitpick standards reviews AND
* you don't intend to fix them yourself

Then you keep it as needs review, leave your review and add a tag such as Novice (or a new tag) for the new contributors to come along and solve those bits in a useful fashion rather than disrupting another issue.

But yeah, if that's not popular, option A sounds fine.

I'd like BOTH options A and B.

> A new status 'Needs architectural review' to be used before 'Needs review'

This would be REALLY useful. There's no point nitpicking details and polishing code based on nitpicks if the whole approach is wrong. And sometimes, it's enough to just write out an outline of how something would be coded.

And 'In progress' would be useful too. Especially now we have bots that set an issue to NW if the MR no longer applies, when all you want is some extra pairs of eyes on it.

fwiw I also like option A, although maybe we could go for a single 'In progress' status and use a 'Needs architectural review' tag?

The lifetime of an issue could then look like this:

Active -> In Progress -> Needs review -> Needs Work -> Needs Review -> RTBC - Fixed

If we'll go with follow-ups(backlog) for minor polishing it will bring 2 new problems
- size limits for new queue (new backlog)
- longer time to get stable state for new features

My example is help topics - years to get reviews

There are these issues to remove boilerplate documentation that we don't really need any more:

I also the case for getters:

/**
 * Returns the foo service.
 *
 * @return FooInterface
 *   The foo service.
 */
public function getFoo(): FooInterface {
  ...
}

In this case I see the @return description redundant, having no value.

quietone → credited cmlara → .

quietone → credited lauriii → .

quietone → credited nod_ → .

quietone → credited smustgrave → .

This was discussed in Slack by catch, smustgrave, lauriii, larowlan and nod_. Here is my summary of that discussion and the last few comments.

In Slack, the option A-D here were discussed as well as plenty of other solutions. That alone indicates that the options here are, at best, part of a number of improvements that could be made. It was also clear that the situation as expressed in the earlier comments in this issue (up to #66) are from when we didn't have automated coding standards checks. We do have them now. But the problem persists. It is less prevalent but it is still slowing progress on issues.

In regard, Options A-D here is what people preferred.

• Option A webchick, xjm, attiks, chx, smustgrave, catch, larowlan, joachim, FeyP, quietone
• Option B smustgrave, joachim
• Option C DameinMcKenna, moshe weitzman, tstoeckler, plach
• Option D lauriii

The 'Needs architectural review' status has the strongest support.

And these are the other suggested solutions.

Do more self-RTBCs and keeping issues RTBC after posting changes

• Another options, 'E' Add status 'In progress' with tag 'Needs architectural review'
• An approval process (apparantly GitLab has this).
• More room for 'fixed on commit' from committers.
• Add a baseline for phpcs.
• Ignore non automated nitpicks, possible followup. These followup can be good for new contributors. However, there is concern about this due to the credit farming behaviors. Also, longer time to get stable state for new features (from #85).
• If you find nit-pick standard reviews, then update the patch yourself.
• Coding standards - remove boilerplate no longer needed

Let me know if I have made a mistake in the Summary.

Adding credit for people in the Slack discussion. Let me know if I missed you.

I probably would have asked this during the work week, but I am away next week, so doing so now.

Is there agreement to proceed with Option A? I am adding that lauriii said in Slack that they are happy to support testing of the other options.

Is there anything blocking implementing Option A?

If we use a tag for option A, then it's easy. But to get a new status we'd need to open a project module or drupalorg (not sure which) issue.

Assuming that Option A is agreed to we also need text for the Issue tags -- special tags → page. Here is a suggestion.

Needs architectural review

The issue is available for high level reviews only. It is not to be set to Needs Work for any minor or nit-pick change.

In Slack, dww pointed out that I was thinking that Option A was a tag, not a status. Comment #95 is incorrect. But some text will still be needed on Status → .

I've been following this issue but didn't have a chance to chime in. I haven't read every comment, but I got through the extensive summary. It seems there are multiple, overlapping problems we're trying to solve. Thanks for everyone's efforts working on this important problem space.

I didn't see an option for what I believe is (part of) what we should do: a new 'Needs architectural review' issue tag (not status).

I updated the summary to use h4's with an id for each of the choices (way better for accessibility than strong), and now we can link to options as needed. 🤓

I also added Option E: A new tag 'Needs architectural review' and put myself down in support. 😉 Copying here as a comment:

Just like option A, but implemented as a new issue tag, usually for things still in 'Needs work' status.

Could also be applied to something in 'Needs review' if an issue is ready for both kinds of feedback.

Sometimes used along with the existing 'Needs XYZ review' tags we've already got (subsystem maintainer, framework manager, etc).

Not every issue needs this step, so instead of forcing everyone on every issue to think about a new status value, use a tag for the subset of issues that would benefit from this.

We could also adopt the new tag immediately by starting to use it, instead of having to wait for a drupal.org config change.

@dww, thanks for your input.

I suggest the we keep the Options closed now and put our effort into resolving this issue in the next few weeks.

Option C/E - add a tag.

Ideally, it should be automated.

- All commits are run through linters (PHPCS, PHPSTAN, Eslint, Stylelint etc.).
- Where possible, said linters would add a "fix" commit for anything they can correct.
- If there are still issues after the "fix" commit, a "needs formatting" (Needs architectural review) tag is applied.
- If there are no linting issues but the "needs formatting" tag exists, it is automatically removed.

The community encourges novices and workshops/camps to work on "needs formatting" tagged issues where possible. The goal being to make commits that will remove the "needs formatting" tag. If they can also contribute to the original issue in other meaningful ways, even better. This helps familiar them with best practices while focusing on smaller code chunks.

In a previous project in a Github workflow, I used php-cs-fixer-ga to scan commits & PRs, and git-auto-commit Action to commit any changes php-cs-fixer-ga created. That automated the first 2 steps above.

IMHO, another state in the progress chain would only slow the release of patches.

Updating the Option support in the IS, based on Slack discussion. https://drupal.slack.com/archives/C1BMUQ9U6/p1682043836509079

Option A webchick, xjm, attiks, chx, smustgrave, catch, joachim, FeyP, quietone
Option B smustgrave, joachim
Option C DameinMcKenna, moshe weitzman, tstoeckler, plach
Option D lauriii
Option E larowlan, FeyP, dww

I think given option E only requires a new tag rather than a status we could go ahead with that and combine the votes for A and E.

@catch, that is certainly a reasonable suggestion.

How about a trial period using the tag? We can start using it now and then in, say a month, evaluate the results.

So, are there any objections to creating a new tag, 'Needs architectural review', and using it for a trail period of one month?

Created 📌 Add a new status 'Needs architectural review' for core issues Fixed to learn how about the time required to implement a new status for core issues,

dww responded in the issue created above 📌 Add a new status 'Needs architectural review' for core issues Fixed explaining all the work required to add a new status. The conclusion there is that it is very unlikely a new status will be added. It would require code changes as well as doc change. And give that moving to GitLab is the priority there is little interest in doing the work.

So, that leaves with setting up a new special tag → . We can start using a tag right now, but it would help to document its use.

Adding tag

I have implemented the tag and started to add credit.

More work on assigning credit.

Updating the title to reflect what the discussion has been focused on. I am taking the liberty of adding credit for myself for the work in updating the issue summary, adding the tag etc.

I think we have reached a conclusion on this issue so I am setting to RTBC.

For further improvements to the review process see the issues at 🌱 [meta] Improve issue management Active before making a new issue.

This is great, let's try to actually use the new tag!

Automatically closed - issue fixed for 2 weeks with no activity.

Adding parent to help find related issues