Description :
Here is a short description of what this module does. It is specifically for the users who want to implement the API services provided by the www.razuna.com. Razuna is a hosted Digital Asset Management system, is the most popular web app for collaborating, managing, sharing and publishing your organizations images, videos, audio files and documents online. More information can be found on www.razuna.com . This module just provides basic functionality for now. In future, it'll include more functionalities provided by Razuna.

Project Link :
https://drupal.org/sandbox/mandar.harkare/1943222

Git Link (clone) :
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mandar.harkare/1943222.git razuna
cd razuna

Reviewed Modules :
https://drupal.org/node/2179021#comment-8842173
https://drupal.org/node/2223359#comment-8842655
https://www.drupal.org/node/2320499#comment-9051985

Files: 
CommentFileSizeAuthor
#56 coder-results.txt2.91 KBklausi
#14 Picture 1.png445.63 KBdman

Comments

PA robot’s picture

Status:Needs review» Needs work

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxmandarharkare1943222git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Haza’s picture

Hi,

Looked through your code and I got a couple of remarks:

- You only have a "master" branch. According to the branch naming guidelines, you should commit your code in a 7.x-1.x branch.
- Do not include a LICENCE.txt file

About your .info file:

- You can remove the first line there (the line that begin with ; $Id:)
- "project" is discouraged, you can remove it (see https://drupal.org/node/542202)
- You say that "

cURL must be enabled before using this module.

", this could be checked using the hook_requirements()

About the razuna.module file

- As said by the bot (see the link to the ventral review), please have a look on the coding standars.
- No not use an "include" to include a class file. Drupal have it's own class autoloader system, you should put that in the .info file (using files[])
- There is a hook_uninstall() in the module file. Even if this will work, we usually put that in a .install file.

That's for a first review on that module.

Thanks.

mandar.harkare’s picture

Assigned:Unassigned» mandar.harkare

Many thanks Haza for your comments. I'll work on the things you said and yes I went through ventral review and started working as per the coding standards.

Thanks,
Mandar

mandar.harkare’s picture

Assigned:mandar.harkare» Unassigned
Status:Needs work» Needs review

Made some changes, please review again.

AndrewsizZ’s picture

Status:Needs review» Needs work

Hi, found small issues:
1) file rezuna.module line - 37
if there are no args we can delete this line.

2) file rezuna.module lines - 10 - 389
incorrect name of functions, also see https://drupal.org/node/299070

3) file rezuna.module lines - 86
function razuna_admin_settings better to transfer into rezuna.admin.inc, and add 'file' => 'rezuna.admin.inc' in hook_menu for this callback.

4) file razuna.class.inc
some comments for methods of class are incorrect
https://drupal.org/coding-standards/docs

mandar.harkare’s picture

Assigned:Unassigned» mandar.harkare

Thanks Andrew.

mandar.harkare’s picture

Assigned:mandar.harkare» Unassigned
Status:Needs work» Needs review

Made the above changes, please review.

FluxSauce’s picture

Status:Needs review» Needs work

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

  • Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
    razuna.admin.inc
    razuna.install
  • ./razuna.module: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalSecure has found some issues with your code (please check the Writing secure core handbook).
    FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.module
    --------------------------------------------------------------------------------
    FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
    --------------------------------------------------------------------------------
    121 | ERROR | Menu callback argument $bundle returned from razuna_create_field
    141 | ERROR | Menu callback argument $bundle returned from razuna_create_field
    --------------------------------------------------------------------------------

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.

FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.admin.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
80 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.class.inc
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AND 12 WARNING(S) AFFECTING 14 LINE(S)
--------------------------------------------------------------------------------
  23 | ERROR   | File doc comments must be followed by a blank line.
  43 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
  44 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
  45 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
  58 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
136 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
158 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
249 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
282 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
401 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
420 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
441 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
456 | WARNING | The use of private methods or properties is strongly
     |         | discouraged, use "protected" instead
502 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
9 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
42 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

