About the Project / Submission:

This is a SIMPLE and straight forward module that creates a block which can either already contain a QR code from the google charts API, or render the QR code on click (reduces HTTP requests, not always needed).

The need for this struck me on a recent freelance project for an apartment complex where they wanted to be able to have QR codes on all of their pages for staff so as they could quickly copy and paste QR codes from the website into promotional material they were creating.

There are infact other modules that deliver QR codes. I documented them in my project page. They do not deliver a simple solution, one of them was never ported from D6, and one of them delivers barcode functionality as well and again introduces complexity.

Project Page: http://drupal.org/sandbox/jnicola/1872858

git clone --recursive --branch 7.x-1.x jnicola@git.drupal.org:sandbox/jnicola/1872858.git google_qr_code_generator

Reviews of other projects:

Comment 1 - Pretty Pager(?) - http://drupal.org/node/1871644#comment-6872054
-learned about L() function
-Learned about not wanting to output HTML directly.
-Learned about theme_image() on my own to avoid typing html directly.

Comment 2 - Views Node Edit - http://drupal.org/node/1872452#comment-6872084
-Learned how to actually setup 7.x.1.x branch. and delete "master" thanks to a link I saw.
-Learned about http://ventral.org/pareview . Used this to bring my module from 100 plus errors down to just 4 total!

Review 3 - Delayed Ownership - http://drupal.org/node/1803114#comment-6872164
-Didn't learn anything new, but was impressed! Best module so far. Good code! Cool functionality!
-Saw some things that made me ask and discuss security.

Comments

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Issue tags: +PAreview: review bonus

Submit for bonus review. I thought it would be somewhat bothersome to do but I learned a lot from it all!

Heads up:

I know I have these errors from PAreview.sh. I don't know how to fix though... please advise :)

1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"

I've tried fixing this repeatedly. No idea what is causing this.

60 | ERROR | Constants must be uppercase; expected GOOGLE_QR_CURRENT_URL but

I don't believe I'm setting this up as a constant? I don't understand this.

162 | ERROR | Files must end in a single new line character

My document just has a single new line character for the end?

htumanyan’s picture

htumanyan CreditAttribution: htumanyan commented

Hi! I reviewed your module manually and didn't find any major issues apart from a few comments below:

a. I've noticed that you set the caching policy for the block as DRUPAL_CACHE_PER_ROLE, which implies that the block will be regenerated each time a user with a new role is accessing a page, where it resides. Looking at the block content I did not spot anything user/role specific. Would it make sense to use DRUPAL_CACHE_GLOBAL for better performance. Again, I don't think this is a major issue/point given that the generation of the block is not resource consuming.

b. Would it make sense to sanitize user-supplied configuration parameters before they are used for generation of end-user visible content? I do see that you have your custom validators upon submission of configuration parameters, but, nevertheless, felt it is reasonable to implement sanitization on the output.

Just my 2c. I hope you find this useful.

jnicola’s picture

jnicola CreditAttribution: jnicola commented

Good points, thanks for mentioning them :)

A- Didn't know this. I grabbed some sample code somewhere and that was leftover. I removed this line of code. It's been commited again sans that line.

B: I hadn't considered sanitizing, and don't know enough about it to consider what to do anyways. What would you advise (or some one else perhaps who sees this)

Further notes on my own admissions of errors:

-Fixed the constants issue. Forgot a $ on a variable. I do this sometimes because I am back and fourth between javascript so often!

-Went into VIM and tried adding the first line break and also the final line character... and it still throws those errors. Anyone with a solution?

jnicola’s picture

jnicola CreditAttribution: jnicola commented

Now that I think about the sanitation comment, I'm forcing the only values that can be changed to be integers, and constricting their values. Not sure what else could even really get through there? I don't think sanitation is a concern, but again... some one may know more than I do :)

sonnym’s picture

sonnym CreditAttribution: sonnym commented

