This module provides Instagram handler for Video Embed Field.
Users can add Instagram videos to their site by pasting the video's URL into a video embed field.

Simplytest.me link : http://simplytest.me/project/2207411

Project page:
https://drupal.org/sandbox/impol/2207411
http://git.drupal.org/sandbox/impol/2207411.git

GIT
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/impol/2207411.git video_embed_instagram

Automated Review
http://pareview.sh/pareview/httpgitdrupalorgsandboximpol2207411git

Manual reviews of other projects:
1. https://www.drupal.org/node/2526480#comment-10089246
2. https://www.drupal.org/node/2528016#comment-10092286
3. https://www.drupal.org/node/2522976#comment-10092368

Comments

impol’s picture

Issue summary: View changes

Changed issue's summary following d.org requirements

impol’s picture

Issue summary: View changes
PA robot’s picture

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.

Plazik’s picture

Status: Needs review » Needs work

Thank you for module!

Some notes:

impol’s picture

Status: Needs work » Needs review
  • Added additional information in README.txt
  • Use theme() function for HTML code.
impol’s picture

Issue summary: View changes

Added review comment

impol’s picture

Issue summary: View changes
Issue tags: +PAReview: review bonus

Review added.

impol’s picture

Issue summary: View changes

Added Simplytest.me link

Chalk’s picture

Hi, Impol!

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

dahousecat’s picture

Status: Needs review » Needs work

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:

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.

impol’s picture

Hi, dahousecat. Maybe it's my bad that i don't have provided proper description, but you only need use following urls : http://instagram.com/p/miqK7HMFz3/ or http://instagram.com/p/k0S3FqsFzC/ . No need to paste the raw url to video file :)

impol’s picture

I do some research and find that error message toggled from embed_video module.
I added 2 warnings to code, when module can't get the video's thumbnail or video id.
Updated project page.

impol’s picture

Status: Needs work » Needs review
dahousecat’s picture

Status: Needs review » Reviewed & tested by the community

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.

impol’s picture

Yeah, exactly! I will update readme file, too. It is will be useful for users.

I agree with you, I think this module doesn't need additional functionality. But in my plans create a module for integration instagram with scald and a module that provides the ability to integrate with funny site http://coub.com/

mpdonadio’s picture

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

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.

impol’s picture

Status: Needs work » Needs review

Added proper comments for helper functions.
Fix some code styles.

#16
I think I don't need to add this module to video embed field project.

PA robot’s picture

Status: Needs review » Needs work

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.

impol’s picture

There are some errors reported by automated review tools, did you already check them?

Thank you robot, I forgot :)
fixed

impol’s picture

Status: Needs work » Needs review
klausi’s picture

Issue summary: View changes
Issue tags: -PAReview: review bonus

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.

Grimreaper’s picture

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.

naveenvalecha’s picture

Issue summary: View changes
Status: Needs review » Needs work

@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’s picture

Status: Needs work » Needs review

I think it would be easier to close this project...
I already explained, that I don`t wont to do this module as part of other module(#17), it is my first project on do, why I need to give all my ideas to the video embed field developers.

naveenvalecha’s picture

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

mdalda’s picture

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.

codesidekick’s picture

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.

casivaagustin’s picture

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

impol’s picture

Hi, casivaagustin
Good comment, I will fix it now

PA robot’s picture

Status: Needs review » Closed (duplicate)
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.

impol’s picture

Fixed all issues.

naveenvalecha’s picture

Status: Closed (duplicate) » Active

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

naveenvalecha’s picture

Assigned: Unassigned » heddn
Status: Active » Reviewed & tested by the community

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’s picture

Do you still want to contribute this module ?

Yeah, exactly. It was closed by robot, because I started new project(It's already in status RTBC).

naveenvalecha’s picture

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

impol’s picture

Issue summary: View changes
impol’s picture

Issue summary: View changes
impol’s picture

Issue summary: View changes
impol’s picture

@naveenvalecha Can I get a review bonus now? I am done with 3 reviews.

impol’s picture

Issue tags: +PAReview: review bonus
heddn’s picture

Status: Reviewed & tested by the community » Fixed
  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:

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.

impol’s picture

Thanks, heddn!
Will fix at least #2 and #3 before promoting project.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.