FILE: /var/www/drupal-7-pareview/pareview_temp/razuna.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 6 WARNING(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
111 | WARNING | Line exceeds 80 characters; contains 93 characters
113 | WARNING | Line exceeds 80 characters; contains 97 characters
130 | WARNING | Line exceeds 80 characters; contains 93 characters
133 | WARNING | Line exceeds 80 characters; contains 97 characters
173 | WARNING | Line exceeds 80 characters; contains 94 characters
322 | WARNING | Only string literals should be passed to t() where possible
362 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

mandar.harkare’s picture

Assigned:Unassigned» mandar.harkare

I'll be working on the errors, have one query though, is it mandatory to remove all the warnings as well cause in this case I am not sure that I can change private properties to protected or not.

Thanks.

FluxSauce’s picture

In regards to private properties - https://drupal.org/node/608152#visibility

The use of private methods or properties is strongly discouraged. Private properties and methods may not be accessed or overridden by child classes, which limits the ability of other developers to extend a class to suit their needs.

What are the technical limitations that are preventing you from using protected?

mandar.harkare’s picture

Assigned:mandar.harkare» Unassigned
Status:Needs work» Needs review

There were no technical limitations as such, it was a third party class so I had to ask them before I can modify it, fixed now. Please take a look at it now & let me know your review comments. And thanks a lot for giving time for this.

dman’s picture

I'll look at this now in the Project applications sprint

dman’s picture

Good project page. It's clear about what this does, and does state that you need an account to use the remote service.

Setup and even troubleshooting instructions on the project page are good. Feel free to add a screenshot or logo for the service.

I enabled the module with drush, and got the message

Module razuna doesn't meet the requirements to be enabled. [error]
Please enable your cURL cURL library is not available. (Currently using cURL Enabled) [error]

Even though my phpinfo says

curl
cURL support enabled
cURL Information 7.16.4

...
You should have used

<?php
  
'severity' => $has_curl ? REQUIREMENT_OK : REQUIREMENT_ERROR,
?>

As any state is currently registering as an error.

After fixing that, and enabling the module, I expected to find some settings either in configurations for 'web services' or maybe 'media'... but you had documented and also provided a nice 'Configure' link, so that's cool.

But on that page, my browser was full of scary stuff. Your admin page is being returned to me raw as un-evaluated PHP code! What have you done?
.. I need to figure this out...

dman’s picture

Status:Needs review» Needs work
StatusFileSize
new445.63 KB

Something is wacky, I can't evaluate it as it's currently acting broken for me. Can you re-test your install process on a clean site please?

Also:

PAReview: 3rd party code
razuna.class.inc appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Sorry, I really wanted this one to work!

mandar.harkare’s picture

Assigned:Unassigned» mandar.harkare

Thanks a lot Dan for looking into this.
About curl, I'll surely change the line as you mentioned above.
And yes indeed, the screenshot has a scary stuff. I never got into this situation before, I will surely re-test it.
About third party code, I will ask them about the modifications in the class file (hopefully work around for copyrights), cause once they told me that I am free to modify it as per my need. I am just preferring this because, I am looking for very few dependencies as possible for this module.
Please let me know your view on this 3rd party stuff.

Thanks again.

dman’s picture

I sympathize with the extra step that it takes to include a third-party library the right way, but over time it makes sense. Things like changes to the Twitter, Facebook, Freebase, jquery, jquery UI, Flikr etc etc APIs over the years show it's a required methodology.
Yeah, not today maybe, but next year. And this process is about being able to support users long-term.

Did you really actually have to modify the library functionally?
Sorry that you did some updates to do it to code standards - you would have been able to avoid that by just calling it as an external library. That was a bit of a loss. But a learning experience yah?

mandar.harkare’s picture

Yes of course , lessons comes along with loss ;) Will look into the Library API module and will change accordingly. At least because of this, now I have a better understanding of Drupal coding standards.

Thanks.

mandar.harkare’s picture

Status:Needs work» Needs review

@Dan : that scary stuff was due to wrong encoding done for the file, fixed it though.
Added dependency of Library API module and removed 3rd party class from this module. And fixed some other issues as well please have a look.

Thanks,

Mandar.

kscheirer’s picture

