Thanks in advance for your review!!

DRUPAL 7
I needed a module to upload video to youtube without saving them in drupal files. I accomplished the same using the below modules.

  • Media
  • Media_youtube
  • Media_derivatives
  • Media_derivatives_youtube
  • youtube_derivatives_formatter

Since its too much to add so many modules to achieve a simple functionality of adding videos to youtube.
so I thought of creating a module for youtube integration. This module uploads videos directly to youtube. No server load, because the file is directly moved to youtube and drupal just stores the 11 character youtube story ID or video ID.

There was a module in D6 which says it provides the similar functionality but there is no module currently in D7 which provides a youtube CCK field which can be used with youtube URL or uploading a video directly to youtube. More info available in comments under.

-- About --

This is a D7 module

A module to embed and upload videos to Youtube from Drupal. Provides a Youtube CCK field which can be used with Youtube URL or uploading a video directly to Youtube.

DEPENDENCIES :
1. Libraries
2. ctools
3. Zend Gdata library v1.12.3

Please download the same version of gdata library, some newer versions may not work with the module.

-- INSTALLATION --
1. Adding Zend-Google Gdata library :
Firstly, you will need to download the Zend Gdata library from "http://packages.zendframework.com/releases/ZendGdata-1.12.3/ZendGdata-1...."
Once downloaded you need to extract the folder in sites/all/libraries and rename it as ZendGdata. So the complete path will look like :

"sites/all/libraries/ZendGdata/library/Zend/Gdata.php"

2. Installing and configuring the module
Install as usual, Download the module and paste it in sites/all/modules.
Activate the module in admin/modules.
Navigate to admin/config. In the right side block find "Youtube Engine Settings". Click on the link.
Fill the form. Your google email id and password.

To obtain Youtube API key,
go to https://console.developers.google.com and create a project.
After you create a project you see APIs & auth on left hand side bar.
Click on APIs & auth>Credentials>create new key> Browser key.
Add you site url ( with wilcard if needed ) in allowed referers.
Copy the generated API key and add it in YouTube Video Uploader Settings.
(Remember to upload a video using youtube.com if its a new account and your gmail
is not linked to your youtube account.)

3. Adding the youtube field.
Goto "admin/structure/types" and select manage fields in any content type.
Create a new CCK field. Select Field type as "youtube".
Click on save settings do not change any settings other than required field setting, Multifield is currently not supported.

-- Project Page --
http://drupal.org/sandbox/sunnyuff/1874604

-- Git --
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/sunnyuff/1874604.git youtube_video_uploader
cd youtube_video_uploader

-- Pareview Link --
Pareview Link

-- My Reviews --
Review of other projects : https://drupal.org/node/2251193#comment-8802687
Review of other projects : https://drupal.org/node/2271545#comment-8799003
Review of other projects : https://drupal.org/node/2251805#comment-8741887

Comments

vineet.osscube’s picture

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxsunnyuff1874604git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

vineet.osscube’s picture

Status:Needs review» Needs work

Manual Review:

1) There is no hook_uninstall to remove your custom variables from database like: youtube_engine_email... , youtube_engine_password.. present in your module file.

2) Use variable_del() to remove custom variables & prefix variable name wuth your module name i.e. "youtube_video_uploader_engine _email" etc.

3) Do not use files[] in .info file to add you module & install file. They are for adding class files.

sunnyuff’s picture

Status:Needs work» Needs review
StatusFileSize
new34.75 KB

Hi osscube,

Thank you for spending your valuable time to review my code. Actually I have used coder module to review the code but It seems that http://ventral.org is better than any. I have done everything you suggested, will you please review the code again.

Thank you.

anwar_max’s picture

Status:Needs review» Needs work

Hi sunnyuff,

It seems that Media youtube provides very similar functionality. Could you describe how your module differs?
We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module.
If the existing module is abandoned, please think about taking it over instead of creating a new one.

Automated review:

please get a review bonus first. Then try to fix issues raised by automated review tools and set this back to "needs review".

sunnyuff’s picture

Status:Needs work» Needs review

