[D7] Ziggeo video service

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

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.

• Please add hook_help in your module file.
• Please follow this URL ("https://pareview.sh") for finding the errors and correct it.

Hi @Stray14,
I tested your module and is really a great idea :-D.

1) I move the state needs work because before should give us a hand on testing at least three other modules to the community.
After you have made revisions and inserted the comment, copy the comments link at the top right and paste it on the main page of this issue, execute this operation minimum for tree times.
See if you like it:
Manual reviews of other projects

<ul>
  <li>https://www.drupal.org/node/XXXXXXX#comment-XXXXXXXX</li>
  <li>https://www.drupal.org/node/XXXXXXX#comment-XXXXXXXX</li>
  <li>https://www.drupal.org/node/XXXXXXX#comment-XXXXXXXX</li>
</ul>

cit: PA robot
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 :-)

2) During the test, in grabbed video area , have a four thumbnail, i save the content but not select thumbnail, the node with field ziggeo is black witout video grabbed :-(, you can fix it ? in my case, when create content type with this field, it is not required.

I have added the screen. see it.

Bye

@sjpagan: the review bonus program is voluntary and should not block an application. Please do a real manual review of the project's source code.

Hi @klausi,

I did a manual review of the source, there are some variables that : "in 1303 - $ embedding_width, 1304 - $ embedding_height, 220 - $ add_delete_translatable_field" might seem undefined, but upon closer inspection were evaluated all possible conditions.
This code is very long but it seems to me very well written.

Maybe some comments more could benefit.

Bye

Hi stray14,
below is my manual review :

1. on line number 368,1015,1077

#options' => array(
      '240x180' => '240x180 - SD (4:3)',
      '320x240' => '320x240 - SD (4:3)',
      '480x360' => '480x360 - SD (4:3)',
      '640x480' => '640x480 - SD (4:3)',
      '240x135' => '240x135 - HD (16:9)',
      '320x180' => '320x180 - HD (16:9)',
      '480x270' => '480x270 - HD (16:9)',
      '720x405' => '720x405 - HD (16:9)',

run through t() for translation

2. line no. 432 to 437 '320x240' => '320x240 - SD ' . t('Medium Quality') . ' (4:3)', Do not concatenate strings to translatable strings

Also Please don't RTBC your own issues, see the workflow:
https://www.drupal.org/node/532400

I'm starting a manual review on this application.
Just a small question before diving into the details, there is a special reason why you use a tracking code in the links of this module to ask signup for ziggeo services? Also in the README.txt it's there, here in the description - i'm just curious :)
Will get back with the results,
Bests,
Balazs.

Hi @Stray14,
after manual review of the code i've found the following issues that should be addressed before going to release and getting the Security Team approval:

• As Drupal7 minimum requirement is 5.2.4 the contrib used techniques requires at least 5.4 - see http://php.net/manual/en/migration54.new-features.php ziggeo.module:118 where there is an array dereferencing on a function - just put php = 5.4 to info file and it's solved.
• It's a bad practice to define constans in a function, especially depending on certain cases as it happens field_presave with ZIGGEO_ADD_TRANSLATABLE_FIELD and ZIGGEO_DELETE_TRANSLATABLE_FIELD. Because you code uses ZIGGEO_ADD_TRANSLATABLE_FIELD a bit later but outside of the condition where it is defined, php could throw a warning about undefined const - just drop them to the top of the file or it could even be stored in a variable, as you wish (if you choose this way, do not forget to delete them in uninstall).
• The form_alter seems could be less generic as it gets used on widget forms and field_ui_display_overview, maybe for performance reason it could be a good improvement to specify it.
• I would use t() for naming $remove_button_caption before adding it the elements in field_widget_info, line 785.
• Comment is misunderstandable in ziggeo_remove_button_ajax_callback() function as there is no ziggeo.js but ziggeo_widget.js (that is btw really well drupal written!!)
• Do not hardcode in code it should use public file system, maybe the site defines private one :) In ziggeo_image case of field_formatter_view i would change it to file_default_scheme() with adding '://'. And I'm not fully sure it won't bring up a performance issue as it always replaces the pictures from ziggeo even it doesn't changed, also system_retrieve_file with these parameters calls a file_save in the end of file_save_data, so i'm not sure why code changes the status of the file to 0 and saving it - the purpose is to not store the file on the file-system and let it be garbage-collected by cron runs?
• In theme_ziggeo_element how $output is built has a chance to get attacked by elements that are not sanitized (e.g. in field_widget_form where $video_token is given natively and can come from user input. I would sanitize at least $value before you send it to output at line 1542.
• In field_formatter_view i would use the second parameter of ziggeo_libraries to make it multilingual-supported here as well as in field_widget_form (or would handle it in ziggeo_libraries function with globalise $language).
• For me the documentation of the code seems a bit poor, no offense, but e.g. i had to think longer why you unset form items in validate callback, it kinda makes sense, but would be better to increase the inline documentation :) Also because .module is pretty huge (almost 2k lines), before it goes to release it would be a nice DX improvement to sort out things like create a .field.inc where the Field API hook implementations would go.

