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

Created on 21 September 2012, over 11 years ago
Updated 5 July 2023, 12 months ago

Spin-off from #1161486-65: Move IP blocking into its own module

Letter of the initiator

I really appreciate the desire for quality [Been there, done that. ;)], but on a personal note, I'd recommend to spend some thoughts on balancing overly strict coding standards/style compliance with getting major things done. :)

Getting a feeling for the right balance certainly takes some time. It pretty much boils down to pragmatism and deciphering what can be done in a less major follow-up issue and what cannot — that is, all relative to the complexity of a particular change proposal at hand.

Perfectionism is the perfect enemy of progress. Especially when it comes to major change proposals, the primary focus for reviewers should actually be on the major [architectural] change proposal itself. Minor things can be happily hashed out in a separate follow-up patch or issue, which can already be created ahead of time, postponed on the major change. On top, such issues are an excellent opportunity for novice contributors to step into contributing to Drupal core. :)

Contrary to minor things, it makes sense to bear in mind that even the major change proposals are done and backed by volunteers, people like you and me, but who potentially spent an awful lot of time to figure out how to get the entire system to work with the new code. For them, it can be incredibly demotivating to see their major change proposals getting blocked on nitpicky reviews instead of moving those super-trivial issues out of the way into a follow-up issue.

So when balancing review issues, it is important to understand that we love to see bold proposals for Drupal core, and contributing to Drupal core ought to be fun, motivating, enjoyable, and respecting the work that others have put into a certain proposal or topic. It really matters to put things into relation.

I hope this helps, and I'm happy to discuss and clarify this some more in IRC or other means, if desired.

Goal

  • Rethink what level/granularity of code reviews is appropriate for major changes.
  • Clarify Drupal [core] development processes.

