CVS edit link for SimmeLj

Hello,

I work at Good Old where we (co-)maintain a lot of modules, http_client, OAuth and Services to name a few. I work closely with voxpelli and Hugo Wetterberg.

The reason for me applying is that I've written an authentication extension for Services that allows the user to create a whitelist of domains, later outputted in the Access-Control-Allow-Origin header to allow Cross Domain XMLHttpRequests to be made to Services. I'd like to publish that module here.
The code for this

I'm also a bit involved in the Ægir community where I've submitted one patch.

I also have a bunch of modules in progress over at GitHub (https://github.com/simme) that I hope to one day bring to Drupal.org :)

What more can I say? I love contributing :)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

SimmeLj’s picture

Status: Postponed (maintainer needs more info) » Needs review
FileSize
14.69 KB

My contribution for my CVS account

apaderno’s picture

Status: Needs review » Needs work
Issue tags: +Module review

Hello, and thank you for applying for a CVS account.

As per requirements, the motivation should include a description of the module features, and a comparison with the existing solutions.

SimmeLj’s picture

Status: Needs work » Needs review

So as I wrote above, the module is an authentication extension to Services 3.x. It lets the user specify one or more whitelisted domains per endpoint.

The new HTTP-Access-Control specification (see http://www.w3.org/TR/cors/ and https://developer.mozilla.org/En/HTTP_access_control) let's a server control what sites can make XMLHttpRequests (AJAX) to it's resources.

Usually the browser will block cross domain XMLHttpRequests for security reasons. The HTTP-Access-Control is a way of letting the browser know that the JavaScript actually is allowed to make that request, so that it is not blocket.

Basically, this module allows other sites (all or the ones you specify) make AJAX requests to your sites API written with Services. :) It's a simple module and a simple implementation, but a useful one!

voxpelli’s picture

Status: Needs review » Reviewed & tested by the community

This is a good module and Simon is a good coder. This module follows Drupal's coding standards, works well and is a unique and useful extension of the Services module. I see no reason to not give Simme a CVS account.

Should be noted that I am a colleague of Simon - feel free to disregard my opinion if you feel I might be biased - I will mark this as reviewed anyway though.

brianV’s picture

I checked the module code (not that I don't trust you voxpelli, but I was curious about SimmeLj's implementation).

That said, this is a nice simple module. I assume the .git subdirectory and hidden files will be removed before committing. I would suggest the README.mdown be changed from markdown format to a text file, as a lot of potential users won't know what .mdown format is.

However, neither of these are enough to hold back the RTBC IMO. Thanks for the application, Simon!

apaderno’s picture