Foremost, I would like to let you know that the git clone line above is incorrect - you want to use http://git.drupal.org/sandbox/jnicola/1872858.git as the URL, not the one provided by drupal.org with your username specified.

It looks like you can also remove some TODOs, as you have both participated in the Review Bonus Program and applied to create full projects!

Now for some actual review of the code. Overall, it looks really solid, the following are some minor tweaks.

  • Missing implements comments, e.g. google_qr_code_block_info and google_qr_code_block_view.
  • Use #attached key in the return value from an implementation of hook_block_view when caching is enabled. See: this comment re: hook_block_view

In closing, I would like to reiterate that this module is very cleanly implemented and looks solid!

jnicola’s picture

jnicola CreditAttribution: jnicola commented

Good points :)

I have removed my todo list (cause I am doing it!). I also adjusted the following:

1- Added code for global cache. Makes sense to cache in such a fashion since it's the same no matter where it is.

2- I used #attached to attach the Javascript. This should only load the JS when the block is called, and will function with cacheing. Good read thanks for linking!

Could some one explain the implements comments or link me to an article on writing strong commenting?

htumanyan’s picture

htumanyan CreditAttribution: htumanyan commented

The easiest to ensure that unsafe content never reaches the user is to use Drupal's sanitization functions (http://api.drupal.org/api/drupal/includes!common.inc/group/sanitization/7) when producing the output, i.e. instead of 

"&cht=qr&chl=" . $google_qr_current_url .

do

"&cht=qr&chl=" . check_url($google_qr_current_url) .
sonnym’s picture

sonnym CreditAttribution: sonnym commented

This is the specific documentation for hook implementations, but the entire document is worth a read.

Documenting hook implementations and theme preprocess functions

esbenvb’s picture

esbenvb CreditAttribution: esbenvb commented

You still have issues with windows-style line endings:

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/google_qr_code.module
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
82 | ERROR | break statement indented incorrectly; expected 6 spaces, found 4
184 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

This is also the reason why line 184 fails. Fix it by setting your editor's text format to unix.
Also, there's a space after <?php
I guess the break at line 82 should be removed since your return command already breaks the switch.

All your ON_PAGELOAD and ON_CLICK constants are not defined anywhere, this is not valid and PHP complains about that.

I also think these names are a little to generic for a module like yours, so you should consider just having the strings 'on_pageload' and 'on_click' instead...

esbenvb’s picture

esbenvb CreditAttribution: esbenvb commented
Status: Needs review » Needs work
jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Needs work » Needs review

I was aware of the EOL issue and it was boggling my mind as I couldn't fix it by editing the file in VIM. I Googled how to fix this in notepad++, and I for those interested in how to convert files to UNIX end of line in Notepad++, simply go to Edit > EOL conversion > Unix.

I switched from constants to strings. Thanks for pointing that out. Learned something new there. I was running off of an example I saw online for handling selects.

I added the check_url() function to both the image output AND the alt output since it includes the URL.

I updated the commenting to the best of my understanding on the topic. Can't say I found the documentation it overly informative, but I feel what I have for comments will help me through it. If I'm not writing them correct, please provide what I should write and a short explanation as to why so I can learn how this all works better, please :)

I am no longer showing any errors, and I beleive this should address everything anyone has said thus far. Module still works as advertised, project page is good. I'm feeling pretty darn solid about this module at this point. Thanks everyone for contributing to my learning experience with this.

Jordan_Fei’s picture

Jordan_Fei CreditAttribution: Jordan_Fei commented
Status: Needs review » Needs work

Hi jnicola,

