[D7] Media watermark

Link to the project page and git clone command are missing in the issue summary, please add them.

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.

added drupal git repository link

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

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

Hello Bogdan1988,

Great idea for this module but there are some things that need resolved.

1. You project page needs some work as it is missing some recommended items illustrated here https://drupal.org/node/997024
2. When trying to edit the fields of an existing watermark, an error appears stating "The file used in the Watermark image field may not be referenced." and new settings are not saved.

Hello Bogdan1988,

I reviewed your module. For me, images were uploading correctly but without a watermark. I'm not sure why.
Nevertheless, here are some suggestions to improve it and help it to be accepted:

• On admin/config/media/watermark, you hard-coded Edit and Delete URLs. This will not work without clean URLs turned on. You should always use l() or url() to generate valid links or URLs.
• You should provide better quality descriptions for your form elements. Some of them are even incorrect, example: allowed files resolution is .png (=> should be ...files extension..., and start the sentence with a capital letter, etc...).
• Similarly, your review request should be edited and made more clear, give the steps to go into to make it work, give the required modules, etc... Even the Git command is incorrect, for example. Please also consider providing online help for your module.
• Coder reports some issues, you should fix the ones about not using t() for i18n (the others are false positive and can be ignored).
• When uploading an image using your module, I got the following warning: Warning: Illegal string offset 'name' in image_style_flush() (line 956 of /XXX/modules/image/image.module). You should check your calls to image_style_flush(). For your information, I used Drupal 7.22.
• For image processing code, you should make your code independent of GD (eg. don't use native GD functions such as imagesx(), imagecopy(), etc...). Instead, prefer the use of Drupal's image toolkit API (see image_toolkit_invoke() to get started).
• In your SQL queries, try to be consistent, always use SELECT ... FROM ... WHERE ... instead of mixing select ... from ... where ... and SELECT ... FROM ... WHERE ..., or even better: use lower-level API like db_select().
• Optionally, your class MediaWatermark may be defined in its own .inc (and loaded only when needed) for performance and clarity.

I would be glad to review your module again once these tasks have been taken into consideration.

I would be glad to review your module again once these tasks have been taken into consideration.

What is the difference between Image actions module, which also provide a watermark effect?

I am the maintainer of "Image Actions" and I welcome this module into the space, as I believe (based on the project description) it helps user experience in a different place than the actual "image actions" do.

However, I found the current project page unformatted and difficult to understand. Like, unreadable.

I tried:
Enabling the current D7 of Media watermark.
- I got an error on install. It seems this has a hard dependency on core image module 7005+ That's not your fault, but it would be nice if other Drupal 7 users did not depend on this or there was a graceful fallback.
- visiting admin/config/media/watermark the 'add' option is in a subtle tab. Many Drupal modules at least prompt us to "add a new X" if the current list is empty.
- On my very first attempt, I found that GIF files were not allowed/ Only PNG could be watermarks. Hm.
- On selection a PNG, I got a warning

Warning: Invalid argument supplied for foreach() in image_style_create_derivative() (line 922 of **/modules/image/image.module).
Recoverable fatal error: Argument 1 passed to imagecache_canvasactions_image_style_flush() must be an array, string given in imagecache_canvasactions_image_style_flush() (line 176 of ***/sites/all/modules/patched/imagecache_actions/canvasactions/imagecache_canvasactions.module).

though that may have been mostly due to my dirty test platform. Not sure.

... And for further info you supply instructions in ODT?
Well, no.
For developers, the instructions are in txt, html, or md. This does not work on the commandline.
Opening that document was ... quite awful.

I could figure out that the next step (I think) is to use media module to upload an image and there may be additional options for watermarking.

... I could not immediately see how to add my new watermark to existing media. As available in the media_modle library at admin/content/file

- when trying to upload a new file, I saw only the last uploaded watermark, and no ability to upload.

I think I need to re-test on a clean site that has no existing media assets to see how it performs. In the meantime ... have you tested to see what does happen if you turn this module on in an existing site? I'm getting many very hard-to-explain errors when enabling this on a working (demo) site...

I think we can call this "needs work" based on #13.

I'll admit that my trial was not a 'clean-room' test as I usually do. I had a scratch site already using media_module that I inflicted upon it.
But the real-world environment is like that. We need to anticipate what happens if someone installs this module on an existing site that already has a number of assets in play.
My test site was not deliberately difficult - I just gave an honest step-by-step user report of what happened when I turned this on on a previously OK (though maybe unusual) site that was using media.module already.

How many error conditions have you anticipated and handled in the code? This is the test...

My issue was not with trying this module on a clean site (which is easier) but trying it on an existing one - which is harder to predict.

To replicate somewhat:
* I built a new empty site using 'default' install profile
* Added media module and media field.
* added a media field to my articles content type
* Added three articles, with image media assets attached

Assume at this point I have something like an existing site...

* enabled the media watermark module, and visited admin/config/media/watermark
* added a new logo for watermarking. PNG type this time.
* visited my current media library at admin/content/file - could not see how to add a watermark there.

???? This can have no effect on existing assets?

Tried adding another article. When adding a media asset, the new logo was dominating my UI. At least I can see it's doing something. unhelpful though.

So, having chosen a watermark at that point - while making a node. .... where do I see it? I viewed my new page, that had a "watermarked" media (a png) embededded -but saw no change to the image.

Where do watermarks happen? if not to derivative images used in displays, then where...?

On re-editing the filed config (eg)
"The file used in the Watermark image field may not be referenced.

I'm lost on how to use this. I need to spend a few hours deciphering the ODF maybe.

Sorry, it seems that I had a significantly older (one year old) version of media module running. Many of the confusing errors and scrambled UI I was seeing may have been because this module requires a newer version of 'media' than the site had installed?

I tried upgrading media module to the latest 2.x code - and now that's just broken - not your fault.

Is there a specific version of media module that you have successfully been using?

Updated media.module and file_entity to their more recent stable releases.
Got a little further this time.

Was able to configure a watermark to use.
Added a media field to a content type and started to create content.
When adding the media field, to try and attach a PNG, the popup showed my watermark (above the normal file upload area - I really think it should be secondary)
When trying to continue, I got the attached error.
I'm guessing that you don't allow watermarks to be applied to PNG files?

tried with a JPEG instead. Got one screen further this tie, then batch errors and a WSOD when trying to 'finish'
Had to visit a new page, and found this:

... this happens each time.
* create content
* attach media item
* upload a jpeg
* 'Save'
= Batch error "Fatal error: Call to a member function getAllItems() on a non-object in /Library/WebServer/Documents/drupal7/includes/batch.inc on line 465" and WSOD

Bogdan1988, please change the git command in the issue summary to use the non-maintainer url.

You should also be working in your 7.x-1.x branch instead of 7.x-1.1.

Documentation standards-
Your .module @file docblock summary should be a full sentence ending in punctuation.

Your other @file docblocks have multiple lines. Include a blank line between the summary and further documentation.

Use your function docblock to describe the function which adds a lot of value for developers reading your code. Some of your docblock summaries are "Function" prefixed to the name of the function. e.g.

Function media_watermark_prepare_names().

You may want to clarify the description of the permission you are defining. Right now it is confusing. Perhaps something like "Manage watermarks on file entities".

Seeing the "No Watermark" checkbox on file upload seems like an anti-pattern. Consider changing this to Add Watermark which will then present the watermark options. Also, consider adding this at the bottom of the upload form (or it's own step in the wizard) so it's not the first thing someone seems when they upload their file.

When I open admin/config/media/image-styles/edit/media_watermark, it shows error:
Notice: Undefined index: height in theme_image_resize_summary() (line 860 of F:\wamp\www\location\modules\image\image.admin.inc).

I find the related codes in media_watermark.install:

  $effect = array(
    'name' => 'image_scale',
    'data' => array(
      'width' => 200,
      'upscale' => FALSE,
    ),
    'isid' => $style['isid'],
  );

it needs height defination. I suggest:

  $effect = array(
    'name' => 'image_scale',
    'data' => array(
      'width' => 200,
      'height' => NULL,
      'upscale' => 0,
    ),
    'isid' => $style['isid'],
  );

Possible Duplication

It seems that ImageCache Actions provides very similar functionality. The first bullet point on the project page is "watermarking". Could you describe how your module differs? When would someone use your module instead?

We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module. (If the existing module is abandoned, please think about taking it over).

If that fails for whatever reason please get back to us and set this back to "needs review".

You also have a number of code style issues reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxbogdan19882036479git.

----
Top Shelf Modules - Crafted, Curated, Contributed.

As in #13

I am the maintainer of "Image (cache) Actions" and I welcome this module into the space, as I believe (based on the project description) it helps user experience in a different place than the actual "image actions" do.

Doh! Sorry.

Project page

Please take a moment to make your project page follow tips for a great project page.

• You should remove these 2 branches, 7.x-1.0-alpha1 and 7.x-1.1. Once this sandbox is promoted to a full module you'll be able to create release tags like 1.0 and 1.1.
• You have a number of style issues that should be resolved, reported here: http://pareview.sh/pareview/httpgitdrupalorgsandboxbogdan19882036479git.
• Can you remove media_watermark_update_7001() and put all that code in hook_install()?
• In hook_menu(), the edit link still has a title and description of "Add watermark"
• I think you actually support more image types than what is listed in the README?
• Instead of media_watermark_get_file_extension(), can you use pathinfo($file_name, PATHINFO_EXTENSION) instead?
• The docblock for media_watermark_form() is wrong, it's not implementing a hook, just defining a form. hook_form is only for modules that define a content type. Same for media_watermark_delete_form() and media_watermark_edit_settings().
• In media_watermark_delete_form(), don't concatenate strings using t() like that - you want to use whole sentences so a translator can understand the context. Instead use t('Are you sure you want to delete %name?', array('%name' => $name)
• Also in media_watermark_delete_form(), this is not a system settings form. Can you use confirm_form() instead? That should allow you to pass a value into the submit function without having to read arg(5) again.

Looks like a great module! Setting to needs work for the branch issues and number of style issues, but otherwise this is RTBC from me.

----
Top Shelf Modules - Crafted, Curated, Contributed.

changes

You have many long lines. In https://drupal.org/coding-standards#linelength there are various conditions where this is ok but, for example, in media_watermark.module:

200   $watermark_obj = db_query("SELECT * FROM {media_watermark} WHERE fid = :fid", array(':fid' => $form_

is what I see. It's much more readable like this:

200   $watermark_obj = db_query("SELECT * FROM {media_watermark} WHERE fid = :fid",
201     array(':fid' => $form_state['values']['watermarks_names']));

Other than that I find the code nice and readable. Good work!

Thanks for those updates! I'm unassigning this issue so that other reviewers know this one is open for final review.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Review of the 7.x-1.x branch:

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

  
  FILE: /home/klausi/pareview_temp/includes/media_watermark_edit.admin.inc
  --------------------------------------------------------------------------------
  FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   39 | ERROR | Key specified for array entry; first entry has no key
  --------------------------------------------------------------------------------
  

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. media_watermark_get_sort_table_watermarks(): why do you have to call drupal_render() here and cannot just return a render array? Usually Drupal does the rendering for you, so please add a comment why you have to do it beforehand.
2. media_watermark_permission(): this permission is never used, why did you define it?
3. media_watermark_add_upload_multiple_submit(): do not document $form and $form_state here, see https://drupal.org/node/1354#forms . Also elsewhere.
4. media_watermark_form_validate(): this looks like it should happen in a submit handler, not in a validation handler. You are not validating stuff here?
5. media_watermark_delete_form(): do not call "print" directly, always return strings or render arrays!
6. media_watermark_get_sort_table_watermarks(): this is vulnerable to XSS exploits. If I enter a watermark name <script>alert('XSS');</script> I will get a nasty javascript popup on the overview page. You need to sanitize user provided text before printing, make sure to read https://drupal.org/node/28984 again. Strictly speaking this is not a security vulnerability because the "administer site configuration" permission is required to inject the malicious snippet in the first place, but this should be fixed anyway.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Hi Bogdan,

Great idea for a module! I have some pointers before you release:

1. media_watermark.info:8 The Class MediaWatermark is only used in function media_watermark_create_image(). Instead of including the file in every page request, you can include the class file conditionally with module_load_include() to reduce memory load.
2. media_watermark.module:30 Can you add the list callback as the '#type' => MENU_DEFAULT_LOCAL_TASK to enable the administration tabs for 'List' and 'Add'?
3. media_watermark.module:50 If the placeholder % represents a database object, you can use %media_watermark instead and create your own media_watermark_load function to load the watermark object from the database. This object then becomes available as an argument in your page callback if you pass it with hook_menu.
4. media_watermark.module:80 You can use db_query()->fetchAllAssoc() instead of fetchAll(PDO::FETCH_ASSOC). The docblock suggests it returns an array, but the return value can be empty when the result is empty.
5. includes/media_watermark_edit.admin.inc:15 If you are not using any form elements, you can use a regular page callback to Drupal doesn't have to instantiate a form.
6. includes/media_watermark_edit.admin.inc:30 The docblock suggest a sortable table, but its a regular table. Although sorting is nice, its probably okay to update the documentation to regular table as well :)
7. includes/media_watermark_edit.admin.inc:101 Instead of coding html, you can use the l() function to create the markup for you. Strictly speaking, the padding on the link should be in a seperate stylesheet file and not hardcoded in the html.
8. includes/media_watermark_edit.admin.inc:113 The empty text never gets used, since you only render the table when there are already rows available. For usability, can you link the empty text to the 'add'-form, so you help users find the right page?
9. media_watermark.js Can you check this file for Drupal javascript coding standards?
10. media_watermark.js:10 The context variable is included in your attach function, but you don't use it in your selectors. Can you add the context when possible, for example $('.hide-select-list') becomes $('.hide-select-list', context). This makes your jquery selectors faster as it only has to search the context element instead of the entire DOM.

Keep up the great work, you are almost there :)

manual review:

1. media_watermark_prepare_images(): don't call theme() here, just build a render array and return that. Drupal will render stuff later for you. See https://drupal.org/node/930760 . Same in media_watermark_get_sort_table_watermarks(), no need to call theme() or render().
2. "'#default_value' => isset($values->fid) ? check_plain($values->fid) : '',": don't use check_plain() here, #default_value is already handled by the from API and double escaping is bad. Please read https://drupal.org/node/28984 again.
3. media_watermark_form_submit(): do not run check_plain() here. "When handling data, the golden rule is to store exactly what the user typed. When a user edits a post they created earlier, the form should contain the same things as it did when they first submitted it. This means that conversions are performed when content is output, not when saved to the database.". So you should use check_plain() in media_watermark_get_sort_table_watermarks() as I told you.

You have done a great job and very near to the goal.
I have used a fresh drual installation to test your module and it worked, but only with media upload option, drupal default image option did not worked for me and if this is the case then please chagne the description from
"Media watermark module allows to add watermark to image files (.jpg, .jpeg, .gif, .png) while they are uploading into CMS"
to
"Media watermark module allows to add watermark to image files (.jpg, .jpeg, .gif, .png) while they are uploading into CMS through media module".

Manual Review: media_watermarks.install file
1. line no 105, is blank line, it can be removed.
2. When we unistall module, entries created by code on line 96
   $file_temp = file_save_data($file_temp, 'public://watermark' . $value['file'], FILE_EXISTS_RENAME);
   are still there in database (in file_managed table). Following link may help you in deleting those data,
   https://drupal.org/comment/7295942#comment-7295942




Manual review for media_watermark_add.admin.inc

3. Just a suggestion, on line number 28, you can remove dot before png and gif.
4. On line number 79, you have used system_settings_form, it is used when we need to submit form automatically and is used with variable_get and variable_set, for more info please visit Drupal Api Page
   You just need to add Submit button at the end and you should remove line 76 and 77 where you have defined validate and submit function. Drupal automatically calls form_id_validate and form_id_submit when form is submitted, so your validate and submit functions will be called automatically as you have followed the correct naming convention for submit and validate function.
5. On line 95 and 102, in form set error, my suggestion will be to replace "20 character" by "20 digits" as you are expecting integer and it would be better to check for integer first and then for length.
6. Just a suggestion, on line 130 you have used
   file_usage_add($file, 'media_watermark', 'image', $user->uid);
   it would be better to use wid from your table instead of $user->uid, but not a road blocker.





7. In file media_watermark_delete.admin.inc on line 21, you dont need to define $form['#submit'], it will automatically submit to media_watermark_delete_form_submit() as I explained above.

Review of the 7.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/includes/media_watermark_add.admin.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   28 | WARNING | #description values usually have to run through t() for
      |         | translation
  --------------------------------------------------------------------------------
  

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.

But otherwise looks RTBC to me.

Assigning to kscheirer as he might have time to take a final look at this.

no objections for more than a week, so ...

Thanks for your contribution, Bogdan1988!

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.