Status: Reviewed & tested by the community » Needs work
  1. There are many files that should be removed (including the .git directory).
  2. The code is not formatted as reported by the coding standards (http://drupal.org/coding-standards); see in particular the control structures, and the used indentation.
  3. Avoid to escape the string delimiter inside a string, especially when the string is passed as argument to t().
  4. Question marks should not be used in the title of form fields.
  5. Strings used in the user interface should be passed to t(). I take that Unauthorized origin. is shown to users from the Services module.
  6. The file CHANGELOG should be named CHANGELOG.txt (the extension should be present, and be in lower case characters for compatibility with Windows).
  7. The file README file should be a plain text file (and have a .txt extension as the CHANGELOG.txt file).
  8. Did you open a feature request to include the code in the existing project? If you did it, what was the reply of the current maintainer?
SimmeLj’s picture

1. Of course I'll remove those file. I didn't mean to include them. Just made a quick tarball when uploading it here. Sorry for that!
2. Fixed I believe. Please be more specific if I missed something.
3. Fixed.
4. Fixed.
5. The text "Unauthorized origin" is returned in the HTTP headers by Services, so I do not agree that it should be translatable.
6. Fixed.
7. Fixed.
8. No I did not. I do not feel that this is something that every project needs and it's better implemented as a plugin to Services. Services exposes an API to create plugins that handle authentication for requests and my opinion is that this is authentication related.

SimmeLj’s picture

Status: Needs work » Needs review

Sorry, forgot to change status.

gdd’s picture

Status: Needs review » Reviewed & tested by the community

As the maintainer of Services, I agree with SimmeLJ's evaluation of this. We made authentication pluggable for a reason, and that was to enable module developers to write their own plugins and release them for the community's benefit. So number 8 is not an issue. I support this application's approval.

zzolo’s picture

Status: Reviewed & tested by the community » Needs work

@SimmeLj, thanks for the application and patience. The following points are just a start and do not necessarily encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

Overall, this is very close, there are just a couple points necessary for approval, most specifically a syntax error.

Neccessary for approval:

  1. Your README.txt needs $Id$
  2.     '#description'    => t("Enter the domains you want to whitelist for this
          endpoint. One per line, remember http://. If you want to allow all
          requests, just enter "*". Please note that this module does not actually
          hinder origins not whitelisted from accessing the content, that's up to
          the browser to do."
        ),
    

    This is badly formed PHP (see the "). But my reason for even looking at this was that you should keep all the contents of a t() string in a single line, because it will be translated as is, meaning the breaks you put there will be presented to the translator.

  3. I agree that you don't want to translate error codes. But "Unauthorized origin." is not an error code, its an error message. You should either use an integer (which is annoying) or a more identifier friendly string, like "unauthorized_origin".

Suggested for a better module (IMO):

  1. /**
     * Authenticates a call by checking the origin agains the whitelist.
     *
     * @param     array         $settings
      *  The settings for the authentication module.
     * @param     array         $method
     *  The method that's being called
     * @param     array         $args
     *  The arguments that are being used to call the method
     * @return    void|string
     *  Returns nothing, or a error message if authentication fails
     */
    

    The documentation is awesome, but checkout the coding standards to do it better (well, more standard).

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article, or read the handbook page on how to review for reference.

voxpelli’s picture

Seriously - how nitpicky do we need to be? Does everything but a totally spotless module need to block the CVS application of someone? If it really do then at least please suggest good solutions to the errors you point out…

2. What solution are you suggesting for the line breaks? Do you suggest putting everything on a single, but extremely long line? Not good practice in my opinion. Do you suggest concatenating multiple strings together? Not good practice either in my opinion, since that makes it hard to parse out translatable strings. So what solution are there?

3. This module is only responding the way Services wants it to respond - see line 112 in services.runtime.inc. What fix are you suggesting?

To summarize: You feel the need to put this CVS application on hold due to the lack of one $Id$ and the obvious mistake of writing a "*" instead of '*'. Is that really needed? Does that really help anyone at all? In what way?

Edit: Btw - forgot - in what way are the documentation not very standard in your opinion? Rather than pointing to a reference document please point out what part you feel isn't standard. The whitespace? The descriptions? The parameters? Everything?

zzolo’s picture

Hi, @voxpelli. I appreciate your feedback. I don't feel that I am being nitpicky at all; there is an obvious syntax error that means that the module won't run and will break a site. I don't feel that a broken module is good for the community. And as the application requirements point out: "You must submit a finished, working module or theme for review along with your application".

And as I pointed out, the syntax error is the biggest thing stopping this. The other things are valid as well but not as important. Please talk with people that actually do translation and they will tell you that that string with line breaks is annoying and would cause confusion. Yes, a single line is appropriate if the string is a complete sentence or paragraph. This may not be the most friendly format in code, but the purpose of it is to be translated, not to look good in an IDE.

As far as those lines in that file, I just see a call to services_error() which does not show me what I believe you want me to see. My point is still valid above. It is either an error message or error code, and if it is an error message, it should be translated, as translatability is very important.

It is quite frustrating to see someone that has time to criticize my review, but looking at your history has not really spent much time on actually reviewing applications. I have spent a lot of time discussing, documenting, trying to improve this process while still keeping its value, and reviewing applications. This is a subjective process and will always be. If you feel my review is not the best, then please feel free to correct me, but not in such an unfriendly and unhelpful way.

voxpelli’s picture

@zzolo:

In response to the issues

  • Multiplying two strings with each other is not a syntax error - the result of that code is an integer value 0 which in turn only results in that the textarea doesn't get the description it should. No WSOD or similar.
  • The error message should be in the format that Services expects it to be no matter if that is english, numeric, binary or something else. Translation should only be used where it is expected - eg. in UI and similar.

In response to the criticism

The reason I'm criticizing you is since you're stopping someone from contributing to the community based on vaguely defined, minor issues - some which you seem to have misunderstood, some which are referring to standards you seem to have invented yourself, some which won't be relevant in a few months (#819874: Decide on a replacement for $Id$ tags in files) and some which are valid - but still very minor.

While feedback is always good - there's not always a point in blocking the application and certainly not in this case.

From "How to review applications", with my highlighting:

A good experience here can lead to a much stronger contributor in the long run, and creating a painful process may turn them away from contributing at all.

The reason why we have best practices and why we should follow them and ask others to follow them is to make code easier to understand and easier to develop upon. The reason is not to bully newcomers into perfection. Good enough is good enough.

Simon has proven to listen to feedback and to be eager to fix even minor things - there's no sign that he wouldn't fix the things you've pointed out the next time he's looking at the code. Still you think it's right to force him to resubmit new code fixing your issues before to being able to get CVS access - seems like some kind of bullying to me? If Simon was new to Drupal I don't think he would ever come back or ever speak good about Drupal again.

If the reason you blocked this application is that you thought it would cause a WSOD due to the "*" then you should have pointed that out clearly so that others could correct you and put the application back on track. Instead you've mixed it up with a lot of other minor, vaguely defined issues making it very hard to understand what problem you really think there is.

(Should point out that this rant is about vague feedback in general - I'm not against Simon's code getting good and fair reviews - all that feedback is great!)

SimmeLj’s picture

1. Oops, missed that file, quick fix! :)

2. I buy the argument for in-string line breaks, I did not consider that. However long lines are a nightmare to maintain, especially when they reached that length. I do much more prefer concatenation then, as it look's good for both the translator and the coder. I will also see if I can write a better and shorter description.

The syntax error will obviously be fixed. That would be even harder to catch if it was one long line ;)

