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
| Comment | File | Size | Author |
|---|---|---|---|
| #56 | coder-results.txt | 2.91 KB | klausi |
| #14 | Picture 1.png | 445.63 KB | dman |
Comments
Comment #1
PA robot commentedThere 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.
Comment #2
hazaHi,
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 "
", 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.
Comment #3
mandar.harkare commentedMany 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
Comment #4
mandar.harkare commentedMade some changes, please review again.
Comment #5
andrewsizz commentedHi, 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
Comment #6
mandar.harkare commentedThanks Andrew.
Comment #7
mandar.harkare commentedMade the above changes, please review.
Comment #8
fluxsauce commentedIt 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:
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.
Source: http://ventral.org/pareview - PAReview.sh online service
Comment #9
mandar.harkare commentedI'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.
Comment #10
fluxsauce commentedIn regards to private properties - https://drupal.org/node/608152#visibility
What are the technical limitations that are preventing you from using protected?
Comment #11
mandar.harkare commentedThere 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.
Comment #12
dman commentedI'll look at this now in the Project applications sprint
Comment #13
dman commentedGood 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
Even though my phpinfo says
...
You should have used
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...
Comment #14
dman commentedSomething 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:
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!
Comment #15
mandar.harkare commentedThanks 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.
Comment #16
dman commentedI 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?
Comment #17
mandar.harkare commentedYes 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.
Comment #18
mandar.harkare commented@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.
Comment #19
kscheirerrazuna_pageandrazuna_setEvent, but I don't see where that is implemented.razuna_form_altertrick 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.----
Top Shelf Modules - Crafted, Curated, Contributed.
Comment #20
mandar.harkare commentedThanks 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
Comment #21
bappa.sarkar commentedSome sniffer issues found. http://pareview.sh/pareview/httpgitdrupalorgsandboxmandarharkare1943222git
Comment #22
mandar.harkare commentedModifying 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.
Comment #23
bappa.sarkar commentedThis seems like an appropriate approach.
Cheers!
Comment #24
PA robot commentedClosing 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.
Comment #25
mandar.harkare commentedChanging 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.
Comment #26
PA robot commentedClosing 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.
Comment #27
mandar.harkare commentedStill working on it. Kind of stuck. Details can be found on the link below
https://drupal.org/node/2267253
Comment #28
mandar.harkare commentedCreated 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.
Comment #29
bappa.sarkar commentedFix 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
Comment #30
bappa.sarkar commentedComment #31
klausiMinor coding standard errors are not application blockers, please do a real manual review.
Comment #32
mandar.harkare commentedThanks Bappa and Klausi.
I've updated the Git URL and cleared the code sniffer issue.
Comment #33
mandar.harkare commentedComment #34
mandar.harkare commentedComment #35
nuezHi,
Thanks for your contribution,
This is my review:
functionality
<h2 style="color:red;">We are so sorry. Something went wrong. We have been notified of this error and will fix it asap.</h2>Code
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).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
Comment #36
mandar.harkare commentedThanks 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.
Comment #37
mandar.harkare commentedComment #38
mpdonadioCan you remove your .idea from the repo, and add it to your .gitconfig so it doesn't get back in?
Comment #39
mpdonadioDoing review on
Automated Review
Online PAReview came up clear, DrupalSecure threw an exception in the CLI version.
Manual Review
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.
Comment #40
mpdonadioComment #41
mandar.harkare commentedThanks 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
Comment #42
mpdonadio@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.
Comment #43
mandar.harkare commentedOh 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.
Comment #44
mpdonadioI 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
Comment #45
mandar.harkare commentedThanks 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
Comment #46
heddn@mpdonadio This is next on my list of projects to review.
Comment #47
klausiRemoving 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.
Comment #48
heddnComment #49
heddnAutomated Review
n/a
Manual Review
if ($phase == 'runtime') {}drupal_set_message(t('Razuna is not configured properly! Please go to Configuration->Media->Configure Razuna.'), 'error', FALSE);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.
Comment #50
mandar.harkare commentedThanks 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
Comment #51
heddnA quick google search found me: https://www.drupal.org/node/270997
Comment #52
mandar.harkare commentedThanks 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.
Comment #53
mandar.harkare commentedComment #54
mandar.harkare commentedComment #55
mandar.harkare commentedComment #56
klausiReview of the 7.x-1.x branch (commit 4b4d53a):
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:
Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #57
mandar.harkare commentedThanks 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.
Comment #58
sendinblue commentedAutomated Review
No issues.
Manual Review
Comment #59
klausiReadme formatting is surely not an application blocker, please do a real manual review.
Comment #60
klausiSorry 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):
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:
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.
Comment #61
mpdonadioNo 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
Comment #62
mpdonadioThanks 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.