Hi anwar_max,

Thank you very much for your effort. Here under, you will find the differences and reasons why "Youtube Video Uploader" is more lightweight, provides clean interface etc as compared to other modules currently available.

1. The main difference between any other module currently available (including Media youtube ) and "Youtube Video Uploader" in my opinion is that the focus of this module is to provide a clean interface for uploading videos directly to Youtube and fomatters to display them. It uses the Zend Gdata library to communicate to the Youtube API and implements the "browser upload method" so the video file never hits the Drupal file system. Drupal just stores the unique 11 character Youtube story ID. It saves two most valuable resources in web technologies i,e. "storage and bandwidth". Media module does not provides the functionality to upload videos to Youtube instead it just provides the ability to embed Youtube videos.

3. As I mentioned, in my first comment to replicate the functionality this module provides, I had to use a couple of modules viz.

  1. Media
  2. Media_youtube
  3. Media_derivatives
  4. Media_derivatives_youtube
  5. youtube_derivatives_formatter

After all of the complex configurations combining the five modules and fixing the internal bugs finally I was able to upload videos to youtube but my site became slow. All of my videos are placed in drupal file system, means videos are first uploaded in drupal then in youtube. That is why I thought of creating this module.

DRUPAL COMMUNITY : Please share your thoughts and suggestions, it really helps.

fr3shw3b’s picture

Status:Needs review» Needs work

Hello sunnyuff,

Great module idea, I've started thinking of doing something similar for soundcloud.
Is their not the possibility of integration with Media and the Media integrated youtube modules?
And by the way the git command in the issue isn't the one for non maintainers,
Go to your sandboxes version control select non-maintainer and show and then use the code with straightforward
URL.
Automated Review:

There are only some minor issues here:
http://ventral.org/pareview/httpgitdrupalorgsandboxsunnyuff1874604git

Manual Review:

1. Firstly you need to change master branch to a version specific branch, more about that here:
http://drupal.org/empty-git-master

2. The User Password input should be encrypted and a normal password field.

3. Calling the Youtube Video Uploader Youtube Engine in configuration seems bad practice you should stick to the name of the module for the sake of UX and call it Youtube Video Uploader settings.

4. It is probably worth changing some variable names like $yt to something more self explanatory.

5. Remove the youtube_video_uploader_install() function as it is empty and not being used.

sunnyuff’s picture

Hello fr3shw3b,

Thank you for the review. Your comment really helped. I have implemented almost all of your suggestions, please check.

  1. Version Specific Branch : Done
  2. User Password : Done(*)
  3. Youtube Video Uploader Youtube Engine (name change) : Done
  4. Variables names : Done
  5. Function install : Done

But I have not encrypted the password. I have checked many contributed modules and for third Party services they are just using textfield/password for saving passwords. Some people are using "base64" but since I have to send the password to Youtube APIs, I cannot use MD5 (one-way hash algorithm). Again, it doesn't matter if it's Base64 encoded or triple-DES 168-bit encryption - they are reversible, so it is exactly as secure as not encoding it at all.

So, I request you to please help me with this, if possible.

NOTE : (Integration with media) Not required, becouse media module is a module which provides framework for handling media and files But here we are just integrating Youtube with Drupal without any videos hitting Drupal. As I already mentioned in my comments, this module just stores 11 charcter Youtube Story id, i,e no media is associated with Drupal. So in this case, integration this simple and lightweight module with a framework like media module provides will unnecessarily increase the server load, user configurations etc. for people who just wants youtube as their video hosting provider.

Regards,

Sunny Jhunjhunwala.

sunnyuff’s picture

Status:Needs work» Needs review

Sorry, I forgot to change the status of the issue to "needs review".

fr3shw3b’s picture

Status:Needs review» Needs work

Hello,

I understand about password encryption and that the media module servers a different purpose.

- On line 377 with the $http_client variable you want to format it so the code is inline with the function being called.

- Where you use require_once a few times for library files couldn't you use the libraries load function as oppose to getting the library path and then setting the include path and requiring each script once?

