Code Review Complaints

Events happening in the community are now at Drupal community events on www.drupal.org.
You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!

So, you think something is wrong? Good, add a comment on here to let us know. You can also help with process building and making more significant changes by going to the Process Building (see Code Review homepage).

Comments

dave reid's picture

A user goes through the project application process. Reviewer notices that the code could easily be a 'refresh' of an abandoned module and is agreed by the abandoned module's maintainer, who makes the applicant a co-maintainer with write privileges on the project. What do we do with the application then? Typically we won't fix it since they have the access they need for that project. Should applicants in that case who have basically put in the work to make a new module get the 'git vetted user' role or not?

In an ideal world this never should have been a new project application to begin with - they should have filed the abandoned project process to take it over. In reality people use new project applications without search for an existing module they can maintain or refresh.

I try to not make exceptions and I get stepped on by other reviewers. If I make exceptions then I'm being unfair to other applicants.

Senior Drupal Developer for Lullabot | www.davereid.net | @davereid

For reference:

dave reid's picture

For reference: http://drupal.org/node/1144348

At what point should we be sending users to the abandoned project process rather than continuing the new application process?

Senior Drupal Developer for Lullabot | www.davereid.net | @davereid

thats a tough one

zzolo's picture

Well, first off, the review process is till really subjective, but things are starting to get more well defined.

In this case, technically someone did a bad review, usually the first step is to address if and how the applicant has looked at existing modules. If there appears to other similar modules, then the reviewer should ask the applicant what the differences are. Then it is subjective on what the next steps are.

In this particular case, I would give the applicant full access (even though there were some review failures), but with a very strong suggestion on moving their work into the abandoned module space instead of going forward with the original project. The reason is that the review process has already given community approval that the person can write a module, and the lack of looking for other modules is a problem, but in this case not worth the headache of holding it back.

Again, so subjective. If we could actually map a lot of this stuff, it would be a lot easier to say, "Well this is the agreed upon approach", and then it wouldn't be as subjective or be a matter of exceptions. I think we are making strides towards that.

--
zzolo

Yeah, I think I'm hung up on

dave reid's picture

Yeah, I think I'm hung up on the part about making sure the person can be trusted to perform a basic search before starting a new project page - it's not just about if they can write code. I didn't feel the application was properly vetted in this case, and the applicant had commit access for the desired project granted. Most of the applications I see anymore fit very close to if not directly duplicate existing projects and I have yet to see one actively report about similar modules and why theirs is different - even though this is stated as something that should be done by the applicant. Maybe I'm too much of a process-follower in order to be a reviewer. :/

Senior Drupal Developer for Lullabot | www.davereid.net | @davereid

I still think that's a good

BrockBoland's picture

I still think that's a good thing.

It's not going to prevent a flood of near-duplicate modules from being released, but it might help. The community is what makes Drupal, and a willingness to contribute to existing projects - or at least know what's out there already and what it does - is a critical requirement for a developer who wants to release their module under the Drupal banner.

Without process, you have

jthorson's picture

Without process, you have chaos ... knowing and abiding by the process is an absolute requirement of any reviewer.

However, from a reviewee's perspective, the length of the queue in recent weeks is a significant source of frustration; and some may (rightly or wrongly) interpret this as being sent to the 'back of the line' on a technicality. Now hopefully, with a few tweaks to the process, the current queue situation can be improved; after which this becomes 'no big deal'.

Until then, the way I'd approach this particular situation (as a reviewer) would be to question whether I feel the user now appreciates the need to search for duplicate modules (as opposed to their understanding at the outset of the process). This would become the basis of a decision whether to grant the 'create full projects' permission ... is having their module rejected (or re-directed) enough of a lesson that they'll be sure to search in the future? Has the applicant met all of the other requirements that would be expected through an 'accepted' code review?

Of course, this becomes a subjective decision for the reviewer, and differences of opinion are still sure to arise ... but if the applicant is able to demonstrate their understanding (and acceptance) of the 'duplicate module search' requirement, can articulate this understanding through their comments, and all other requirements have been met through the code review process, then I believe the process should be flexible enough to allow a reviewer the choice of granting 'create full projects' permission without acceptance of the module itself. The onus to demonstrate this understanding falls upon the applicant; and if they are not able to do so to the reviewer's satisfaction, then 'to the back of the queue' they go.