Review issues for google.qrcode-execute.js:
1. "if(moduleBlock)" code line is redundant.

      moduleBlock = $('#block-google-qr-code-qr-code').html();
      if(moduleBlock){
        if(moduleBlock.length > 0){

For the "$('#block-google-qr-code-qr-code').html()", jQuery will always return an object. Therefore the if statement will always be true and never be false. Just use "moduleBlock.length > 0" is ok

2. "return false;" in click event to prevent the default behavior.
I don't believe there is a W3C specification for this. All the ancient JavaScript interfaces like this have been given the nickname "DOM 0", and are mostly unspecified. You may have some luck reading old Netscape 2 documentation. The modern way of achieving this effect is to call event.preventDefault(), and this is specified in the DOM 2 Events specification.

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Needs work » Needs review

Good point on the object always existing in jQuery, even if it contains nothing. Again, mixing up my PHP and JavaScript :)

Fixed the unnecessary IF and committed. Changes nothing functionally though... not sure you really needed to flag as "needs work" since it affect nothing, but hey, why not save a dash on file size!

Additionally... you are incorrect on return false, but only in this particular instance!

"return false from within a jQuery event handler is effectively the same as calling both e.preventDefault and e.stopPropagation on the passed jQuery.Event object."

http://stackoverflow.com/questions/1357118/event-preventdefault-vs-retur...

Full discussion and details available there. Outside of a jQuery event handler you are quite correct, but not inside of a jQuery event handler, which my .click() is. So, now it's my turn to share something with you :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Postponed (maintainer needs more info)

This sounds like a feature that should live in the existing qr_codes project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the qr_codes issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

I'm sure that a port to Drupal 7 would be very welcome.

If that fails for whatever reason please get back to us and set this back to "needs review".

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Postponed (maintainer needs more info) » Needs review

I'm well aware of collaboration over competition, and I'm aware of the alternatives and I documented it in my module. I chose to continue with this module despite all of this, for reasons beyond competition.

How's this:

I'll move this back to "needs review" as I want further code review. Even if the module goes nowhere I'll learn something.

While getting further (and hopefully final) code review, I am going to follow up with the abandoned projects process for the following reasons:

1- The original module never even left Beta.
2- There is an open issue for a D7 port, It's been open for a year and nothing functional has been delivered. The last comment from a month and a half ago is calling for reporting the module as abandoned.
3- Their module is NOT the same as this to begin with. It offers functionality beyond what my module offers and is therefore more time consuming to install and configure, and does not work right out of the box. Theirs is also NODE FIELDS dependent, while this is URL dependent (plays better with panel pages, Taxonomy term pages, views pages, etc etc). Theirs won't retain view arguments (or views at all) while this does. Adjusting their module to deliver this functionality would be a complete departure...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Postponed (maintainer needs more info)

So I propose this: since there is no D7 version yet you can simply rename your module accordingly and push it to a 7.x branch in qr_codes. Just follow the abandoned module process and you will become the maintainer. You can clearly state on the project page then that the 7.x version is a simple minimum implementation and that you are willing to accept patches that port features from 6.x. Benefits: no duplicate project and qr_codes finally gets a first 7.x version.

In the meantime I'm setting this to postponed to keep the project application issue queue organized. Feel free to come back to us anytime you have a new project that does not exist yet and you want to promote.

If that fails for whatever reason please get back to us and set this back to "needs review".

jnicola’s picture

jnicola CreditAttribution: jnicola commented

Just placing a comment here to document my progress:

  • Sent a message to Klausi with concerns over differences. He said reopen if I desire and state why. I decided to wait and first see the results from the abandoned module process.
  • Opened up a "still maintained thread" to start the abandoned module process on the QR codes project.
  • A maintainer posted, stating they'd be interested in the possibility of me taking over but was curious as to the differences. I posted all of the differences (they take significantly different paths).
  • Didn't hear back from maintainer in aforementioned thread. Sent them a private message a week ago.

If I don't hear back from him in another week I intend to re-open this as "needs review". My module is a significant departure from theirs, the maintainer in question who post up hasn't committed in a long time and isn't communicating very well, although they're communicating just enough that the module probably would not be classified as abandoned.

Overall, I'd like to get this module out there as I think people would benefit from it. It's a quick and simple solution that works (it's on two production sites and on two soon to be released sites) and I'm proud of :)

So... see you guys in a week!

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Postponed (maintainer needs more info) » Needs review

Okay, I am reopening this. Here's the situation on the possibility of getting involved with the QR Codes project:

1- I tried to get the module marked as abandoned. A maintainer responded, so it can't get marked as abandoned.
2- I discussed collaborating my module and integrating it with theirs. Radio silence ensued and I heard nothing else. I sent a private message at that time as well... and still nothing... for two weeks straight now.

So, I am reopening this module. I have tried to mark the module abandoned, but the contributors are just active enough it won't get marked abandoned. I tried to get involved and work with them possibly... but nothing, no a word back.

So i tried, neither option is in the cards, and my module is inherently different anyways for all the aforementioned reasons.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

Ok, so let's continue with this application then.

Review of the 7.x-1.x branch:

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/google_qr_code.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 144 | ERROR | Function comment short description must start with exactly one
     |       | space
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  1. "variable_get('google_qr_code_height', '250'))": all variables defined by your module must be deleted in hook_uninstall().
  2. google_qr_code_block_view(): why do you call drupal_add_js() four times? Just call it once with the whole array()?
  3. "'url' => $_SERVER['PATH_INFO']))": why do you need that? Please add a comment.
  4. google_qr_code_contents(): the check_url() calls are not necessary since theme_image() will sanitize it for you.
  5. theme_image(): do not call that directly, use theme('image' ...) instead.
  6. "'QR Code for ' . check_url($google_qr_current_url)": all user facing text must run through t() for translation. Do not put variables into t(), use string literals and placeholders where possible. Same in google_qr_code_max_total_px().
  7. "'access arguments' => array('administer users'),": that permissions does not make sense? Better use "administer site configuration" instead or create your own permission?
  8. "$GLOBALS['base_url'] . $_SERVER['REQUEST_URI']": why can't you use url(current_path(), array('absolute' => TRUE)) or somehting similar ?
  9. Notice: Undefined index: PATH_INFO in google_qr_code_block_view() (line 41 of google_qr_code.module). Please enable PHP notices in your development environment.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

jnicola’s picture

jnicola CreditAttribution: jnicola commented

All good things to know. I'll implement those changes and put it back in the queue.

jnicola’s picture

jnicola CreditAttribution: jnicola commented
  1. "variable_get('google_qr_code_height', '250'))": all variables defined by your module must be deleted in hook_uninstall(). Done. Added uninstall function.
  2. google_qr_code_block_view(): why do you call drupal_add_js() four times? Just call it once with the whole array()? Done. Didn't know you could do this. Did some reseach, did some tooling around... and got it to work!
  3. "'url' => $_SERVER['PATH_INFO']))": why do you need that? Please add a comment. No idea how I got this mixed up. I meant to use $_SERVER['REQUEST_URI']. Switched to $_SERVER['REQUEST_URI']
  4. google_qr_code_contents(): the check_url() calls are not necessary since theme_image() will sanitize it for you. Removing. It was added per the recommendation of a previous poster on here. I think you're a bit more senior than them though so I'll go with what yousay here!
  5. theme_image(): do not call that directly, use theme('image' ...) instead. Switched. Good to know!
  6. "'access arguments' => array('administer users'),": that permissions does not make sense? Better use "administer site configuration" instead or create your own permission? Good point. I hadn't really thought about this before. Adjusted per your advice!
  7. Notice: Undefined index: PATH_INFO in google_qr_code_block_view() (line 41 of google_qr_code.module). Please enable PHP notices in your development environment. Not sure where this came from. Notices and warnings are enabled! Anyways, this is fixed.

Questions regarding two other items you wrote:

"'QR Code for ' . check_url($google_qr_current_url)": all user facing text must run through t() for translation. Do not put variables into t(), use string literals and placeholders where possible. Same in google_qr_code_max_total_px().

So I understand moving any text heading out in the t() function... but I don't understand the rest of what you wrote, and I wasn't able to find a good answer on this, probably due to not knowing enough to ask the question properly. Could you please elaborate?

"$GLOBALS['base_url'] . $_SERVER['REQUEST_URI']": why can't you use url(current_path(), array('absolute' => TRUE)) or something similar ?

Alternatives do exist... but is there a particularly compelling reason NOT to use request_uri that I am unaware of? Unless this introduces a flaw or security issue, I don't understand why it would matter. If it does though, please enlighten me, I need to know if I'm doing something wrong!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

"'QR Code for ' . check_url($google_qr_current_url)" should be t('QR Code for @url', array('@url' => $google_qr_current_url));

The PHP super globals for paths, URLs etc might not be reliable, so it is recommended to use the appropriate Drupal functions to avoid issues with sites that don't have clean URLs enabled, where the Drupal installation lives in a subdirectory etc.

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Needs work » Needs review

Good to know about PHP super globals.

Okay, implemented the placeholder for url's, and also implemented the new method for getting the current path. Pushed changes to a few freelance jobs I have going on and works across all in staging, will hit Prod later this week. I feel pretty confident about most of what I've done thus far.

I am a little bit uncertain about my adding the uninstall code for the module without actually adding install code... but it works so uh... there must be something I don't know? I found the documentation on all of this to be a bit minimal, but it works and throws no errors anywhere. Please let me know if I missed anything though!

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Issue tags: +PAreview: review bonus

I did three more reviews. I tried going to the back of the list but a lot of the modules were pretty much abandoned in the process. Is there a way to mark module submissions abandoned?

Anyways, 3 reviews!

Comment #1 - Etsy Treasury
http://drupal.org/node/1889102#comment-6958038

Comment #2 - Contact Plus
http://drupal.org/node/1822068#comment-6958060

Comment #3 - Pretty Pagination
http://drupal.org/node/1871644#comment-6958108

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus

Thank you for your reviews. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no major flaws). Every application that is in the "needs review" state is not abandoned and needs feedback. And you can ignore the "needs work" applications, they have been reviewed already and need some fixes from the maintainer.