- The story id naming convention could be changed to developer id to be more self explanatory if i'm following the code correctly.

- You could also seperate the module file to break it down and reduce the size by using include files. You would put all functions to do with admin settings and the form functions for the multi-step upload form in one, possibly all theme/page output pages in another and anything else you might find is worth putting in it's own include for the greater clarity and structure. I'd then best advise to put all these includes in an includes folder.

fr3shw3b’s picture

Category:support» task

Changed category to task.
read here:
http://drupal.org/node/1011698

sunnyuff’s picture

Hi fr3shw3b ,

Thank you for the comment. Let me tell you what i did this time.

1. On line 377 -- DONE.
2. require_once -- Firstly, i tried to use libraries load only, but Zend library internally could not load some of the files, then i used libraries_get_path.
3. story id is the id of the video uploaded to youtube. Its a 11 character long unique video id. Many Youtube discussions mention it as story id so, I have also used story id.
4. break it down and reduce the size by using include files : Great suggestion, -- DONE.

Your comments really helped, the module looks in a quite decent structure now.
I have many modifications and features in pipeline, I will keep adding new features as required.

Thank you.

Note : I did not understand your last comment.

sunnyuff’s picture

Status:Needs work» Needs review

Sorry, I forgot to change the status of the issue to "needs review" again.

fr3shw3b’s picture

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

http://drupal.org/project/video_upload

I didn't think to look before, taking your word for it but this already exists for Drupal 6.
This particular module's status is seeking a new maintainer so it is best if you overtake that module if it is possible and integrate your needs for Drupal 7 into that. It would be bad to have to modules with the same name even if they were for different version of Drupal as they do the same thing and it's best if it's all integrated into one thing as you may know the as a community we are into collaboration over competition.

The maintainer specifically states

If you are interested in maintaining this module as an alternative to the Media module in Drupal 7, please contact me.

Sorry about not picking up on this earlier.

PA robot’s picture

Status:Postponed (maintainer needs more info)» Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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

PA robot’s picture

Issue summary:View changes

changed branch specific 7.x

sunnyuff’s picture