3. @voxpelli is correct. If you had made a search, in that file you posted a link to, for "function services_error" you would've seen that that function throws an exception, which Services catches and returns as an HTTP-header. And I do not believe that headers should ever be in any other language then English.

I therefor stand by my point that this string should not be translatable. And I hope that you can change your opinion now that you know how it's handled.

I'd also love it if you could point out the exact problem with the documentation. I believe I've written Doxygen compliant documentation. Is it the aligning of parameter type and name? I know that is a personal thing and I can remove it if needed, I find it easier to scan though.

I really appreciate the thorough review because I do want to release quality software. I also appreciate the support from @heyrocker and @voxpelli. All those small things are easy to miss, especially when the module is developed quickly for another bigger project. I'll to get a fix going tonight after my girlfriend has hit the hay.

SimmeLj’s picture

Status: Needs work » Needs review
FileSize
15 KB

Fixed the multiplying strings thingy, added the ID-tag to the README file and clarified the untranslated string with a comment in the code.
I did opt to got for concatenated strings in the description (I also made it shorter and moved a bit of it to the .info file) since I strongly believe that this is the better option for me as a maintainer and it does not interfere with the way a translator works.

I also did nothing with the documentation since I believe that it is way better documented then most modules and the issues you (@zzolo) pointed out was undefined.

I really hope you'll let this one through now as I really want to start contributing to this awesome community! :)

SimmeLj’s picture

Messed up the archive. Here's a new, working, one.

zzolo’s picture

Status: Needs review » Reviewed & tested by the community

Hi @SimmeLj and @voxpelli. I will rebuttal some points above, but I will not continue with my review of this module and thus approval (though happy to put it as RTBC); I feel attacked on this application and it is not a positive discussion IMO.

On specific points

  • Yes, that expression would end up being the multiplication of strings and thus not break a site. I stand corrected. But I would still point out a glaring syntax error and hold back on approval because it shows that the module has not actually been fully tested. That's a pretty big description that should be missed if someone was testing a module.
  • I still believe that my advice about the translation string is appropriate. There is no documentation on it, but given my experience in this specific area, and that the topic had already been brought up, I thought it was a valid point.
  • I also still stand by my point about error messages and translatability. And doing some basic research found no indication that HTTP headers should be in English. I would have no intentions on stopping approval based on this, but I thought it was important to point out, and it is totally acceptable to have disagreements on things that are obviously thought out.
  • The documentation issue, as pointed out in my original comment, would not stop the approval. But it is also clearly documented, and yes it was triggered by your spacing, but also your use of defining a type.

Both of you keep saying that my comments were "undefined". And maybe to some degree they were brief. But I am not trying to write the module for someone, instead I am trying to point someone in the correct direction. My goals with comments are to help understand an issue, not just copy code. I also don't necessarily have the time to write a memoir on my experiences in the Drupal community and why I say the things I do, instead I try to help facilitate the applicant in finding the answers in the community more efficiently. I could have been more exact with comment about the Doxygen blocks.

@voxpelli, in response to your comments about my methodology. First let me point out that you quote me when trying to criticize me. I wrote that How To Review article from many hours of discussion, editing, revising, and feedback on this process. I have spent many hours on the process and helped many people become contributors to Drupal. I don't actually disagree with most of your points, as most of them are laid out in that How To. But I am extremely offended that you would attack me when my goal is clearly to be helpful and you have no obvious connection to doing application reviews.

I have made it pretty obvious in my footer of every comment of every application review, in the How To, in issues on this subject, in blog posts, and in person with community members that I feel this process should be a mentoring opportunity, not a jump through a ring of fire sort of exercise. I am trying to pass on my knowledge and experience, not ensure every coding standard is followed. I am sorry that you have perceived this as "nitpicky".

What I don't find helpful are comments like #4. "This is a good module and Simon is a good coder." I can't disagree with that, but how helpful is that to someone that is giving keys to the repository? How is that mentoring at all? If a module is that good, then you can easily give positive reinforcement comments, or give suggestions on how to improve it as it goes forward into the community. There is always room for improvement. My comments may seem minor and meticulous to some, but it at least offers the ability to show links and conventions that might have not already been known.

SimmeLj’s picture

