[D7] Video Embed Instagram

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.

Thank you for module!

Some notes:

• "Configuration form" use instead of "Our configuration form " in comments. "Return a render array." use instead of "We want to return a render array."
• $embed = '<iframe src="http://instagram.com/p/' . $id . '/embed/" width=!width height=!height></iframe>'; - use theme() function for HTML code. https://api.drupal.org/api/drupal/includes!theme.inc/function/theme/7
• Add addition information in README.txt file. See https://drupal.org/node/2181737.

Hi, Impol!

Small, clean code - module seems to be stable and correct. Perhaps you need to add more features in your module?

Hi, Impol,

I agree, code seems clean and everything looks correct.

My only issue was I couldn't actually embed a video but not sure if your module was at fault or if it was user error having not ever used Instagram before.

After having added a Video Embed field I searched for an example Instagram video url to use. I tried with the following 2 urls:

• http://distilleryvesper3-7.ak.instagram.com/2ea7416ad9e311e28f2322000a9e...
• http://distilleryvesper9-13.ak.instagram.com/090d06dad9cd11e2aa091231381...

But when I tried to save the node I got the error message:

Video: This video provider is not currently supported.

I'm got to set to needs to work as if it is user error maybe you could include in your documentation how to get the correct URL to use, and if it is not then it needs fixing.

Yeah - I had a feeling I was over complicating things!
Thanks for the clarification - works perfectly now.

I think the example URL on the project page is definitely useful - I think it would be a good idea to put this in the readme too.

I can see what Chalk was saying about it being a small module but this post suggests 5 functions and 120 lines of code as a minimum, and you have 6 functions and 160 lines of code so I'm going to go ahead and set RTBC status.

Manual Review

Did you ask the Video Embed Field maintainers whether they would entertain this as a submodule (like the FB one that comes with it)?

I think that some may deem the image on the project page a little offensive.

Your settings go straight to the output w/o filtering. Try setting the width to foo"></iframe><script>alert('XXX');</script> to see it in action. Risk is mitigated by needing to be able to manage the video styles, which is typically an admin thing, but this is still a problem. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

You should probably validate height and width with the Form API and element_validate_integer_positive, but validation doesn't count as XSS prevention.

Your helper functions should have proper docblocks.

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

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

Removing review bonus tag, you have not done any manual review, 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.

Hello,

I have just reviewed and tested the module. It works.

About the comment #16. I test that there is a verification on the config form, OK. But there is no verification in the template. I do not know if you want to use a check_plain at this moment of the output.

As it is my second application review and there is the question on security, I do not know if I can change the status to RTBC.

Thanks for your module.

@impol,
As per #16, Did you ask the Video Embed Field maintainers whether they would entertain this as a submodule (like the FB one that comes with it) ?
Would you please open a follow up(by support request in the module issue queue) with the video embed field maintainers regarding whether they would entertain this module also as the submodule ?
I am setting this to 'needs work' for the follow up with video embed field module.Please do it to 'needs review' after follow up.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away as @klausi mentioned in #21

@impol,
As the features this module provides is awesome,
Just for decreasing the duplication of the projects on drupal.org I just suggested to ask the video embed field module maintainers.
Please get the review bonus to make your project application in high priority list, then git administrators will take a look at your project right away :-)

Hello @impol and thank you for your work!

video_embed_instagram.module:
Line 183:

You evals if there're matches checking the parameter by reference in preg_match. Instead you can check the returned value by preg_match function.

  if (preg_match('/instagram\.com\/p\/(.*)[\/]?/', $url, $matches)) {

It returns 1 if matches, 0 if not and FALSE on error.

Hope it helps and thank you.

Manual Review

No duplication

Yes: I don't believe this module should be merged into the video_embed_field module as that would mean that all users who download video_embed_field would be forced to download an Instagram component that they may or may not need. Also that would make maintainability of video_embed_field harder - so you've done the right thing keeping this separate

README.txt/README.md

Yes. Looks great.

Coding style & Drupal API usage

1. MINOR: implementation of hook_video_embed_handler_info contains many unnecessary inline comments that will be hard to maintain and don't do much to enhance understanding.
2. MINOR: despite lots of inline comments I still haven't been able to work out why VIDEO_EMBED_INSTAGRAM_DEFAULT_WIDTH and VIDEO_EMBED_INSTAGRAM_DEFAULT_HEIGHT are 616 and 714 respectively. These seem like unusual dimensions. Need some documentation above the constant definitions stating the reason for those defualt dimensions.
3. MINOR: implementation of hook_theme contains too many uncessary comments. It feels like the comments are explaining how the API works rather than the reasons for unusual module specific decisions.
4. MINOR: in video_embed_instagram_handle_video & video_embed_instagram_handle_thumbnail variable name $id is not verbose enough to allow easy debugging and interpretation. Suggest using the name $video_id or $instagram_video_id as $id is too generic.
5. MINOR: in video_embed_instagram_handle_video and _video_embed_instagram_get_video_id single input/output is recommended for easy maintenance and debugging. The function should start with $output = ''; and at the end return $output. Managing multiple returns within if statements leads to hard to maintain code and more room for error.
6. (+) MAJOR: in video_embed_instagram.tpl.php width and height of the iframe should be in quotes to be W3
7. MINOR: (nice to have) frameborder="0" on the iFrame. Borders on iFrames can easily be added and modified using CSS without overriding the TPL but unfortunately frameborder cannot, so setting it to 0 means people will not have to override the tpl file when they do their theming.

Code long/complex enough for review

Yes (only just)

I think this is a good project for a first time contributor to maintain. It's not overly complex, enough people will want to use it (I get a lot of requests for Instagram requests on client projects) and everything is to standard.

No tests written for it, however it's simple enough that it doesn't need them as yet and video_embed_field doesn't have any tests (we really need to get more people to write tests for their contrib projects!).

This review uses the Project Application Review Template.

Module looks great, works well.

My comments

In video_embed_instagram.module, line 120 and 150, you are forcing the use of http as protocol. If the website uses https this request to a non https may cause some warnings in the browser and even broke the functionality (more info https://developer.mozilla.org/en-US/docs/Security/MixedContent/How_to_fix_website_with_mixed_content).
It is possible to use // or detect the used protocol in each page load ?

In video_embed_instagram.tpl.php, use check_plain to print $height and $width, any non expected char may break the generated HTML. Don't forget the "" that were mentioned before.

Thank you for the effort, hope to see this module ready soon

Multiple Applications

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

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

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

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.

Do you still want to contribute this module ? As duplication is no longer a blocker so make this active if you want to ?

Module is Nice!
Review of the 7.x-1.x branch (commit f69ee3d):

• 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. Add a hook_help in module at the top of all functions in .module file.
2. _video_embed_instagram_get_video_id : Please add comment above preg_replace and preg_match functions what they do so that it will be handler for others developers to contribute to the module easier.
3. Please add more details on the project page about the module.Use the project page tips page https://www.drupal.org/node/997024

Rest looks good to me.RTBC :)
Please take a review bonus to come in high priority list

Assigning to heddn to take a final look at this if he has time.

@impol
Please take review bonus by reviewing 3 project applications.

1. Using render arrays rather than calling theme() directly is prefered. See https://www.drupal.org/node/930760 & https://api.drupal.org/api/examples/render_example%21render_example.modu... for some examples.
2. I didn't figure out a way to exploit it, but I have a theory that sending a properly crafted URL string through _video_embed_instagram_get_video_id() could result in an XSS. Maybe through a check_plain on the video id to be safe.
3. Last feedback, rather than putting all that logic into the tpl, try using a preprocess function. It will move all that PHP code (i.e. check_plain) back into a php file, rather than putting it into the theme layer.

None of these are blockers, although I'd recommend fixing #2. #2 isn't a blocker because I wasn't able to prove it is a security bug.

Thanks for your contribution, Yauheni!

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.