Title:D7 Razuna Module» [D7] Razuna Module
Assigned:mandar.harkare» Unassigned
Status:Needs review» Needs work
Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
  • In the razuna.module, "Etablish" should be "Establish".
  • Can you remove razuna_init()? The curl check is handled by hook_requirements, and you should add the css where it's needed, not on every page load.
  • In razuna_node_presave() you don't need to make $lan a variable. Also, shouldn't you check to make sure the node is using your field or something? I think right now you're modifying every node save.
  • You have a hook_menu entry with a page callback of razuna_page and razuna_setEvent, but I don't see where that is implemented.
  • Please remove all instances of camelCase, like razuna_setEvent and razuna_getAssets.
  • The access callback on razuna_getAssets is TRUE, meaning anonymous users can use this page, doesn't this present a security risk? The same applies to razuna_create_field().
  • All user-facing text should be wrapped in a t() function, like 'Already Created !!'
  • I don't think the razuna_form_alter trick is going to work. This gets set whenever a user edits a node. You're using that variable again in razuna_file_insert(), but there's no guarantee that nothing has happened in between those 2 events. Or that there's more than 1 user on the site, both of whom can edit nodes at the same time. Please rethink this portion.
  • What does razuna_types() do?

----
Top Shelf Modules - Crafted, Curated, Contributed.

mandar.harkare’s picture

Assigned:Unassigned» mandar.harkare