@zzolo, I was only referring to your comment about the documentation when I said you were being vague. I do agree that you should not be the one writing the module, however being specific helps me fix things faster! :) I did not mean to offend you in anyway.

I fixed the syntax error and made the string look better for translators.

The module has been tested a lot, we are using it in a live project. However, in the theme we're using the description is only visible when hovering the textarea. Something I rarely do since I'm a big tabber! That is why I missed it.

I'm sorry you feel like you need to abandon this review. I really think that this module is something a lot of people using Services could benefit from.

SimmeLj’s picture

Status: Reviewed & tested by the community » Needs review

Setting status to "needs review".

apaderno’s picture

Status: Needs review » Needs work
  1.   $form['whitelist'] = array(
        '#description'    => t('Enter the domains you want to whitelist for ' .
          'this endpoint. One per line, remember http://. If you want to allow ' .
          'all requests, just enter "*".'),
        '#title'          => t('Accept Origin Whitelist'),
        '#type'           => 'textarea',
        '#default_value'  => $whitelist,
      );
    

    The first argument passed to t() is a literal string, or the script that extracts the strings to translate (which reads the module as a text file) will not extract them, and they will not be translatable. Literal string means that values like the content of a PHP variable, the result of a function, or the concatenation of strings are not values that should be passed to t() (or the effect would be the same as not calling t().

  2.   // Check no origin policy
      if (!$origin) {
        if ($settings['no_origin_policy']) {
          return;
        }
        else {
          return 'Unauthorized origin.';
        }
      }
    

    In Drupal core code I follow the following comment:

        case DRUPAL_BOOTSTRAP_ACCESS:
          // Deny access to hosts which were banned - t() is not yet available.
          if (drupal_is_denied('host', ip_address())) {
            header('HTTP/1.1 403 Forbidden');
            print 'Sorry, ' . check_plain(ip_address()) . ' has been banned.';
            exit();
          }
          break;
    

    Drupal doesn't use t() merely because the function is not yet available in when that bootstrap code is being executed; differently, the string would be passed to t(), which is what should be done for every string that is shown in the user interface or returned to the user.

  3. Form field titles are written in sentence case. (This is an example of sentence case.)
  4. The Doxygen comment for the function doesn't follow the format reported in Drupal coding standards; in particular, the parameter type should not be reported.
  5.   foreach ($whitelist as $domain) {
        // Make star always pass requests through
        if ($domain == '*') {
          $origin_is_whitelisted = TRUE;
          $origin = $domain;
          break;
        }
        elseif (strpos($domain, $origin) === 0) {
          $origin_is_whitelisted = TRUE;
          break;
        }
      }
    
      // Send headers and let request through since we got
      // an origin and it's in the list
      if ($origin_is_whitelisted) {
        drupal_set_header('Access-Control-Allow-Origin: ' . $origin);
      }
    

    The code could be optimized.
    If there is an asterisk in the white list, there is no reason to check the origin against the value of every item of the white list; it's enough to check if there is an asterisk in the white list and accept any origin, and compare the origin with any item of the white list if there aren't asterisks in the white list.

  6.     if ($settings['no_origin_policy']) {
          return;
        }
        else {
          return 'Unauthorized origin.';
        }
    

    Also that code could be optimized.

voxpelli’s picture

In response to point 1 in #20: Out of curiosity: That change was a fix in response to point 2 in #10 - what should the "correct" solution be according to you? Do you think it's acceptable with line breaks in a t() if they isn't concatenated? Or are you as #10 in favor of putting everything on one line? I'm in favor of linebreaks in t().

apaderno’s picture

As I reported, translations work if the first argument of t() is a literal string; this is because the strings to translate are extracted from a script that reads the module as plain text. Literal strings don't include concatenations of strings.

Using code like the following

  $output = t("This is an") . t(" example of wrong code.");

would be not correct because the sentence has been split, and the translators could have problems to translate a word without to know the word which follows it.

Using a new line character inside the string would cause the character to be present in the string to translate. I personally don't see any problem with this, as the translators would be free to use the new line, or not.

SimmeLj’s picture

Status: Needs work » Needs review
FileSize
4.53 KB

1. Now using new lines to break the translatable string in to a manageable block of text instead of concatenation.
2. Passing error message through t(). I still don't feel like it's the right thing to do. But I'll cave to the demands.
3. Fixed form title formatting.
4. Fixed Doxygen documentation.
5 & 6. Optimized code.

Thank you for taking a look at it! :)

apaderno’s picture

Assigned: Unassigned » apaderno
Status: Needs review » Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

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

apaderno’s picture

Component: Miscellaneous » new project application
Issue summary: View changes
apaderno’s picture

Status: Closed (fixed) » Fixed

I am giving credits to the users who participated in this issue.

Status: Fixed » Closed (fixed)

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