In the end, this should be a fairly uncommon situation, as the process should flag the duplicate module issue long before a detailed code review is completed, and therefore the 'other requirements' conditions will rarely be satisfied ... but if they are, and the applicant appreciates the need for a duplicate module search, then the only thing to be gained by denying the 'create full projects' permission is a second chance for the applicant to prove they can abide by an application process that, if successful, they will never need to do again.

Redundancy can be a blessing in the worlds of hardware, software, and networks ... but my experience has taught me that redundancy in process is seldom a good thing.

promote this step more

zzolo's picture

It's interesting. Since we have moved to Git, people don't have to upload a tarball/zip file as an application. Before, with CVS, the application issue practically became the project page and so it was very necessary for the applicant to describe their project in full.

But with sandboxes, we technically have a project page where an applicant can describe their project and how it compares to other projects. In reality, this is not happening, and reviewers are not pushing for this. Before, @kiamlaluno would come by and prety much ask everyone to expand their description and what other modules it compares to.

We need to promote this more to reviewers. First step should be: "Do you have enough description and have you look at other modules?".

Also, I don't actually see that much duplication. Of course there are modules that are similar to existing projects, but not too many applications that I feel are really duplicates.

--
zzolo

Speaking of duplication, I

jordojuice's picture

Speaking of duplication, I was going to email, but I would like @David Reid to take a look at http://drupal.org/node/1186036#comment-4593760 if possible. Considering the popularity of poormanscron (and my inexperience with it), and the fact that this is another cron module I think you would be perfect to check it out and give accurate input on the situation.

Currently, anyone following

tim.plunkett's picture

Currently, anyone following the steps in Dealing with abandoned projects undergoes no code review, and its a task for the webmasters. Instead, I think it could become a part of the Full Project Application, where they have to create a sandbox to demonstrate their port/rewrite of the abandoned module. That would certainly merit the "vetted git user" flag, and might even cut down on duplication of existing modules.

From someone who has a

mjpa's picture

From someone who has a project in the application queue, to me, the process is off putting to people trying to get vetted.

There are modules with no comments that are weeks old.
There are modules that have had replies (mentioning the project is fine) yet the project is still stuck in "need review" stage and is going no where.
There are modules that get 1 review and the user is vetted quickly (the way it should be), although this looks rare.

While it was good there was a code review drive the other day, without that happening often, the queue is just getting longer and longer and looking less cared for.

thank you for your comments.

zzolo's picture

Hi @mjpa. Your comments are very valid and this is a place for complaints, but what would be good is if you provided ways for this to be better (that are reasonable). If you look at this groups as a whole, you will see there are tons of discussion happening about what is good and what is bad and what could be done to make it better. It is not going to get better overnight. I encourage you to help out where you can. Many thanks.

--
zzolo

Hi, I have a module that I

alex dicianu's picture

Hi,
I have a module that I want to share with the drupal community, I've added it as a sandbox module, done all the modifications the comunity suggested, but for a week or so, my module is stuck in the project applications queue with the "needs review" status and looks like it will stay like this for a while http://drupal.org/node/1399106. I guess that, since nobody gets paid for this, we all have to land a hand ... But my question is, after all the modifications are done, who is the person that gives the GO? Can other users, that already have modules as full projects, do it?

Thanks.

Anyone can sign it off as

Niklas Fiekas's picture

Anyone can sign it off as "reviewed and tested by the community". Then an admin will look at it once more and eventually grant you the git vetted role. Thank you for your patience so far. Looking at it now.

But the way, you are

penyaskito's picture

But the way, you are encouraged to review other applications.

There's a @klausi initiative of reviewing those projects whose authors are reviewing before any other. More reviewers are participating in that experiment.

See http://drupal.org/node/1410826 for more information.

--
Christian López Espínola (@penyaskito)

Hi guys, Thanks for your

alex dicianu's picture

Hi guys,
Thanks for your replies. I'm just beginning to understand how things work in the drupal community ...
I read the page you showed me, I think I'll give it a go and try to review a module.

PS: Thanks @Niklas Fiekas for reviewing my module.

Code Review Priority needs clarification

jasonrichardsmith@gmail.com's picture

Does code review priority mean anything.

The docs would leave one to believe that it would move you up in the review process.

Bu I was told in IRC:
klausi1> jasonrichardsmit: yeah that also exists in theory, but i think it is not widely used by reviewers. sorry for the confusion