I can offer you my help on the mentioned issues as i think the module deserves some nice hours of contribution, it's a really well written code and high standards are followed, let me know if you think you need me to make it even better ;) :)
Bests,
Balazs.

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.

Hi @Stray14,
great to see your comment, also your fixes with answers - it will definitely help me tomorrow to check them and give you my feedback :)
Thanks for your professional habit and work, I'll get back to you tomorrow!
Bests,
Balazs.

Hi @Stray14,
as i've promised i checked your work, here are some details:
* Minimum requirement version of php is 5.5 since uses new functions (array_column()). This is already specified in the README.txt file. I specified it also in .info file. (commit 41ce12b) -> OK
* This constants are using for to understanding of complex code of function ziggeo_field_presave() and used inside this function. Not sense to use them for all module. But make sense to move the define to the begin of function. (commit 032b81a) -> Current version still has them not in the beginning of the function, but it's definitely not an application blocker :)
* Here adds validate and submit functions for widget and display. I can't see Another way to add validate and submit functions than form_alter. -> The issue here was more the generic form_alter then how validate and submit handlers are used, as this form_alter is called everytime when a form get generated/invoked, i guess these triggering_elements, field widget instances and the ziggeo_remove_button could be specified and moved to a more specific form_alter implementation. It's also not a blocker.
* Yes, forgot to wrap caption of Remove button by t() function. (commit f2d057b) -> OK
* Yes, forgot to change filename after rename of the file, thanks, fixed. (commit cbb9c5e) -> OK
* Images is an external resource (as same as a video), so I think that it should be downloaded from the server and should not stored on the site. There is a chance that the image on the server may change. Therefore, the status of file is 0 is set to make it temporary. I made check the existence of the file to reduce the number of downloads, but still, after a while, the file should be deleted. Also, if you re-record the video, the image should change, but it was not change. Therefore, to display a new image, I include the current stream token in the file name. Also removed the hardcode part of URL. I rewrote all this code, which, I think, will allow to achieve optimal performance. (commit 93f6b1d) -> The new version of ziggeo_image formatter view makes more sense then before, for sure with this all these 0 status files will be removed by cron (or other activities, depending on installation), but i think it's already improved, so i would go with it for the release (and then if someone reports performance issues in the public issue queue, then investigate on a more performance friendly solution). So it's also OK.
* $video_token does not come from user input. But other settings is come from user input and really can be output without sanitizing. I implemented two-factor checking of user input of settings: on settings form and on output using check_plain(). (commit e5f3d0) -> If i'm not mistaken $video_token does come from user input in ziggeo_field_widget_form() even it's used by trusted users, but with this implementation in the theme function, you make it safe (and catch is i guess in the best moment!) So it's also OK.
* Language parameter using only for recording instructions, i.e. only for widget. There are no text instructions on the player. Added comments about it. (commit f8f15c6) Makes sense, OK.
* I reread the code and added comments in places that were not quite clear after some time after writing the code. (commit f8f15c6) -> Inline docs, their quality, also quantity improved a lot! Thanks for it!
* Indeed, the amount of code in .module has become too large. I moved part of the code into the ziggeo.field.inc. (commit 035c9bf) -> Nice to see them separately, but in this case i would refer the file from .info to make sure it's cached well by drupal.

Overall i feel the whole solution improved a lot, if you don't mind i would put it back to 'Needs work' as some questions of mine need answers, but i also feel in a short time you'll reach the RTBC with it - i have to be honest, a lot of code i've reviewed in the last few years (custom and contributed ones too), but one of the best written is definitely this one, basically it's just good to read your code, very very nice work! :)

Bests,
Balazs.

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.

Hi @Kostya19,
i've checked again the codebase and found no issues that could block the application - it's a well written and documented contrib, could be a nice example to other maintainers :)
Let me change the status to RTBC and wish you the bests!
Cheers,
Balazs.

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.