Details

  • The coding standards/style "initiative" started way back in ~2007.
  • Back then, Drupal core code looked significantly different and extremely poor in terms of coding standards, consistent coding style, etc — i.e.:
    • plethora of comments that did not wrap at all (a single line ending at ~1,458+ characters ;))
    • no @param/@return docs
    • heck, no phpDoc blocks in the first place
    • random comments as an attempt to put code into groups
    • extremely compact and unreadable code in one-liners all over the place

    (Note: Links are only possible, because no one ever touched those lines since ~2007.)

  • Thus, the code quality idea/initiative had to perform many unpleasant and unwelcome code reviews all over the place — so as to implant a new mindset and paradigm across Drupal developers that knows and understands that good code is properly documented, complex logic is explained, consumers can look at the code and learn from it, and that this is actually executed "by design", without having to ask for it first.

  • Times have changed since then. Significantly. The initiative was and continues to be a great success. Drupal [core] has one of the best documented source code throughout PHP applications.

    That's a major achievement for the Drupal community at glance!

  • However, perfectionism doesn't come for free. At the same time, we've seen a decrease of contributors, since they became very frustrated, after getting overly nitpicky reviews for bold patches that attempted to rewrite and revamp something. It's not fun anymore, if your major change proposal gets blocked by a never-ending amount of coding standards issues, after you've spent entire days or perhaps even weeks to get the actual, major change to work. Staying on top of an issue and resolving nitpicks requires a lot of patience, as well as a strong desire — and spare time — to get it done. Truth is, some of the code quality reviews of the past resulted in a few significant/major patches getting stuck.

    In other words: Significant improvements that are not in Drupal core today.

    (...and among a few others, @sun gotta take the blame for that.)

  • Finding the right balance for code reviews is not trivial. But it is highly important for Drupal's long-term success.

    Given the status quo, human/common sense should allow us to differentiate between minor/trivial things that can be adjusted after the fact, and things that can not. We need to make more sense of that. With regard to patches doing "major" things, we may need to introduce new guidelines for "absolutely unacceptable issues" (trivial examples being TABs, trailing white-space) and "minor stuff we can happily fix later". [That said, we'd obviously require a definition of "major", since there is none.]

  • It is time to adjust our thinking, paradigms, "gates", and processes for the current quality of code in Drupal core. That is, with regard to coding style/standards compliance. The root cause that originally led to the pedantic reviews we're doing today no longer exists.

    Our current behavior causes more harm than good.

Proposed resolution

Option A: New status 'Needs architectural review' to be used before 'Needs review'

If you want to clean up coding standards in the earlier review state, you do so with your own patch and interdiff, and you do not kick the patch to NW over such things while it's still under active development.

Option B: New status 'In progress' to be used before 'Needs review'

Someone has started work on it, it's not ready to commit, but there's something for someone else to look at. You could potentially use this for new issues with a developed proposed resolution that needs looking at but no actual code yet.
(this is from Slack discussion, Discussion in https://drupal.slack.com/archives/C04CHUX484T/p1677082624145459)

Option C: New tag 'Needs Code standards review'

Option D: Status quo

No new status or new tag.

Option E: New tag 'Needs architectural review'

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.

Other

I tried to gather the ideas where there was agreement

  • The priority of the issue should be considered when reviewing coding standards
  • It seems as though the "Core Gates" have set the standard for what constitutes patches that can be committed.
  • formatting issues are followup, meaningful comments are mandatory.

Remaining tasks

None.

Support

Count of support for each option. See #87.

  • Option A: 9
  • Option B: 2
  • Option C: 4
  • Option D: 1
  • Option E: 3

Resolution

Option A was the most popular, however adding a status to d.o is not going to happen as explained #3358079-3: Add a new status 'Needs architectural review' for core issues . Therefor, Option E, adding a new tag is implemented.

There is now a new special tag , 'Needs architectural review', with the following description.

The issue is available for high level reviews only. If there is a patch or MR it is not to be set to 'Needs work' for coding standards, minor or nit-pick changes.
🌱 Plan
Status

Fixed

Version

9.5

Component
Other 

Last updated less than a minute ago

Created by

🇩🇪Germany sun Karlsruhe

Live updates comments and jobs are added and updated live.
  • Coding standards

    It involves compliance with, or the content of coding standards. Requires broad community agreement.

  • API clean-up

    Refactors an existing API or subsystem for consistency, performance, modularization, flexibility, third-party integration, etc. May imply an API change. Frequently used during the Code Slush phase of the release cycle.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇳🇿New Zealand quietone New Zealand

    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.

  • 🇬🇧United Kingdom catch

    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.

  • 🇳🇿New Zealand quietone New Zealand

    @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.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    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.

  • 🇬🇧United Kingdom joachim

    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.

  • 🇬🇧United Kingdom catch

    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

  • 🇫🇷France andypost

    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

  • 🇷🇴Romania claudiu.cristea Arad 🇷🇴

    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.

  • 🇳🇿New Zealand quietone New Zealand

    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.

  • 🇳🇿New Zealand quietone New Zealand

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

  • 🇳🇿New Zealand quietone New Zealand

    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?

  • 🇬🇧United Kingdom catch

    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.

  • 🇳🇿New Zealand quietone New Zealand

    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.
  • 🇳🇿New Zealand quietone New Zealand

    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 .

  • 🇺🇸United States dww

    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.

  • 🇺🇸United States dww
  • 🇳🇿New Zealand quietone New Zealand

    @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.

  • 🇯🇵Japan tyler36 Osaka

    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.

  • 🇳🇿New Zealand quietone New Zealand

    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

  • 🇬🇧United Kingdom catch

    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.

  • 🇳🇿New Zealand quietone New Zealand

    @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?

  • 🇳🇿New Zealand quietone New Zealand

    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,

  • 🇳🇿New Zealand quietone New Zealand

    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.

  • 🇳🇿New Zealand quietone New Zealand

    Adding tag

  • 🇳🇿New Zealand quietone New Zealand

    I have implemented the tag and started to add credit.

  • 🇳🇿New Zealand quietone New Zealand

    More work on assigning credit.

  • Status changed to RTBC about 1 year ago
  • 🇳🇿New Zealand quietone New Zealand

    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.

  • Status changed to Fixed about 1 year ago
  • 🇬🇧United Kingdom catch

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

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

  • Status changed to Fixed 12 months ago
  • 🇳🇿New Zealand quietone New Zealand

    Adding parent to help find related issues

Production build 0.69.0 2024