manual review:

  1. "'@url' => check_url($google_qr_current_url)));": double esacping is bad. The "@" placeholder will already sanitize the variable for you. Make sure to read the documentation of t() again.
  2. "'path' => t($google_qr_image_url),": why do you translate a URL? REmove the t() call.
  3. "'alt' => t($google_qr_alt),": $google_qr_alt has been translated already, why do you run it through t() again?

Although you should definitively fix those issues I think they are not major application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Issue tags: -PAreview: review bonus

I really appreciate you pointing that all out. I suppose I didn't consider that I was unnecessarily sanitizing some input. Reviewing it all conceptually it now makes a lot of sense.

I'm going to mark "Needs review" again as I made the changes you state, and I'm not sure it would be appropriate to leave the status as is. I apologize profusely if this results in wasting your time :(

I'll review a few more modules to get back into the bonus review, and this time I'll try and tackle some that are further back in the queue.

jnicola’s picture

jnicola CreditAttribution: jnicola commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +PAreview: review bonus

I did three more reviews. I even dug pretty far back for Turn.js. Not sure why that one has set ignored for so long. It's mostly there actually, and unlike some other stuff I've tested, it actually worked when enabled!

#1 - JS Spellcheck
http://drupal.org/node/1860736#comment-6960602

#2 - FileField Mail
http://drupal.org/node/1885736#comment-6960752

#3 Turn.js
http://drupal.org/node/1802112#comment-6960786

sprocketman’s picture

sprocketman CreditAttribution: sprocketman commented
Issue tags: +PAreview: review bonus

Regarding commenting, refer to Doxygen and comment formatting conventions.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Fixed

no objections for more than a week, so ...

Thanks for your contribution, jnicola!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

Adding review notes