So what can we expect the code review priority to mean?

At the end of the day,

greggles's picture

At the end of the day, everyone is a volunteer and lives by their own rules. There is no official SLA regarding the issue queue priorities.

If you want a fast response, it seems the review bonus is a pretty reliable way to get one if you do good reviews.

Just taking a look at the

dubcanada's picture

Just taking a look at the Project Application queue and there are currently 220 open issues, taking a look at everything in review, and we see there are applications that have been in "Needs Review" for 2+ months. 2 pages of "Needs Review" applications. 31 applications in RTBC.

Something needs to be done about these, this is not only turning people off contributing to Drupal the first time. It's making it look like this isn't active anymore, so people just stop trying.

There is a list of about 20 or so "git admins" yet I only see about 4-5 of them being active. Maybe it's time to clean the ranks and get some new ones in? I'm not sure, but this current "Project Application" system just isn't working...

Anyways these are just my thoughts, it's becoming frustrating that things like contributing patches, and being a member of the Drupal community for over 2 years, active on both Drupal SE and other areas of the community means that I am a complete noob in regards to Drupal. I imagine there are other people in the same place. Most of which have been turned away by the extremely rigorous "Project Application" system.

That's all fine and dandy,

dubcanada's picture

That's all fine and dandy, talking about it is good.

Most of those threads are over 6 months old, and some of them are 1-2+ years. Yet nothing has changed. If there is such a large amount of "discussion" obviously something is not working.

This discussion has been

perignon's picture

This discussion has been going on for a lot longer than 6 months. We all have been voicing the same things; however, one must realize it comes down to someone standing up to make the changes. It is a volunteer system. I myself have tried to help by reviewing modules as much as I can but I cannot dedicate the time that's needed to be a full project reviewer. Wish I could. Klausi has stepped back from the work because he has other things in his life. And what you are seeing is a direct cause of him having to step down, no one as taken his place. He was doing the majority of all the reviews.

The list of Git admins. That page probably needs to be revised. Several of those people are git admins because they need them for reasons of working on core. Even though that list is fairly long, the only one that was focusing on project applications was klausi.

Bottom line... someone has to volunteer to get the work done. So far all the talk has been just that, no one has "put their money where their mouth is" to include myself!

