[D7] Video Embed UStream

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

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.

Hi,

I have manually reviewed your code,

The quotes are not required in .info file.

Else, the code looks fine for me.

@manojbisht_drupal, thanks to you for contributing.

I installed this module and try to add content, after submission getting following issue:

Fatal error: Call to undefined function custom_module_knownId() in C:\xampp\htdocs\drupal731\sites\all\modules\2306561\video_embed_ustream.module on line 122

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2320099

Project 2: https://www.drupal.org/node/2311531

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Hello manojbisht_drupal,

I did quick review and following my code comments
1> Please add method definition for each and every method in module file
2> remove commented from file
3>Try to put helper function in different file so code will be in more readable format.

Thanks
Harshil

Hi manojbisht_drupal,

Please check the following link. It will show you an automated review result.
At present your application contains formatting related issues which you need to resolve first.

http://pareview.sh/pareview/httpgitdrupalorgsandboxmanojbisht8912306561git

Manual Review
---------------

Few things that I have noticed

1] All the function names do not follow Drupal naming conventions of functions.According to
Drupal coding standard(https://www.drupal.org/coding-standards#naming), the function name
should not have "Uppercase" letters. Rather you can user underscore. So according to me,
the following functions

_video_embed_ustream_getVideoProperties
_video_embed_ustream_getChannelProperties
_video_embed_ustream_getUserProperties
_video_embed_ustream_validId
_video_embed_ustream_knownId

can be renamed as
_video_embed_ustream_get_video_properties
_video_embed_ustream_get_channel_properties
_video_embed_ustream_get_user_properties
_video_embed_ustream_valid_id
_video_embed_ustream_known_id

2] The following functions do not have definitions

_video_embed_ustream_getVideoProperties
_video_embed_ustream_getChannelProperties
_video_embed_ustream_getUserProperties

3] Got an error after adding an url in the video embed field. [See image Video_embed_ustream_undefined_variable.png].
In the video_embed_ustream_handle_video function, _video_embed_ustream_check_type is called with '$embedCode'
which is neither defined in the function _video_embed_ustream_check_type nor passed to it.This variable is being
passed as arguments without initializing properly.

I request you to review my sandbox project also. Following is the URL

https://www.drupal.org/node/2322009

With Regards
Debarya Das

attached pic

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 :-)

Hi Manoj,

Can you please let me know the key differences between your module and those provided by Media Ustream.

https://www.drupal.org/project/media_ustream

Also, I tried putting different values inside Video field

1.

Got error: Video: Invalid Video URL.

Perfectly Fine.

2. Valid Ustream URL:
http://www.ustream.tv/recorded/17516386

Worked fine.

3. Invalid ustream URL:
http://www.ustream.tv/embed/recorded/17516386

Got a exception:

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.

In response to post #15, I think the major difference between this project and the module provided by ustream is there is no dependancy on the Media module ( Media 1.x or Media 2.x).
The only requirement here is the Video Embed Field Module.

For anybody who has run in to the file_entity issues with: was in media 1.x, is standalone in media 2.x well this looks to be clean and simple. I'm going to give it a go.

That was so sweet / easy :)
Don't forget to enable Ustream Video

/admin/structure/types/manage/video/fields/field_video
Add:
Select the allowed video providers

Youtube
Vimeo
+ Ustream Video

:)

---
Let me help test if you are still interested in promoting this @manojbisht_drupal

Hi manojbisht_drupal,

Pareview.sh is showing lots of warning - http://pareview.sh/pareview/httpgitdrupalorgsandboxmanojbisht8912306561git
which you need to resolve first.

Manual Review
---------------

1. No need to use quotes in .info file.
2. It would be better if your info file look like:

name = Video Embed Ustream
description = Provides Ustream handler for Video Embed Fields.
core = 7.x
package = Media
configure = admin/config/media/vef_video_styles

dependencies[] = video_embed_field

3) You should provide the hook_help to allow site builders to find information about your module using Drupal UI.

Hi manojbisht_drupal,

1) Please format all code in one standard and remove spaces on end of lines

2) At line 258 in security reason you need rewrite code from