Category:Task» Support request
Issue summary:View changes
Status:Closed (won't fix)» Needs review

Hi All,

Last time I created this module because I needed something similar for a client project, and after a year I am here again.

I think we should make this module available for the community, today I searched for all available options for youtube video field and I did not find anything. Also, the functionality of both adding a youtube url or a video upload directly to youtube is not seen in any of those.

If we see the time, its almost a year I waited for, if nothing is coming from the old module creators, I think its a justifiable reason to make this one available for the community.

I reopened this only because I got comments from people asking about its release date ? I do not have any answers, I like the community to answer on it.

Regards,

Sunny J.

klausi’s picture

Category:Support request» Task
Status:Needs review» Postponed (maintainer needs more info)

Project applications are tasks.

Could you answer the question from comment #13 first?

Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the video_upload 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.

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

sunnyuff’s picture

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

Hi klausi,

Thanks for the quick reply. I respect the community and I myself do not like module duplication. I have contracted the maintainers of video_upload before, they told me that they are trying to port the drupal 6 version of their module to drupal 7, and they need coders who can do it. But they do not want my module to be used as a D7 version of video_upload. And if you see there is no activity from their side within last couple of years.

Last time only I tried to combine my code with their but their version is too complicated and needs a lot of work to be done, I do not mind to place this module as a D7 version of video_upload if the maintainers are okie with it, but I seriously cannot start coding from begining to make their module compatible with D7.

Regards,

Sunny J.

sunnyuff’s picture

Hi Guys,
I wish, if i can get a release on Christmas or new year, please review and suggest me the next step.

Regards,

xqus’s picture

243   $username = t(variable_get('youtube_video_uploader_engine_email', ''));
244   $password = t(variable_get('youtube_video_uploader_engine_password', ''));
245   $developer_key = t(variable_get('youtube_video_uploader_engine_dev_key', ''));

Why are you passing this trough t() ?

262     youtube_get_video_status($story_id);

Should this be a call to youtube_video_uploader_get_video_status(), if so will this keep calling itself until it succeeds? If so there should be some kind of failsafe preventing it from and indefinite loop.

  10 module_load_include('inc', 'youtube_video_uploader', 'youtube_video_uploader.admin');

Instead of loading this file every time. Consider loading it with a file property in the hook_menu()
See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...

access administration vs. administer site configuration
The administration menu callback should probably use "administer site configuration" - which implies the user can change something - rather than "access administration pages" which is about viewing but not changing configurations.
Alexxikon’s picture

Hi sunnyuff,

Some suggestions for your module:

There are many strings all over the code that should be translatable via the t() function.

There is likely typo in 'Videblog' (youtube_video_uploader.module line 188). It should probably read 'Videoblog'.

The word 'youtube', when appearing in human-readable strings, should probably be capitalized as in 'YouTube'.

It seems that the 'Implements hook_...' comment was deleted in some functions. This would make it more difficult for maintainers to understand your code.

sunnyuff’s picture

Hi xqus and Alexxikon,

Thank you for testing and providing your feedback.

@xqus :

1. unnecessary t(), by mistake, fixed.
2. youtube_video_uploader_get_video_status - that is not in use currently, removed the unused code, fixed.
3. Instead of loading this file every time. Consider loading it with a file property in the hook_menu() -- great suggestion, fixed.
4. access administration vs. administer site configuration -- fixed.

@Alexxikon :

1. Added t() where necessary.
2. typo in 'Videblog', fixed all youtube categories.
3. The word 'youtube', fixed with YouTube.
4. Code cleanup and added code signatures.

sorry guys got lil busy due to office work, so was not able to update as planned.

zhuber’s picture

Status:Needs review» Needs work

I'm still seeing a lot of things that could be cleaned up from PAReview:
http://pareview.sh/pareview/httpgitdrupalorgsandboxsunnyuff1874604git

It looks like you are still using t() functions in hook menu and have some unused variables declared.

Also, it looks like you might still have the master branch. You probably want to remove this, so PAReview (as mentioned in comment #6 https://drupal.org/comment/6889170#comment-6889170).

sunnyuff’s picture

Status:Needs work» Needs review

Hello zhuber,

PAReview, code clean-up done. Now the default branch is 7.x-1.x

Thanks for the review.

sunnyuff’s picture

Hello Team,

Please let me know the next steps,

Br,
Sunny.

PrineShazar’s picture

Hi sunnyyuff, I had issues with Chrome Version 32.0.1700.102 (latest) accepting Youtube response to "youtube_uploader/nexturl" menu link after the video was uploaded. I was getting "No 'Access-Control-Allow-Origin'" headers error

I understand in yu.js line 87 you wrap the json response in submitToYoutube method with a success callback, though im not sure why this was throwing an error as it seems like it should work.

I got around this by downloading and enabling CORS module and updated the config to accept YouTube request to "youtube_uploader/nexturl".

sunnyuff’s picture

Hi PrineShazar,

I am so sorry that you faced so much trouble, I have checked the module with a few available options with me, details below.

MAC OS X (10.9.1)

Chrome Version 32.0.1700.107
Safari Version 7.0.1 (9537.73.11)
Firefox v 27.0

I did not faced any problem in my initial check, i am checking this in detail. In the meantime, if you have any more info on that please fell free to post.
Btw, very clever decision of using CORS module.

sunnyuff’s picture

Hi PrineShazar,

I have tested the module in many environment including windows 7, Mac OS and Ubuntu. I am not able to reproduce the issue. If you are still facing the issue please let me know.

gobinathm’s picture

Status:Needs review» Needs work

PAReview is still reporting issues in this module. Please have them fixed before manual reviews.

Note: these are not application Blockers

gobinathm’s picture

Status:Needs work» Needs review
pritamprasun’s picture

StatusFileSize
new114.92 KB

Hello Sunny,
the module seems to be solving my problem but I got the following error in 1st step
error
I have followed the configuration steps properly.

pritamprasun’s picture

Sunny,
Thanks a lot for your assistance.
You were right, the problem was with the credential. Now it is working perfectly fine for me.

This Module is really lightweight and solved my purpose. I am using it in the next version of http://techtud.com.
I highly recommend this module to someone who wants to upload videos directly to youtube transparently.

sunnyuff’s picture

Priority:Normal» Major

Hi Guys,

Its now a long time I am waiting for publish of this module, people are contacting me on fb and mail for support, please respond about the next step.

Regards,

Sunny.

nsuit’s picture

Priority:Major» Normal

Hi sunnyuff,

I feel your pain. Have been in the applications queue for over a year now myself. If you need some comfort, I found those comments about the review process: https://groups.drupal.org/node/288818. Seems we are not the only ones stuck.

It's a small thing, but could you correct the git command at the top to reflect the new branch 7.x-1.x? It just speeds things up for reviewers.

It should be
git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/sunnyuff/1874604.git youtube_video_uploader
not
git clone --recursive --branch 7.x http://git.drupal.org/sandbox/sunnyuff/1874604.git youtube_video_uploader

Once I installed the Zend folder in sites/all/libraries and enabled the module everything worked like it described.

You still got three pareview errors that should be easy to fix.

Good luck with your project.

nsuit’s picture

Status:Needs review» Reviewed & tested by the community

I forgot to change the status when posting my last comment.

sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Hi Nsuit,

Thanks for the update, I forgot to update the copy when I updated to new branch.

All pareview errors are only for indentation, the Readme one is for more than 80 character, but that was a url and i was adding it inside quotes, so i removed the quotes and its fixed.

But whatever i do for the other its ask me the vise-versa. Please check two screen-shots i attached.

1. Screen Shot 2014-04-13 at 12.34.18.png
2. Screen Shot 2014-04-13 at 12.33.53.png

Thanks,

end user’s picture

Man I've been looking for a YT upload module for ages, thanks for the effort in creating it there was no way I could afford to have this created. This will come in handy for a few new projects I got planned.

sunnyuff’s picture

StatusFileSize
new87.58 KB

Hi Arek,

Thanks for appreciating my work. I liked your username "end user" !!!.

Kathy Chavez (drupaler) asked me how this module is different from youtube upload widget, so I have attached a image mentioning the main points here,

YoutubeModuleVSyoutubeWidget

If you need any help regarding the module or want any feature addition let me know.

Best,

Sunny.

sunnyuff’s picture

EducoWebDesign’s picture

Hi Sunny,
Awesome module, thanks so much for your hard work and perseverance! I had a bit of an issue with the instructions in light of the recent Google API changes that I thought you should know about.

According to https://developers.google.com/youtube/2.0/developers_guide_protocol?csw=..., the YouTube API v2.0 was deprecated at the beginning of this year and any new users who go to http://code.google.com/apis/youtube/dashboard/ and try to create a product and get a developer key will get the following response: "Network or server error: Could not complete the registration of the new product. Please try again later." No thanks to Google's completely unhelpful error code and documentation, I eventually made my way to the new developer console at https://console.developers.google.com.

I created a new project at https://console.developers.google.com and created a new browser key under APIs & auth > Credentials.

Also, the current version of the Zend Gdata library is 1.12.6 would you recommend that we stick with 1.12.1?

When I was trying version 1.12.6, I got the following error: Warning: require_once(Zend/Xml/Security.php): failed to open stream: No such file or directory in require_once() (line 30 of .../libraries/Zend_Gdata/Zend/Gdata/App/Base.php). and had to download the full Zend Framework and upload the Xml folder to get past step 1, so be advised of that particular roadblock if you try to use the newest library. I switched back to 1.12.1 because I was having an https issue that I thought was related to the library version. I didn't realize at the time that I was getting hung up on step 2 because Firefox was blocking step 3 for security reasons. That might be something to look into for those users using https instead of http.

Thanks again!

klausi’s picture

Removing review bonus tag, you only did one review and not three? Also make sure to add your reviews to the issue summary.

sunnyuff’s picture

sunnyuff’s picture

sunnyuff’s picture

Hi EducoWebDesign,

Thanks so much, I am currently adding some more features and doing testing with APIV3.0, but currently its working properly and I recommend to use ZEND GDATA v1.12.1 untill I do the compatibility test with new library.

Once I update the module and test it properly with APIv3.0, I will update the docs.

Previously, I have tested the module with all known browser versions and will do it again when I release the new update. Just checked the module in firefox, not able to reproduce the issue, are you finding this only with https ?? can you please give little more info how to reproduce the issue.

Best,

Sunny.

mpdonadio’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+PAReview: security

Automated Review

FILE: /var/www/drupal-7-pareview/pareview_temp/youtube_video_uploader.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
70 | ERROR | Array indentation error, expected 4 spaces but found 6
87 | ERROR | Array indentation error, expected 4 spaces but found 6
--------------------------------------------------------------------------------

All minor, but fix eventually.

Manual Review.

You should really link your reviews in the description above, as well as provide a link to PAreview.

(*) Project duplication is a major problem on drupal.org. Your description above, and the project page need list similar modules, and outline how this one is different. This information is scattered in the comments, but you need to centralize it in one place if you want to get this approved.

Your project page should have more information on dependencies, and information on where to get Zend-Google Gdata library.

The README should have a better link to the Zend-Google Gdata library in case a new version gets released. I would also give the path of at least one file in the library (eg, sites/all/libraries/Zend_Gdata/Zend/Gdata/YouTube.php) to help users make sure they have everything extracted properly.

In youtube_video_uploader_engine_settings_form_validate, you should really use the first argument for form_set_error() to make sure the form field gets the proper classes to show what is wrong. Also, since you marked the password and API key as required, you don't need to validate them here. Since you are only validating one field, you can use element level validation. Pretty sure the Form API docs have examples of this.

(*) You are calling module_load_include() at the global level. This can be bad. See https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo... for more info. require_once dirname(__FILE__) . '/youtube_video_uploader.field.inc'; at the top level is acceptable in this case. See also http://drupal.org/node/977052

Long term you may want to consider your own access permission(s) for the settings. Especially with content related configuration, finer grain control is beneficial with larger organizations.

(*) In youtube_video_uploader_page_callback(), you need to call ajax_footer() instead of exit(). This ensures that the Drupal shutdown functions run.

In youtube_video_uploader_options, the text is untranslated.

youtube_video_uploader_url_step_2() has untranslated text.

youtube_video_uploader_step_3() has untranslated text.

You have a dependency on Libraries, but only use it to get the path. The proper way to use this is to implement a hook_libraries_info().

(*) In youtube_video_uploader_page_callback(), I am pretty sure you have a problem with the $story_id. On line 119, you output the thumbnail image, and you use $story_id directly in the path. This value comes from your form via $form_state['values']['url'], and the ID is extracted with youtube_video_uploader_get_story_id(). You are extracting this via string manipulation using v= and & as boundaries. I think it is possible, though very unlikely, that this value would be output directly to a page w/ XSS. By very unlikely, I mean that at this point the video should be complete, so the story_id should be valid. I would check_plain() this as a precaution, and long term I would instigate one of the well-tested regexes for extracting the ID. I am adding the security tag, please don't remove the security tag, we keep that for statistics and to show examples of security problems. If a review administrator deems this as being too cautious, they will remove it.

I know this has been in the application queue for a long time, but I'm setting this back to Needs Work for the starred items. The first really needs to be addressed as part of final approval. The second two are rather big API issues, but easily remedied. The last is a potential security issue.

sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Issue summary:View changes
sunnyuff’s picture

Status:Needs work» Needs review

Hi mpdonadio,

Thank you very much for the review, please find my comments under.

Automated Review : All Fixed!!! Pareview Link

Manual Review.

(*) I have edited the project description and added the link to newest supported Gdata library. Since some new versions are not compatible with the module and there are many open issues so I think its better if I test any new versions instead of making the user in trouble.

I have added proper info about all dependent modules and libraries. Also I have updated the README with newest supported version of the library and added a full path of a file so user can be sure of that everything has been extracted properly.

Since I was just validating a single field so I have implemented it using element level validation.

(*) I have replaced module_load_include() with require_once() and I have added a module specific permission using hook_permissions.

(*) Replaced exit with ajax_footer. Implemented hook_libraries_info() and hook_requirements. I added hook requirements so user see some error if library is missing.

(*) Lastly using regex for extracting video ID from youtube URL is really a good idea and implemented, I am also using check_plain. Don't think there is any security issue because first it will go through regex and then check_plain.

Thanks for your review, really helped!!

sunnyuff’s picture

Issue summary:View changes
klausi’s picture

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus

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/youtube_video_uploader.install
    --------------------------------------------------------------------------------
    FOUND 1 ERROR AFFECTING 1 LINE
    --------------------------------------------------------------------------------
    5 | ERROR | Doc comment short description must end with a full stop
    --------------------------------------------------------------------------------

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. the differences to existing modules are not mentioned on the project page. Please add that, since we identified them already in the comments here. And please do not post pictures of differences, because that cannot be indexed for drupal.org search or search engines.
  2. youtube_video_uploader_field_schema(): why is there an index on the Video column? Please add a comment.
  3. youtube_video_uploader_form_node_form_alter(): why do you need to clear all those caches everytime the form is built? Please add a comment.
  4. youtube_video_uploader_get_story_id(): why do you call check_plain() here? You are not printing the ID to the user here. Sanitization should only be done when you are outputting something to HTML. See https://drupal.org/node/28984
  5. youtube_video_uploader_menu(): so the youtube_video_uploader_page_callback() is not protected at all, which means any anonymous user can access this callback. I fear that this means that any anonymous user can upload videos to your youtube account, so they could post whatever spam videos under your account. Could you add a comment to that hook_menu() line how you prevent that and where that protection happens? I wanted to test this, but got PHP fatal errors when trying an exploit (see next points).
  6. Installation if libraries is not enabled: PHP Fatal error: Call to undefined function libraries_detect() in youtube_video_uploader.install on line 41
  7. when visiting /youtube/0/upload: Fatal error: Call to undefined function ctools_modal_form_render() in youtube_video_uploader.module on line 174

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

PA robot’s picture

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

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

sunnyuff’s picture

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

Hi klausi,

Thanks for your review, Please find below the updates,

How it differs from other modules is added to project page.

1. No Errors : Pareview Link
2. Index Removed : Now there is no need to index video-id.
3. The form takes its title and description from the current node and saves in ctools cache, so it needs to be cleared everytime.
4. check_plain removed.
5. youtube_video_uploader_menu : Since its driven through ctools form and zend library so no one can actually access the url directly and will get a fatal error if anyone tries to do so, but yes it was a security issue I suppose, i added node access permission now.
6. libraries_detect should be done in runtime phase, fixed.
7. As I mentioned in #5, you cannot access the upload urls directly, you have to go via ctools forms.

Thanks.

sahil chadha’s picture

good one

Swarnendu-Dutta’s picture

Hi Sunny,

Thanks a lot for your contribution !!

Some minors :

  1. Wrong @doc comment

    * Implements system_settings_form (YouTube Engine Settings).

    Use this one instead

    * Configuration form for the general settings for Youtube Video Uploader.

  2. youtube_video_uploader.field.inc : function :
    theme_youtube_formatter_thumbnail : Use theme_image in place of $output .= "<img src='http://img.youtube.com/vi/$story_id/default.jpg' class='youtube_default' />";
    Similarly for function theme_youtube_formatter_hq.
Swarnendu-Dutta’s picture

Category:Task» Bug report
Status:Needs review» Needs work
StatusFileSize
new15.72 KB

Hi Sunny,

Whenever i switch between the two options, the previous form still displays.
For example if i select 'YouTube URL ' and click on 'Select Video', a pop up opens. If i close the pop-up and select another option, the same form appears with the validation error.
Images attached for reference.

And i am recieving An AJAX HTTP error occurred.HTTP Result Code: 500 error on uploading video.

AjitS’s picture

Category:Bug report» Task

Project applications are task and not a bug report.

PA robot’s picture

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

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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