Youtube Formatter

You have some issues with coding guideline. You can see drupal coding guideline issue at http://ventral.org/pareview/httpgitdrupalorgsandboxhydra1404188git

and you will have to work in separate git branch rather than in master branch. more information http://drupal.org/node/1127732

- You need to provide a non-maintainer URL to the repo and to the correct branch.

Manual review of branch 7.x-1.x:
- youtube_formatter_field_formatter_request_youtube_uri(): If $value does not contain a valid URL but just some text, a broken image is rendered and I get this notice:
Notice: Undefined offset: 3 in youtube_formatter_field_formatter_request_youtube_uri() (line 221 of /Users/Rade/workspace/d7/sites/all/modules/custom/youtube_formatter/youtube_formatter.module).
- Even though I set "Show in HD" to Yes the video's don't play in HD by default. Tried with videos that have both 720 and 1080 versions available.

Otherwise seems like a good module! There are however a few similar modules. Please describe how your module differs from the modules below. This could also be added on your project page under the title "Similar modules".
http://drupal.org/project/media_youtube
http://drupal.org/project/video_filter

Hello Hydra! Great job, but you need to tweak this module a little bit to make it even better.

Manual review:
1) The HD option dose not work. I added a video that supports 1080p with setting "Dimensions: 1280x720, Play in HD: Yes" and video plays in 480p . After doing a research on this problem i found that your code is ok, but youtube is trying to play it smart and disable the HD embed feature. Please read this and this. In simple words the only way to embed HD is to use the old embed code and force the youtube player API version to 2 (then add the hd=1 parameter). I even tried to add a 1920x1080 iframe but the video was still playing in 480p.
2) Add the option to autoplay the embed video. Its not that hard and its very useful. But if you do that make an if statement that will check if there are other videos on the page and only autostart the first video.
3) Add support for youtube playlists.

Overall its a great module but i the following modules do the same job:
http://drupal.org/project/media_youtube
http://drupal.org/project/video_filter
http://drupal.org/project/youtube (which is pretty similar with yours but has views integration)

If you plan to have success with your project you must add more features than the modules shown above. Good luck!

Hi Hydra,

The automatic review shows some small formatting issues: http://ventral.org/pareview/httpgitdrupalorgsandboxhydra1404188git

I have also performed a manual review:

• I see an issue in function youtube_formatter_field_formatter_youtube_colorbox(). The $settings['safe_value'] can already be a URL which contains the "?" character (see line 281 in the case of a playlist). On line 89 you append another "?" to the URL. A possible fix could be like this:
  • In function youtube_formatter_field_formatter_get_settings(): In case of a playlist, don't add the "?list=$id" to the URL, but add it to the $parameters array.
  • In the same function, don't use drupal_http_build_query() yet, but pass the $parameters array as follows $new_settings['query'] = $parameters;
  • In function youtube_formatter_field_formatter_youtube_colorbox(), replace line 89 with the following:
    
    

    $settings['query'] = array_merge(
      $settings['query'],
      array(
        'width' => $settings['width'],
        'height' => $settings['height'],
        'iframe' => true,
      ),
    );
    $video_uri = url($settings['safe_value'], array('query' => $settings['query']));

    Also note that instead of building $video_uri with the url function, you can also do everyting the l() function like l('your text', $settings['safe_value'], array('html' => TRUE, 'attributes' => array('class' => 'colorbox-load'), 'query' => $settings['query']));

• Same thing on line 122: use the url() function to build the URL instead of concatenating safe_value and query.
• Line 150: use t() around the description to allow translation.

For the rest I don't immediately see any issues with your code.

Hi Hydra,

• Please change line endings in your IDE to "Unix line endings only".
• I wouldn't worry about the short comment error it seems to be a mistake. You could refactor the comment if you want rid of it.
• It would be nice if each of your functions had a little description
• You should add a hook_help()
• Line 54 of youtube_formatter.module you have a redundant assignment $settings = $display['settings']
• Line 254 has a redundant assignment. $file = file_save($file);
• Your comments seem to have a few typos. I know english may not be your first language so ill try point them out. Line 237 "preview", Line 266 "invalid", Line 283 "Helper function" , Line 295 "auto play", Line 189 "Auto play"
• Line 121 Them ternary operations seem to be getting a little hard for everyone to read? Only my opinion though.

Once they've been fixed up I think it's looking pretty good :).

Nice module.

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

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hi Hydra,

really nice module! I like the easy way in which it can be used.

the automated review shows some errors: http://ventral.org/pareview/httpgitdrupalorgsandboxhydra1404188git-7x-1x, but I don't see any problem with that.

I manually reviewed your module:

1) Please set the right git url at "git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Hydra/1404188.git youtube_formatter" in the Issue Summary of this page and in the project page
2) You should define @param and @return in the function definition: this seems to be optional (see http://drupal.org/node/1792992) but I think it is a good practice.
3) In youtube_formatter_help (youtube_formatter.module, row #15) you get $current_theme without using it: please remove this row or use the value.

I also tested your module and the "Youtube video" format works really good.

Instead with "Youtube image" format I can't make it to work: please double check it.
If I inspect the dom, I can see this URL:
http://mysite.local/sites/default/files/styles/medium/http/img.youtube.com/vi/TZLwfyVYJJw/0.jpg
Maybe it should be http://img.youtube.com/vi/TZLwfyVYJJw/0.jpg?

Cheers and good luck!

Sounds very similar to the youtube field module http://drupal.org/project/youtube

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.

changed clone url to non maintiner