$result = db_query("SELECT fid FROM {file_managed}
                          WHERE REPLACE(filename, ' ', '') = '$id'");

to

$result = db_query("SELECT fid FROM {file_managed}
                    WHERE REPLACE(filename, ' ', '') = :id", array(':id' => $id));

Based on #22 and #23, needs work.

Minor coding standard issues and a missing hook_help() are surely not application blockers, anything else that you found or should this be RTBC instead?

Assigning to myself for a review.

hi manojbisht_drupal,

there are some pending issues in
http://pareview.sh/pareview/httpgitdrupalorgsandboxmanojbisht8912306561git

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

EDIT: removed long pareview.sh dump.

@fabian.fernandes_30: please don't directly post the long pareview.sh dump, use a txt attachment instead or just post the link to the online service.

Please fix the coding standard corrections

http://pareview.sh/pareview/httpgitdrupalorgsandboxmanojbisht8912306561git

Removing review bonus tag, you you have not listed 3 manual reviews of other project in the issue summary. Make sure to read https://www.drupal.org/node/1975228 again.

Hi Manoj,

Your module is working fine.

There are few suggestions:

1. Implement hook_help().
2. Provide a valid configure link : 'configure = admin/config/media/vef_video_styles' in .info file.

Thanks for your contributions.I would suggest you to take a review bonus to speed up the process. Please help reviewing and put yourself on the high priority list, then GIT Admin will take a look at your project right away :-)

Hi @manojbisht_drupal,

I have manually reviewed your code. I found that this functionality is already available in Media: Ustream. Also the code written in your module is available in Media: Ustream.

Duplication

This sounds like a feature that should live in the existing Media: Ustream project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Media: Ustream 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".

I think this was RTBC already.

@manojbisht_drupal,

This feature can still exist in Media: Ustream module!

Reviewing it now.

Automated Review

Everything good! http://pareview.sh/pareview/httpgitdrupalorgsandboxmanojbisht8912306561git

Note that perfect adherence to Drupal Coding Standard is NOT a reason to block an application, except for total disregard of them. However, modules should follow them as closely as possible.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements

Coding style & Drupal API usage

1. Project page needs to be updated. Please add in more details. You can check the project of of Video embed field module for an example.
2. (*) It seems that the embed code of format '@ustream\.tv/([a-zA-Z0-9_\-]+)$@i' accepts invalid ustream URLs as well; or the module doesn't handle it well. Ex embed codes that were accepted during my tests:
   1. http://www.ustream.tv/arirangtv
   2. http://www.ustream.tv/embed/14067349?html5ui
   3. http://www.ustream.tv/<script>alert('hi');</script>
   4. http://www.ustream.tv/recorded/54591846
   5. http://www.ustream.tv/channel/ghosttw
   6. ustream.tv/user/growveinitiative

   NOTE: In case of invalid URL #3 from the list above, the iframe shows the typical 404 error from ustream site. See screenshot below:
   

   This should be handled in the validation, or support for such URL format should be removed.

3. (*) On node add page the following error is displayed:
   

   Notice: Undefined offset: 1 in _video_embed_ustream_get_video_id() (line 304 of /htdocs/d7/sites/all/modules/sandbox/video_embed_ustream/video_embed_ustream.module).

4. Comments on the functions could be improved so that it would be easy for people to understand. They are in format Implementation of foo_bar. if the name of the function is _foo_bar(). These type of comments are typically used if the function is a hook implementation (correct way - Implements hook_foo().). As these are not hook implementations, the comments needs to improved to mention what the functions do exactly.
5. I would recommend to remove the debugging comment // print_r($embed_url);die('Dying in Embed URL');.

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.

This review uses the Project Application Review Template.

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.

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• 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. _video_embed_ustream_check_input(): doc block is wrong, this is not a hook. What does this function check/validate? Please describe that oin the doc block. Same for _video_embed_ustream_known_id(() and possibly others. Please check all your doc blocks.
2. _video_embed_ustream_known_id(): you must never concatenate strings directly into DB queries, always use placeholders instead. Make sure to read https://www.drupal.org/writing-secure-code about SQL injection.
3. _video_embed_ustream_valid_id(): this function throws exceptions, are they caught somewhere? Please add a comment. Looks like Drupal will throw a 500 error if the exceptions are not caught anywhere?
4. video_embed_ustream_form(): doc block is wrong, see https://www.drupal.org/coding-standards/docs#forms

The wrong db_query() usage is a blocker right now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder:

FILE: /var/www/drupal-7-pareview/pareview_temp/video_embed_ustream.module
-------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
-------------------------------------------------------------------------
48 | ERROR | Missing parameter comment
50 | ERROR | Description for the @return value must be on the next line
-------------------------------------------------------------------------

Manual Review

It is working fine for me and i don't see any major blocker in the module.

Thanks,

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

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/video_embed_ustream.module
  --------------------------------------------------------------------------
  FOUND 6 ERRORS AFFECTING 6 LINES
  --------------------------------------------------------------------------
    48 | ERROR | [ ] Missing parameter comment
    50 | ERROR | [ ] Description for the @return value must be on the next
       |       |     line
   222 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
   230 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
   238 | ERROR | [x] Line indented incorrectly; expected 6 spaces, found 8
   269 | ERROR | [x] Object operator not indented correctly; expected 4
       |       |     spaces but found 6
  --------------------------------------------------------------------------
  PHPCBF CAN FIX THE 4 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. the doc blocks of most function are wrong and just repeat the function name. Please describe what each function is doing in the comment if it is not a hook implementation.
2. _video_embed_ustream_parse_url(): @return comment is missing. Sometimes this function return a URI and sometimes NULL, but in what case?
3. _video_embed_ustream_parse_url(): watchdog() should use the module name as first parameter, see https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/watc...

But otherwise looks good to me.

Thanks for your contribution, Manoj!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.