This is a discussion stemming from a module review here:
http://drupal.org/node/1822068
In this situation, the user has submitted a module that works, has been reviewed with no blatant issues found. However, a user can go in to configure the module, and input an incorrect path. This will result in a 404 error.
A_thakur believes that it is a requirement for passing the application process that the path is validated as legitimate.
I disagree that this should be a requirement. YES, it would be a nice feature, but it works as is. I don't feel it's appropriate to require additional functionality unless it's in the name of security or proper usage of API. I also feel this plays into the whole approval process taking too long.
So, please weigh in on this, and if this is a requirement let's add it to the documentation as such.
Comments
This depends where the input
This depends where the input validation is considered. In this case http://drupal.org/node/1822068, the validation is a requirement not a feature request as I had already mentioned that a wrong input can lead a user to invalid page. So why not check this redirect url while it is being entered.
RTBCing a project takes time, but in case it is RTBCed with a wrong functionality then I believe we are getting another broken module. In this I feel the validation is a requirement not a feature request.
"Good understanding of API" includes using it for validation?
A good thing having moved this discussion out of the particular application issue queue.
If looking at What to Cover in an Application Review there are some elements that are clear like "Security" or "Module duplication". "Overall understanding of Drupal API's" would cover also showing you are able to validate user input where it could be expected.
The definition of "where it could be expected" remains subjective. A field like "Number of items to show" should only take numbers but should a field "Country" be validated on being an existing country?
Also what makes validation more or less important is if the user is an admin that sets a config page or a visitor that fills in an email address.
I understand that the original poster wants to exclude any validation all together, but the more elements you take out, the harder it gets to get an idea if there is an overall understanding of Drupal API's.
It's inevitable that something like "Overall understanding of Drupal API's" remains subjective and is tied to personal opinions.
For sure, consider this. This
For sure, consider this. This is what Klausi writes on a lot of modules when he grants permission to create full projects:
"Although you should definitively fix those issues they are not critical application blockers, so ..."
I feel that validation, unless it can cause PHP errors (letters where numbers are, resulting in botched equations) or introduce security issues, isn't a CRITICAL APPLICATION BLOCKER.
Useful. Yes.
Good Idea. Yes.
Critical application blocker? I say no! Let's get out of this guys way and toss it in his issue queue as a feature request!
Jesse Nicola -- Shredical six different ways to Sunday! -- My Portfolio
Let's get it out of the way here but not put it aside as a rule
Agree that in case of the review mentioned above, after an application issue queue of over 50 comments and more than three month old, it is not appropriate to put it as a hard requirement.
On the other hand, speaking more in general, substantial part of the Drupal API is in place for validation. If project code does not make much use of the API and validation of a field is reasonable common sense (could be expected), then it might be considered critical to show "Overall understanding of Drupal API's". But validation is not the only way to show understanding of the API. The developer might come up with other code to show they make use of existing functions instead of reinventing the wheel.
Aside from the particular case above, in general it is acceptable during a review to ask validation of some field if it could be expected. If the rest of the code doesn't make use of the API enough it might come closer to a requirement than a feature request. It remains subjective and that's why we should be grateful we have some capable people in the role of Admin. They can make a fair judgement taking in consideration more than just one element to put on the plate.
Please, let not consider validation as something that an applicant can put aside as being a feature request too easy.
All very good points. I can
All very good points. I can see many situations where a need for validation is essential and should be required.
I wonder if we could try and come up with some variety of semi-distinct ways to determine if it should be a feature request or not.
Jesse Nicola -- Shredical six different ways to Sunday! -- My Portfolio
For what it's worth, my two cents ...
Above all else, please keep in mind ... the role of reviewers in this process is that of a 'mentor', not 'traffic cop'. Our job is to facilitate the learning process for project applicants, not to play judge and jury. Given the amount of pain we already inflict on potential contributors via an over crowded and under managed queue, I personally feel the focus for every reviewer should be on doing everything possible to help people reach RTBC, rather than finding reasons and excuses not to.
That said, there are a few areas where we do ask for perfection ... security being one of them. 'Proper' usage of the Drupal APIs is another ... but the goal here should be i) to identify and correct cases where these are being misused, ii) to identify and point out where usage of Drupal APIs is required in order to facilitate security/translatability/accessibility/etc, or iii) to make the user aware of a Drupal API function which can be used in place of existing code (typically because we've added a layer of security or functionality into the API). In my mind, while input validation should definitely be pointed out and suggested as a good coding practice, we can't demand it ... one can easily make a case that they want to enable a workflow where administrators configure the redirect and then go create the page after the fact.
One thing that sticks with me as I've read through this and other recent reviews in the queue is the level of 'absolute-ness' in many reviewer's comments. I recommend reviewers try to stay away from 'black-and-white' terms, and try to phrase things in a gentler manner. One technique for softening the message and avoiding the natural 'me vs. them' reaction from applicants is to try and avoid using the word "you" ... this forces wording such as "I'd recommend" and "I suggest" instead of "you should" or "you need to", which naturally elicits a less confrontational response.
The role of reviewers in this process is that of a 'mentor'
Rightly said...the role of reviewers is that of "mentors" and not "traffic cop". I understand the person who had created the project application might feel a bit dejected when the project is not RTBCed, but please look to the brighter side, in this process there is a great deal of learning. I would have never changed the status to "needs work" in case I felt this was a feature request, as according to my understanding it felt like a requirement.