The problem (as I have said

fuzzy76's picture

The problem (as I have said in numerous of the discussions linked above) is not too few reviewers. The problem is that we actually have a process that relies on manual volunteer work. Drupal is growing, and with the system in use today there will never be enough reviewers. Ever.

And one man can't change the system. If I started auto-approving git applicants because I didn't like the system, I'd be thrown out fairly quick.

Best part IMO of D8: No reliance on D.O.

Brian Altenhofel's picture

I personally think that one of the best features coming from D8 is no longer having to rely on Drupal.org for package management.

Some auditors for compliance with various regulations (both .gov regulations and industry self-regulations) like to see a package management system in place for managing notifications of out of date software. What that basically means in the case of a Drupal website/application is that the module better be on Drupal.org unless it is truly custom to a very specific use case that is limited to that client.

The project application process in it's current state is bad for business. It creates an unnecessary reliance on a third party that the business has no control over which is something no sane business owner wants.

I'm on my third project application. My first one disappeared at a time where D.O was having some issues. It wasn't a big deal to me at the time because I was only working on Drupal as a hobby at that time.

My second application was commissioned by a client with a stipulation that it could only be open-sourced with sponsorship attribution, and that compensation for that part of the project would be contingent on their public acknowledgement of sponsoring open-source software. Unfortunately, several weeks after my project application had been submitted and I agreed to drop my project to collaborate with someone else's project application, someone else with full project access started a project to do the exact same thing, so the application was declined as duplication of efforts. See what happens with a reliance on a third party that you have absolutely no control over? Plus, it was rather discouraging to perform due diligence ensuring that a module did not exist only to have the application declined a month later because a module was created by someone with full project access after the application was started. For a long time, I actively decided to not participate in releasing code to D.O. because of that experience.

I do a lot of e-commerce work and third-party integrations now which has severely limited my ability to submit code to D.O. But recently, several potential clients and partnerships have placed a high priority on projects being hosted on D.O. This was rather evident with leads generated through past community event sponsorships. The most common reason given for not closing a sale was lack of projects on D.O, with two specifically citing lack of ability to create projects they'd like to sponsor. With what was left, I basically broke even on those community sponsorships. That really discourages me from doing bigger things like upgrading from an organization member to a supporting partner of DA or committing to a Drupalcon sponsorship because on paper those look like they will be net losses.

Drupal 8 solves the issue of relying on Drupal.org by allowing modules to be managed by Composer. Sure, it takes a little massaging in it's current state, but if Drupal gets around to implementing Composer correctly in core it will be a lot easier.

To me, the current project application process fractures the community. It discourages new developers from participating. Much of the feedback since the implementation of the Review Bonus system is rather useless (with some of it showing a complete lack of knowledge on the part of the reviewer). It frustrates veteran developers who have an established track record yet just happen to not have full project access. It puts those who have full project access at a distinct professional advantage over those who don't have full project access.

Right now I'm waiting on a rather simple module to get reviewed in order to get access to create full projects (been 3 months since last review).

@Brian Altenhofel I feel

kreynen's picture

@Brian Altenhofel I feel your pain.

I've opened to
https://www.drupal.org/node/2316955 to discuss allowing modules and themes that are developed on GitHub to be pulled into distributions on Drupal.org. While this workaround currently only benefits distributions, it would open the door to more development on GitHub where the decisions about how to structure a project are made by the people doing the work. Obstructions like this are more difficult to put in place on GitHub because routing around them is as simple as clicking fork... and yet, without a project review process vetting new contributors, very large projects thrive on GitHub.

Like most contributors who already have the permission to create full projects, I haven't paid much attention to the project review process. But now that it is hindering my ability to collaborate effectively on Drupal.org, I want to see this process modified. Some basic functionality we need for https://www.drupal.org/project/cm_starterkit_easy is being held up by this process. Until recently I was unaware of steps other people have taken to route around this problem as we've tried to bring more people into our project. Routing around the issue is actually very easy to do if anyone involved has the permission to create full projects.

Since most Drupal shops have 1 or more people who already have this permission, I suspect they are all working around this process rather than subjecting their new developers to it. I would love to see the numbers for users employed at the largest Drupal companies with Drupal.org user acconts, the number of vetted those users that have the vetted user permission, and how many of them went through this process.

I've skimmed dozens of project review applications, but haven't found any requests for reviews from new developers at any of the large Drupal shops. There may be a few in the queue that I've missed, but I suspect that routing around the review is standard practice. Intentionally or unintentional, this creates a barrier to getting involved that benefits the businesses who employ many of the project review admins.

Before the D7 upgrade of Drupal.org, anyone with permission to promote projects could be added as a co-maintainer and promote the project even if the owner of the node didn't have that permission. Now a user with permission has to create the project or it must go through this process. So the work around is the permissioned users creates a project, adds the non-permissioned user as a co-maintainer, and (optionally) request that ownership of the project be given to the other user.

What does Drupal gain from this? What are we stopping non-permissioned users who are already co-maintainers or have taken over an abandoned project from doing that they can't already do on Drupal.org?

It hasn't always been this way. Go far enough back you'll find examples were CVS access was given to people who've never written a line of code just for asking so they could help manage projects (https://www.drupal.org/node/873930 ). CVS access was required for someone to even be able to assign tasks to other users contributing to a project. It was a radical notion that non-developers could use the Project module to help manage a project. Now any user can be added as a co-maintainer and even own a project without the vetted git user permission.

Unless I'm missing it, after the D7 upgrade of Drupal.org it was no longer possible for a vetted user to promote another user's project. The only way to get a sandbox project promoted or gain this permission became this process.

Developers who've contributed patches, co-maintained, and maintained abandoned projects for years can have their progress delayed by weeks by the project review process.

And when did it become acceptable to have policies and a process that actually discourages collaboration?

By suggesting what I've always thought was the "Drupal way" in https://www.drupal.org/node/2224023, after 4 months of jumping through hoops @nyariv will lose any progress he's made towards becoming a vetted user if he collaborates with @Chris Gillis and I on https://www.drupal.org/project/views_isotope.

You are encouraging @nyariv to come up with excuses about why Isotopia different just to get it approved instead of contributing to improving a project with > 8,500 installs that would benefit from some of what was in Isotopia.

https://www.drupal.org/contribute/development

The Drupal community holds a strong "collaboration rather than competition" ethos, which values joining forces on improving one awesome project rather than building several sub-standard ones that overwhelm end users with choices.

Opting to collaborate vs. creating a project with functionality that is similar to an existing project should be rewarded when vetting users. This permission should never have been tied solely to creating new projects.

I know it's dated, but I haven't found any indication that @jthorson's comments aren't still the consensus of this group...

...then the only thing to be gained by denying the 'create full projects' permission is a second chance for the applicant to prove they can abide by an application process that, if successful, they will never need to do again.

I see the same refusal to grant vetted git access by reviewing an abandoned module that was taken over in https://groups.drupal.org/node/266858

@jthorson is wrong. The other thing gained by all of this is Drupal's reputation for being a difficult project to contribute to and a growing community of developers looking for alternatives to Drupal.org.

There are almost 17,000 repos on GitHub referencing Drupal. Some of these are clones and forks of modules on Drupal.org. Some are full Drupal sites. Some are javascript libraries that are used in Drupal themes and modules.

But hundreds (if not thousands) are Drupal modules and themes that someone decided not to commit to Drupal.org. There are obviously many reasons someone might decide to use GitHub over Drupal, but you are kidding yourselves if you don't think this process is contributing to that.

Adding to my frustration is incentivizing developers new to Drupal with review bonus karma to provide feedback on other modules. My experience has been that in attempt to provide some feedback, these karma seekers can suggest changes that make the code worse. @vanyamtv was being told to add unnecessary paths to includes and that he shouldn't even use CiviCRM because there isn't a supported release on Drupal.org.

I've done a few reviews in attempt to get a collection of Feature exports the required karma necessary to get it promoted, but the more time I spend in these queues the more frustrated I get that this process so much influence over how projects are managed on Drupal.org.

Managing a project that contains Feature exports contributed by site admins using a distribution that will make new installs easier for new users doesn't require someone with Drupal development skills... just a solid understanding of Drupal, Features, and Git.

While there are a number of ways I can work around this specific issue, I can only do this because of the permissions I already have.

I have contributed to Drupal using Drupal.org for years and I am considering moving a large part of our project to GitHub because this process isn't something I want the people we are trying to get more involved to experience.

I can see that people have been complaining about the process since it was put in place. The defacto response is to suggest the complainer suggests improvements and/or getting involved. I don't have time to get more involved in yet another area of Drupal.org, but these are my suggestions:

  • Review new versions of abandoned projects that have been taken over by new developers as if they were sandboxes
  • Create criteria other than creating a new project that would allow collaborators with substantial contributions to request to be a vetted user based on those contributions
  • Highlight users who have gone through the review process in some way on their profile with a badge like reference to how they were vetted. "I've Been Vetted!" Distinguish these user from all of the users like myself who have been grandfathered in.
  • Encourage users who have been grandfathered in to get vetted. I guarantee this process would change quickly if that happened.
  • Stop encouraging users to give feedback on modules that integrate with systems they have little or no understanding of. New developers shouldn't be told they are implementing CiviCRM wrong from someone who's never used it.
  • Allow project manager/org owner accounts to either create full projects without needing to have new code vetted and/or revert to the previous configuration where vetted users could promote a project as a co-maintainer. This is done by the organizations that have been grandfathered in, but wouldn't be possible today.
  • Solicit feedback from the user who walk away from the process. Periodically publish a summary of those results. Did these people just stop using Drupal. Why? Are they still using Drupal, but not Drupal.org? Did they move to GitHub? Are the contributing to another project like WordPress?

I'm going to give the specific issue I need resolved a few more days in the queue, but after that I'm going to find use a different approach to get the Feature exports we want into our distributions.

Drupal users at big Drupal shops

dsnopek's picture

So, I don't know about the other stuff in this post, but I just wanted to comment on one part:

Since most Drupal shops have 1 or more people who already have this permission, I suspect they are all working around this process rather than subjecting their new developers to it. I would love to see the numbers for users employed at the largest Drupal companies with Drupal.org user acconts, the number of vetted those users that have the vetted user permission, and how many of them went through this process.

I've skimmed dozens of project review applications, but haven't found any requests for reviews from new developers at any of the large Drupal shops.

I know of at least one in the queue currently:

https://www.drupal.org/node/2173359

It's Josh Turton from Phase2. :-) I don't know how representative that is, though.

You said: I haven't paid

greggles's picture

You said:

I haven't paid much attention to the project review process

As a wise man once said:

but when you don't know anything about [a process] just say that and leave it at that. Who wants to read a [bunch of feedback] that starts with "I have never used XXX, but even though I'm completely uninformed these are the things I think you should change..."

Notwithstanding that potential problem, you've got a lot of great insights into ways this process can be improved.

You asked what the benefits of this process are. They are many, but I'd say that the main ones I care about are related to licensing, security, proper use of projects and git repository space.

Basically what we're doing now is using the creation/approval of a new project become a way for people to prove that they know a lot of things. Since it's a proxy it has a variety of problems. Michael Hess spent a lot of time working on a quiz that would be on drupal.org to try to let people show that they know some information without having to create a module. I think this could be a good solution.

I myself circumvented the

perignon's picture

I myself circumvented the process of being a vetted git user by being added as a co-maintainer for other modules. So what is said here is true. There are some fundamental loop-holes in this process.

Dive into the technical debt that resides on Drupal.org and you open up another gigantic can of worms. I keep saying that the review bot needs to be unleashed on existing projects. And those projects be given a 30 day warning to be taken offline as a supported module if they don't conform to standards. Don't be nice. Be brutal. If you want to use a standard, then you need to have the balls to support it and back it up. If that means taking down enormously popular modules such as Views then so be it. No one is above the law no matter how popular they are. Easily put, make it a security violation and red-flag their module.

Kudos to Phase 2 for eating

kreynen's picture

Kudos to Phase 2 for eating this dog food. Are there others from Acquia, Mediacurrent, Palantir, Commerce Guys, Blink, Trellon, Cap Gemini, Wunderkraut, Metal Toad, etc (I have no idea if these really are the biggest shops in terms of developers, but they seem to always be hiring)?

If the plan was to unleash bots, we could scan the repos for 3rd party code/assets and GPLv2 compatibility violations first. That will likely eliminate a large number of projects (maybe 50% of themes) before even evaluating the quality of the code. This was suggested and rejected by the group discussing updating Drupal's policies on GPL licensing and 3rd party assets. You can read the notes from that meeting and discussions surrounding it as well as a draft of the presentation of recommended changes Holly Ross is planning to make to the DA on 8/20 at https://www.drupal.org/node/1449452#comment-9026367.

If the recommendation is followed and a new Licensing Team is formed, it will be charged with actually enforcing policies we are willing to enforce. I've been referring to this as the JQuery Update clause since a 3rd party code violation in that module was pointed out in 2007, but nothing was ever done.

Trying to be proactive about licensing creates additional liability for the DA. Trying to be proactive about code is actually the opposite of what I'd like to see.

My suggestion about asking developers who have been grandfathered in to be "revetted" would go along way toward improving contrib, but scanning contrib for crap code is somewhat redundant. Contrib is scanned manually be developers looking for existing solutions. The projects that get higher usage have been vetted to some level. Modules with low usage should be used with caution.

Projects are also getting vetted before they are included in distributions. If you see a project or patch is added to Commerce, Commons or OpenAtrium, it's been vetted.

GitHub is a free for all. It is a bazaar and innovation thrives. Lately it seems like Drupal is trying to build a cathedral.

The move to D8 will kill off a large part of contrib on it's own. There's no need go looking for even more problems.

The current project review process is far too tedious for something like a Features export. The bot based code reviews actually find issues in a Features exports "out of the box". @avguy went ahead and modified the export to meet this group's definition of properly formatted code which obviously differs from the Feature maintainers definition.

If you apply the bar that a new Feature export based project has to meet for a project review to existing Drupal distributions, I doubt any of them would make the cut since the majority include Feature exports.

My point is I have the permissions required to poke a hole in this process for distributions. I'm guessing if I do that, we won't be the only project that makes the move.

I don't really want to do that. I know there is value in security reviews, update notices, linked issue queues, and packaging on Drupal.org, but I'm not going to watch good, passionate people quit contributing because it takes several weeks for "Drupal" to approve a Feature export so it can be included in a distribution.

Comment on kreynen's seven suggestions

gisle's picture

@kreynen, thank you for moving this subject forward in such a constructive manner.

Below are my comments on the seven bullet points for reform of the review process you suggest:

  • Review new versions of abandoned projects that have been taken over by new developers as if they were sandboxes.

Since you link to my application to illustrate this, let me add that I finally were given permission to create full projects by taking over an abandoned project: https://www.drupal.org/node/2287023. Let me also add that I learned a lot about the Drupal API and security in the process. As one who has eaten this dogfood, I say that I found the review process educational and rewarding.

I think this bullet point is now 90 % in place. See Abandoned Project Applications for guidelines for reviewers and https://www.drupal.org/node/2291419 for a recent example.

As for the remaining 10 %: We need to provide better documention to applicants about how to proceed with the application after owership/transfer has happened (I plan to add it when I can find some time).

  • Create criteria other than creating a new project that would allow collaborators with substantial contributions to request to be a vetted user based on those contributions.

Agreed. Maybe the current group of code review admins: https://groups.drupal.org/node/142454 can also serve as a "review" board where applicants that believe they've made "substantial contributions" can send in a CV with their qualifications and contributions, and become vetted based upon that?

  • Highlight users who have gone through the review process in some way on their profile with a badge like reference to how they were vetted. "I've Been Vetted!" Distinguish these user from all of the users like myself who have been grandfathered in.

Currently, I would regard this as a "rookie badge". The process needs to gain a lot more status before such an badge would be something I would want on my profile page.

  • Encourage users who have been grandfathered in to get vetted. I guarantee this process would change quickly if that happened.

The vetting process is seriously underpowered. Asking users who have been grandfathered in to get vetted would make it even more so.

As a sometimes reviewer, I'd say you learn a lot about the Drupal API and security by reviewing other people's code - at least if you take the time to look up the reference material while doing so to make sure your review is more than just a random opinion.

I would instead encourage users who have been grandfathered in to become reviewers. That would balance the rookie reviewers that the review bonus program creates.

If we can find a way to incentive users who have been grandfathered in to participate in the review process, I think this will greatly benefit the review process and also help those reviewers become better drupalistas.

  • Stop encouraging users to give feedback on modules that integrate with systems they have little or no understanding of. New developers shouldn't be told they are implementing CiviCRM wrong from someone who's never used it.

We don't actually encourage this. However, since the review process is open, people sometimes submit projects that is almost impossible to review without having very specialized skills.

For example, this one https://www.drupal.org/node/2268473 cannot be properly reviewed by someone unfamiliar with the various non-Drupal frameworks (behat, selenium) the project leverages on.

I think perhaps we should discourage apprentice projects that cannot be reviewed stand-alone, but requires the reviewer to have good knowledge of some external dependency.

Just discouraging inexperienced reviewers would slow down the whole review process to an almost standstill, as experienced reviewers are few and far between.

  • Allow project manager/org owner accounts to either create full projects without needing to have new code vetted and/or revert to the previous configuration where vetted users could promote a project as a co-maintainer. This is done by the organizations that have been grandfathered in, but wouldn't be possible today.

There need to be a vetting process for organizations (otherwise anyone could bypass the system by creating a single person "organization" account), but I see no problem having a process in place to allow some well-known organization (e.g. Acquia, Mediacurrent, et al) known for its commitment to Drupal have this right.

Note: We should still insist that all commits to a project owned by an organization are made by individuals, re: https://www.drupal.org/node/1966218.

  • Solicit feedback from the user who walk away from the process. Periodically publish a summary of those results. Did these people just stop using Drupal. Why? Are they still using Drupal, but not Drupal.org? Did they move to GitHub? Are the contributing to another project like WordPress?

It would be great if somebody could find the time to conduct such a survey.

The vetting process is

fuzzy76's picture

The vetting process is seriously underpowered. Asking users who have been grandfathered in to get vetted would make it even more so.

Which is because the vetting process is dependent on volunteers (with nothing but a couple of wiki pages guiding them) putting in hours. Which will never scale, and will always be completely unpredictable. IMHO, that is the problem people should focus on solving.

There isn't an option on

kreynen's picture

There isn't an option on https://groups.drupal.org/code-review for filing praise, but since I'm always quick to criticize I wanted to pass this along as well. I'm not sure if this was result of changes to the PAR process, the type of project, or just timing, but there were 2 helpful, high quality reviews added to https://www.drupal.org/node/2646234 within 2 weeks of opening the PAR issue. Progress?

Code review for security advisory coverage applications

Group organizers

Group notifications

This group offers an RSS feed. Or subscribe to these personalized, sitewide feeds:

Hot content this week