Thanks Karl for jumping in.
I had used this module for one of my past applications, but was very specific to the application, now I am trying to make it generic. Few of your comments are related to the stuff which remained un-removed :( . I will surely take care of those.
About master branch, all my latest commits are showing for branch 7.x, can you please suggest what is to be done?
About $lan variable, can you please suggest the best way to alter a value in $node object on hook_node_load(),
other than using this language variable?

Thanks

bappa.sarkar’s picture

mandar.harkare’s picture

Modifying the flow to get a rid of one of above mentioned bug, so taking long for next step. Instead of using Drupal's default file field type, trying to create custom field type. Any more suggestions are welcome.

Thanks.

bappa.sarkar’s picture

This seems like an appropriate approach.

Cheers!

PA robot’s picture

Issue summary:View changes
Status:Needs work» Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

mandar.harkare’s picture

Status:Closed (won't fix)» Needs work

Changing the status as I am still working on this. As I mentioned in my previous comment, now changing from the basic flow so taking it so long.

PA robot’s picture

Status:Needs work» Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application (see also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

mandar.harkare’s picture

Status:Closed (won't fix)» Needs work

Still working on it. Kind of stuck. Details can be found on the link below
https://drupal.org/node/2267253

mandar.harkare’s picture

Status:Needs work» Needs review

Created a custom field type for the purpose. Please can someone have a look at it and let me know if am I on right track or not.

Thanks.

bappa.sarkar’s picture

Fix http://pareview.sh/pareview/httpgitdrupalorgsandboxmandarharkare1943222git

Also Git URL is wrong. goto your sandbox url and click Version control tab. you will see git url http://git.drupal.org/sandbox/mandar.harkare/1943222.git

bappa.sarkar’s picture

Status:Needs review» Needs work
klausi’s picture

Status:Needs work» Needs review

Minor coding standard errors are not application blockers, please do a real manual review.

mandar.harkare’s picture

Issue summary:View changes

Thanks Bappa and Klausi.
I've updated the Git URL and cleared the code sniffer issue.

mandar.harkare’s picture

Issue summary:View changes
mandar.harkare’s picture

Issue summary:View changes
nuez’s picture

Status:Needs review» Needs work

Hi,

Thanks for your contribution,

This is my review:

functionality

  • I created an account on Razuna and tried to connect to the API through Drupal. On the Razuna platform its not extremely easy to find the API key, email ID, folder ID, and host. I finally got it working but i think it could be useful to add some information to the settings fields about where to find these numbers.
  • The email ID field should be compulsory. Without it it doesn't throw any error, but it doesnt work either. after putting in a dpm at razuna.field.inc:192 with dpm($response), I realised something had gone wrong:

    <h2 style="color:red;">We are so sorry. Something went wrong. We have been notified of this error and will fix it asap.</h2>

  • The configuration of the razuna field is an essential part of the instructions, so I would add that to the instructions.
  • I deleted the node with a razuna image, but it wasnt deleted on the razuna server. Not sure how it should work.

Code

  • A lot of people will activate the module with drush. Drush won't display any html in the warning thrown by the hook_requirements, so you can't see where you can download the library. This is working properly in the UI. See razuna.install:32 and razuna.install:21
  • There are default values missing in the field instance settings, causing errors upon adding a razuna field:
    Notice: Undefined index: file_directory in file_field_instance_settings_form() (line 80 of /Applications/MAMP/htdocs/razuna/modules/file/file.field.inc).
    Notice: Undefined index: file_extensions in file_field_instance_settings_form() (line 87 of /Applications/MAMP/htdocs/razuna/modules/file/file.field.inc).
    Notice: Undefined index: max_filesize in file_field_instance_settings_form() (line 103 of /Applications/MAMP/htdocs/razuna/modules/file/file.field.inc).
  • Depreciated function: Deprecated function: curl_setopt(): The usage of the @filename API for file uploading is deprecated. Please use the CURLFile class instead in curl_setopt() (line185 of /Applications/MAMP/htdocs/razuna/sites/all/modules/razuna/razuna.field.inc).
  • The module only checks for the $api_key to be empty or not before throwing an error. But there are more scenarios for the settings to be invalid, such as wrong api key, wrong directory ID, wrong server, wrong email ID. Would be nice if the user gets to see those errors or at least use the watchdog. See razuna.field.inc:163
      $api_key = variable_get('razuna_api_key', '');
      if (!empty($api_key)) {
        ...
      }
      else {
        drupal_set_message(t('Razuna is not configured properly! Please go to Configuration->People->Configure Razuna.'), 'error', FALSE);
      }

I have some doubts if it's necessary to create a new field type: Why not use the existing field type of file and image to communicate with de Razuna API. Would be interested in knowing if there is a use case scenario where this module comes in handy.

Not sure of any of these issues are showstoppers

mandar.harkare’s picture

Status:Needs work» Needs review

Thanks nuez for the review. Below are my comments on your review

Functionality

1. Mentioned the steps to get API key and Folder Id in Configuration.
2. Made the email id compulsory.
3. Added the Configuration details.
4. This functionality is there on purpose so even if you delete Image Node, it will be there on Razuna server and if you want to delete it, you can go on to Razuna and can delete from there. (Not sure if this is the right approach. Can someone please suggest on this.)

Code

1. Added link to download the razuna library to the intallation guide.
2. Added the default values to field instance settings. Thanks for the catch.
3. The Deprecated Function error was beacuse of the PHP version. PHP 5.5 has introduced CURLFILE which was not in my code. Modified it.
4. The condition "if (!empty($api_key))" is added only to check if these fields are not empty. And assuming that all the required fields will be entered at the time of Admin Form submit, I thought it would be fair to check just one field. And if the values are entered wrong, an error will be thrown saying "Razuna API Calling Failed."

Why new field type?
In the previous flow, File field was used and from Razuna Configuration Form it was asked to select the content types amongst all content types for which the files will be sent to Razuna server. In that case every file field for those selected Content types will be sent to Razuna, there might be a need that particular file fields for the same content types should not be sent to Razuna.
And besides from future prospective, for adding the extra functionality provided by Razuna API, custom field would be a better option.

@nuez, I hope this answers your question.

Mandar.

mandar.harkare’s picture

Issue summary:View changes
Issue tags:+PAReview: review bonus
mpdonadio’s picture

Can you remove your .idea from the repo, and add it to your .gitconfig so it doesn't get back in?

mpdonadio’s picture

Status:Needs review» Needs work

Doing review on

~/par/razuna$ git branch -v
* 7.x-1.x c55d20e Drupal Standards.

Automated Review

Online PAReview came up clear, DrupalSecure threw an exception in the CLI version.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements
3rd party code
Yes: Follows the guidelines for 3rd party code.
README.txt
Yes: Follows the guidelines for in-project documentation and the README.txt Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
No. See security issues identified below.
Coding style & Drupal API usage

Remove the .idea directory from the repo and make sure it is in your .gitignore

The cURL warning doesn't have to be in the .info description.

drupal_http_request() is preferred over cURL.

You don't need to function_exists('libraries_get_path'); you already have libraries as a dependency.

The hook_requirements() logic did not work when I enabled Libraries and Razuna at the same time.

Your hook_requirements() should also try to load the library and then do a class_exists('Razuna') to make sure everything is OK.

The README should have a sample of the exact path to the library file; the zip from GutHub will extract in a subdirectory and people may forget to rename.

Long term, consider a drush command to download and/or clone the library file from GutHub.

You have a system_settings_form(), but you don't delete the variables in your hook_uninstall(). You need to do the cleanup.

What is your hook_uninstall doing? Where did field_assetid come from?

There is a hook_registry_files_alter() trick to get the class into the autoloader so you don't need to explictly do a libraries_load().

I can't remember if drupal_get_path() is safe to call at the top level in all instances. You can use the PHP magic constant for this.

Why is the config under People? Maybe under Media would be better?

The non-admin callbacks for the hook_menu() need better paths.

The $may_cache argument for hook_menu is gone in Drupal 7.

Normally, you would specify your menu wildcards in the $item key itself.

(*) How are the two non-admin hook_menu() items used? You don't normally need to explictitly specify user_access for the access callback (it's the default),
and you don't have any access arguments, which means only uid 1 would be able to access these paths. $items['create-field'] seems to have a callback
that doesn't exist? And $items['ajax_img'] just outputs a URL?

You do not need the hook_init(). This hook gets invoked for every page request. If you really need to use cURL, you need to determine a better way to handle it.

I don't follow the logic in razuna_file_url_alter(). Will this only work with the standard image styles? Will it work over HTTPS connections?

Use drupal_json_decode() instead of json_decode().

(*) razuna_get_assets() needs a better error message on line 133. This is third-party input, so it needs to be sanitized. See https://www.drupal.org/node/28984

It looks like a lot of the code from razima.field.inc mirrors the file_field functions. This should be in the comments.

You have a fair number of long lines with arrays. See https://www.drupal.org/coding-standards#array

So the Razuna class doesn't actually handle the uploads?

(*) The Help Text in the field is XSS vulnerable. Add a Razuna field to a content type, set the Help Text to alert('XSS'), save.
The make a new node. You should see an alert. The same does not happen with File or Image. I cannot trace out exactly why this is happening in this case.

The starred (*) items are blocking issues. I also find the direct use of the file functions to be a bit troubling, and would be concerned about
how this will work out in the long run.

Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

mpdonadio’s picture

Issue tags:+PAReview: security
mandar.harkare’s picture

Status:Needs work» Needs review

Thanks Matthew for the review.

I mainly focused on clearing out the blockers first.

Those menu items were created for testing purpose and were left unmoved. I removed those as they are not used.

Added check on third-party input in razuna_get_assets.

And about "field is XSS vulnerable", as suggested, I set the help text to alert('XSS'). But unfortunately I did not get any alert at the time of adding new node. Please send me some more details to replicate the issue.

I cleared few other issues as well and I'll be working on others (hoping that this will not block the review process). Meanwhile it would be great if someone could progress with the review to the next level.

Thanks
Mandar

mpdonadio’s picture

Status:Needs review» Needs work

@mandar.harkare, I forgot to escape that. I sent it to

<script>alert('XSS')</script>

Setting back to Needs Work, as that was the primary blocker.

mandar.harkare’s picture

Oh yes Matthew, now I could seen the alert at the time of adding new node.

But my observation is that this same thing happens with image field (I modified the help text of image field in Article and could see the alert there as well). But file field does not alert this, so not sure if this is the issue or not. Confused here, please suggest.

Mandar.

mpdonadio’s picture

I can't make it happen with an Image field; I tried on a few different sites.

I don't quite understand why it is happening with your field. I suspect it has to do with how you are making the field forms. The sanitization should be built in on the help text (which is technically the description on the field), so I think it has to do with how you are building the field form. This may give you some more information: http://drupal.stackexchange.com/questions/123146

mandar.harkare’s picture

Status:Needs work» Needs review

Thanks for the link, it helped in resolving my issue. Added the filter on help text. Can you please proceed with the review to the next level?

Thanks,
Mandar

heddn’s picture

Assigned:mandar.harkare» Unassigned

@mpdonadio This is next on my list of projects to review.

klausi’s picture

Issue summary:View changes
Issue tags:-PAReview: review bonus

Removing review bonus tag, you have not done all manual reviews, you just repeated the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

heddn’s picture

Issue summary:View changes
heddn’s picture

Status:Needs review» Needs work

Automated Review

n/a

Manual Review

Secure code
Yes/No. If "no", list security issues identified.
Coding style & Drupal API usage
  1. (*) remove hook_init(). This has been mentioned in several of the above reviews.
  2. (+) Do you requirements check using a phase of runtime so folks and at least enable the module without the library. A lot folks just want to kick the tires of a module without having to install the library.
    if ($phase == 'runtime') {}
  3. (*) Remove the .gitignore. If you need to ignore some default files/folder, put these ignores in a system/global .gitignore in ~/.gitignore
  4. No need to re-assign to $form, simply return system_settings_form from the function.
    function razuna_admin_settings($form_state) {
    ...
      $form = system_settings_form($form);
      return $form;
  5. (*) What version of Razuna is required/tested with this module? Check for that in the libraries hook. The README and other documentation wasn't clear what to name the library path. I wasn't able to get the module installed because I couldn't get the module to detect the library. This is probably likely due to the fact that the latest tag I was using (1.2-API-1) didn't include a version string in it.
  6. I know that curl is fairly popular, but drupal_http_request comes with Drupal. Why doesn't this module use it instead?
  7. You can't assume that everyone is using LANGUANGE_NONE. Use $langcode instead. Also seen in razuna_field_submit().
    (*) function razuna_field_widget_form(&$form, &$form_state, $field, $instance, $langcode, $items, $delta, $element) {
    ...

      foreach ($form_state['field'] as $field_name => $field_object) {
        if ($field_object[LANGUAGE_NONE]['field']['type'] == 'razuna') {
          $razuna_fields[$field_name] = $field_object;
        }
      }

  8. (+) No need to return as markup, rather directly use '#theme' => 'image_style'. Also seen in razuna_field_formatter_view()
    function razuna_field_widget_process($element, &$form_state, $form) {
    ...

        $element['preview'] = array(
          '#type' => 'markup',
          '#markup' => theme('image_style', $variables),
        );

  9. Why not provide a link? Make it easier for users.drupal_set_message(t('Razuna is not configured properly! Please go to Configuration->Media->Configure Razuna.'), 'error', FALSE);
  10. This is a bit of a nit, but entities don't have node ids, rather entity ids. And fields can be added to more things than just nodes. So maybe call the variable $entity_id.
    function razuna_field_prepare_view($entity_type, $entities, $field, $instances, $langcode, &$items) {
      foreach ($entities as $nid => $entity) {
        foreach ($items[$nid] as $id => $item) {
          if (!empty($items[$nid][$id]['asset_id'])) {
            $uri = razuna_get_assets($items[$nid][$id]['asset_id'], 'thumbnail', 'img');
            $items[$nid][$id]['uri'] = $uri;
          }
        }
      }
    }

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

mandar.harkare’s picture

Status:Needs work» Needs review

Thanks Lucas for the in depth review.
All the above mentioned issued are resolved except using drupal_http_request. Still curl is being used. Could not find a way to POST files using drupal_http_request (lack of drupal knowledge). This was not mentioned as blocker so changing the status to Needs Review.

Meanwhile if someone could help me out with the some example on drupal_http_request or any reference to how to post files using drupal_http_request, would be a great help.

I'll do some manual reviews as well to add the PAReview: review bonus tag.

Please review this module and suggest if I am missing out some more drupal standards.

Thanks
Mandar

heddn’s picture

A quick google search found me: https://www.drupal.org/node/270997

mandar.harkare’s picture

Thanks Lucas,
I found that too but unfortunately did not work for me. I tried with few modifications as well but no luck. I think I am missing something. I'll keep digging and trying.

mandar.harkare’s picture

Issue summary:View changes
mandar.harkare’s picture

Issue tags:-PAReview: security+PAReview: security PAReview: review bonus
mandar.harkare’s picture

Issue tags:-PAReview: security PAReview: review bonus+PAReview: review bonus, +PAReview: security
klausi’s picture

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus
StatusFileSize
new2.91 KB

Review of the 7.x-1.x branch (commit 4b4d53a):

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • DrupalPractice has found some issues with your code, but could be false positives.
    FILE: /home/klausi/pareview_temp/razuna.field.inc
    --------------------------------------------------------------------------------
    FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
    --------------------------------------------------------------------------------
    131 | WARNING | Unused variable $variables.
    --------------------------------------------------------------------------------
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. razuna_uninstall(): why is this needed? What is field_assetid? It is never used anywhere else? Please add a comment.
  2. razuna_connect(): Where do $library['error'] and $library['error message'] come from and why are they not sanitized before printing with drupal_set_message()? Do they directly stem form the third party razuna service? Please add a comment.
  3. razuna_file_url_alter(): looks like the "large", "medium" etc. image styles are hard coded here, why? Why do you need to str_replace() those here? Please add a comment. Why is this function needed at all? You are rewriting any public file URL regardless of which field it is used in?
  4. razuna_get_assets(): why do you call "echo" here and directly end execution with exit? Please change that or add a comment why you need to do that.
  5. razuna_field_widget_process(): doc block looks copy & pasted?
  6. razuna_admin_settings(): all variables defined by your module need to be deleted in hook_uninstall().
  7. razuna_field_submit(): why do you have to rewrite the razuna_hostname like that? You could just validate the URL when the users specify them in the admin form?

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

mandar.harkare’s picture

Status:Needs work» Needs review

Thanks klausi for the review and sorry for the delayed response & fix.

Fixed the above issues.

1 . asset_id was related to old flow which, now is removed.
2 . Added the comment.
3 . This too was related to old flow and is now removed.
4 . This was kept for returning image URL only, if needed. Thought would be helpful from future prospective.
But can be added later if required, now removed.
5 . Modified the doc block.
6 . Variables defined by this module are deleted at the time of uninstall.

Now will do the manual reviews to add review bonus tag.

sendinblue’s picture

Status:Needs review» Needs work

Automated Review

No issues.

Manual Review

Individual user account
Yes.
No duplication
No.
Master Branch
Yes
Licensing
No
3rd party code
No.
README.txt/README.md
No. Your readme.txt don't follow template. Follows the guidelines for in-project documentation and the README Template
Code long/complex enough for review
Yes.
Secure code
Yes.
Coding style & Drupal API usage
klausi’s picture

Status:Needs work» Needs review

Readme formatting is surely not an application blocker, please do a real manual review.

klausi’s picture

Assigned:Unassigned» mpdonadio
Status:Needs review» Reviewed & tested by the community

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

Review of the 7.x-1.x branch (commit 38bb71c):

  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    FILE: /home/klausi/pareview_temp/razuna.install
    --------------------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------------
    21 | ERROR | [x] Expected one space after the comma, 0 found
    --------------------------------------------------------------------------------
    PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
    --------------------------------------------------------------------------------
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. README.txt is missing instructions about installing the library?
  2. razuna_connect(): why do you str_replace() http:// with the empty string here? Please add a comment.
  3. razuna_connect(): so the XSS security point here is that everything that comes from a third party source has to be treated as untrusted user provided input. And that needs to be sanitized with check_plain() or similar before printing to drupal_set_message(). Make sure to read https://www.drupal.org/node/28984 again. So please fix the comment as it does not make sense right now. Here it looks like the error message is coming form the libraries module (no user provided text), that's why I asked.
  4. razuna_get_assets(): why the drupal_set_message() with the empty $json_asset here? Shouldn't you use watchdog() to just log any errors?

But that are not critical application blockers, otherwise looks RTBC to me.

Assigning to mpdonadio as he might have time to take a final look at this.

mpdonadio’s picture

No commits since @klausi's review.

You have a few instances where you use double-quoted strings. Single quotes ones are preferred. No bigee, but it is nice to be consistent with this.

razuna_field_submit() removes index.cfm from the URL, and then adds it back? Comment needed.

I would like to see a comment on why cURL is needed, and drupal_http_request can't be used.

razuna_connect(), razuna_get_assets() need a proper docblock defineing the parameters.

Concur with @klausi, no blockers. But, the library installation is documented in the README, INSTALLATION step 8

mpdonadio’s picture

Assigned:mpdonadio» Unassigned
Status:Reviewed & tested by the community» Fixed

Thanks for your contribution, mandar.harkare!

I updated your account so you can 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 stay 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.

Status:Fixed» Closed (fixed)

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