[D7] Image Factory

@jeffstric I tried your demonstration. the resizing is pretty straightforward.

however, I found some usability problems:

• the "cumstom" button doesn't seem to do anything (also there's a typo: it's "custom"). however, when I clicked on "resize image", it worked.
• the customize screen has an empty fieldset under the image.
• fields are missing some hints, for example: "Type the message you'd like displayed on the image" under the field "content".
• "size" should tell users in what size the text goes (I'm assuming it's text size, right? you should review this label). also use a suffix property, so that people will know whether it's pixels, points etc.
• "color" field should accept direct input. I tried to type "#FFCC00" but it didn't work. also, the button which inserts the selected color has no label, and I only found it after looking around the color dialog a bit.
• also "x" and "y" have no clue... I'm assuming it's the text position, but relative to the top left? bottom left? you should tell users.

finally, I'm not sure the fields should be glued to the frame like that. did you customize the CSS?

anyway I'll install the module in a couple of days and let you know if it works in my setup... in the meantime, review your code here: http://ventral.org.

good luck!

cheers,
Luciano

PS.: I like the cat Nicholas Cage :)

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

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 put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

ok, it's looking much better now.

I finally understood what the apply button does in the first step, where users can type width and height - now it's clear that it's related to the textfields, but you could make that fieldset collapsible just in case.

there's some spelling errors, and your text is somewhat technical. try changing "color must be string with six character" to "please use six letters or numbers to describe your colour", for example.

also it'd be nice (if possible) if the input color field accepted uppercase letters and strings with three characters, like FFF which is the same as ffffff.

there's not a measure for text size yet (pt, px etc).

and though it's more of a look and feel type of thing, I think it'd look better if you checked the layout. some elements are glued to the margins. please see the attached image.

@jeffstric, I'm pretty sure reviewers will tell you to fix those errors.

you must follow Drupal coding standards because that's how other people will be able to understand your code in the future (and how you will understand theirs).

but your module works just fine! fix the issues in the automated review and don't give up! :)

good job! I saw how big that list was before. I think you can change to needs review; I'll do that for you.

please note, it could be a couple of days until official reviewers go over your module.

meanwhile, you might want to consider evaluating other developers' code as per the PAReview bonus program.

please read the directions there, choose other projects that you feel comfortable reviewing, and help fellow developers turn their sandbox projects into full-scale modules. that'll help speed up your own review process.

cheers

1. imageFactory.info:
- What about capitalizing the first letter of the module name, it would look better in Drupal Module list:
name = Smarter image factory

2. README.txt: the file looks like a mess now:)
- extra lines:

file_managed

table.

- wrong comma placement:
resize ,add

- first letter should be capitalized:

module url
    http://drupal.org/sandbox/jeffstric/1937132

3. imageFactory.module:
- imageFactory_menu(): use t() for titles and descriptions.

Recommendations

• Include capitalize letters where necessary.
• Change fonts manage to Manage Fonts.
• Creative configure page that allows the user to access the factory administration pages easier.
• Give image selector a grey tint because when a user uploads a larger file with a white background it is very hard to notice the image selector.
• When you upload a font to the system the 'upload fonts' and 'back to image factory' links appear twice; once above the main content and once below.
• There is no option to add text onto the image.
• Remove 'apply width and height to drag area' h1 and just put an Apply button on the upload image screen.
• Once you have uploaded an image, there is no option to remove it.
• Give users the option to edit the image name.

a more other review

• Why imageFactory? image_factory is better
• Please test your module with coder
• Please use t() function where possible
• If I send a font it works fine, but if I send an image I get this error: The file could not be uploaded, because the destination /home/paolo/public_html//sites/default/files/imageFactory/upload/2013/04/01779 is invalid

Please use t() function where possible
I'm not sure where i can use t in other place.I try t in menu hook,but this cause a error in review tools.Can you give me some suggests?

No need to use t() when you implement hook_menu...
Otherwise you can use t() format feature, for example
drupal_set_message(t('Delete image: !file success', array( '!file' => $file->filename)));

file image_factory_form.inc line 222

Some suggestion about usability. I think that if you using input type number or dropdown for size, size, angle will be better than now, also may be some predefined values will be good idea too. Other the color selecting tool doesn't work, i suppose when clicking and choising a color should be populated.

You should have to use t() instead pure messages.

Great idea for a module! I think it will be very useful for in-place photo editing as opposed to having to use a separate application.
Here are some more usability suggestions:

• I agree with dimitrov.adrian, a drop down list with suggested font sizes (point sizes) in various intervals (e.g. 8,12,14,18,24,36,72,other) would allow the users to pick a predefined size. This is important because not all users know what commonly used text sizes are. In addition to the drop down you could enable font size by user input. If a user selects the "other" option you could show an input field or use something like a combo box field.
• The color picker was still confusing to me even though I am a graphic designer and work with this tool everyday. I thought that I could use my crosshair cursor to select the colors until I figured out I had to drag the circle around. The circle was kind of hidden in a corner and hard to see. Maybe you could specify a default color that would move that selection circle to the middle of the color picker field so the tool is more intuitive.
• Please consider adding cancel buttons to the image and font upload forms. There was no way to opt out of that process if I decided to go back to the list without uploading anything.

Forgot to set this to needs work.

hi @nsuit, I aggree with your suggestions for @jeffstric, except that a combo box field might be a little too much. we, beginner module developers, have enough on our plate as it is ;) but I would ask him to consider this as a future feature.

@jeffstric, maybe you could add a help page explaining users how to work the color picker?

also, I took the liberty of reviewing the text on your project page. here you go:

About

The Image factory module can be used to resize images on the fly and add infinite captions with custom fonts. It saves pictures in the public files path and adds records to the file_managed table.

Usage

First time
Upload ttf font files [in sites/all/files/fonts etc.] <- please provide the path.

Temporary pictures

When you create pictures using Image factory, it saves temporary files in public://imagefactory/tmp. There's a cron job which automatically removes them.

How to use images you create

Try the FileField Sources module, it has a Reference existing function.

sorry for my absence... I've been busy working on my module :)

cheers

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

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

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. colorpicker appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
2. "variable_set('image_factory', time());": all variables defined by your module need to be removed in hook_uninstall().
3. image_factory_font_del(): this is vulnerable to CSRF exploits. You must not execute destructive actions directly on GET requests, use confirmations forms or link tokens instead. See also http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . This is a security blocker. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
4. image_factory_font_del(): There is no ":" placeholder for t(), use "@" or "%" instead.

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

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

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

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

  
  FILE: /home/klausi/pareview_temp/fonts_manage.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   91 | WARNING | Do not use the raw $form_state['input'], use
      |         | $form_state['values'] instead where possible
   95 | WARNING | Do not use the raw $form_state['input'], use
      |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/image_factory_form.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 8 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
   108 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   111 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   120 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   121 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   121 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   129 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   160 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   161 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  Time: 0 seconds, Memory: 8.50Mb
  

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. image_factory_fonts_delete_confirm(): module files are always loaded at a fully bootstrapped Drupal, so no need for drupal_load() here.
2. image_factory_fonts_image_create(): why are you writing to $_POST? That looks very wrong. Why can't you pass the settings along to image_factory_fonts_image_create() as usual function parameter?
3. image_factory_save(): use drupal_json_output() to return JSON stuff with the proper HTTP headers.
4. image_factory_save(): this is vulnerable to CSRF exploits. You are just taking values out of $_POST without verifying any token to check that the user made that request willingly.
5. image_factory_resize.tpl.php: do not write form elements yourself, use the drupal form API to build forms, that will also solve your security problem from my previous point. See http://api.drupal.org/api/drupal/includes!ajax.inc/group/ajax/7 . The form and ajax API are pretty powerful and you should be able to solve your problems without using any $_POST in your module. That will also clear up your security problems.
6. image_factory_resize.tpl.php: do not hard code JS into templates! Always use drupal_add_js() or #attached on form arrays to properly inject the Javascript.
7. "drupal_set_message('Remove directory:' . image_factory_get_dir($v, TRUE, FALSE) . ' fail.', 'warning');": all user facing text must run through t() for translation. Please check all your strings.

This is a weird default value: $clear_time_last = variable_get('image_factory_cron_time', 'sites/default/files'); shouldn't the default be a time() or 0 instead?

In image_factory_image_list() you have 2 queries using like - db_query() thinks % is an escape character, so you need to use %% instead.

In image_factory_image_edit_form() you can use range() and drupal_map_assoc() functions to generate the option list in a cleaner manner.

You could probably put all your drupal include files in a single includes/ directory if that's easier. And some cleanup like using drupal_add_js() instead of putting js directly into your template file is a good idea.

But none of these are blockers, looks like a useful module.

----
Top Shelf Modules - Enterprise modules from the community for the community.

Sorry for the delay. Make sure to review more project applications and get a new review bonus and this will get finished faster.

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

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

  
  FILE: /home/klausi/pareview_temp/includes/fonts/fonts_manage.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
  --------------------------------------------------------------------------------
   123 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   124 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/includes/form/image_factory_form.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 8 WARNING(S) AFFECTING 7 LINE(S)
  --------------------------------------------------------------------------------
   108 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   111 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   120 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   121 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   121 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   129 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   160 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
   161 | WARNING | Do not use the raw $form_state['input'], use
       |         | $form_state['values'] instead where possible
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/includes/lib/image_resize.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   8 | WARNING | Class name must be prefixed with the project name
     |         | "image_factory" (omitting underscores)
  --------------------------------------------------------------------------------
  

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. theme_image_factory_file_formatter_table(): do not create image markup yourself, use theme('image', ...) instead. Same for theme('item_list', ...).
2. theme_image_factory_file_formatter_table(): that function should be registered in hook_theme(), no?
3. image_factory_path_public_to_ser(): why do you need that function? PHP stream wrappers can handle public:// just fine as file URI, no? Same for image_factory_path_ser_to_web() and possibly others.
4. image_factory_image_resize_validate(): hook_validate() does not exist, see https://drupal.org/node/1354#forms on how to document forms.
5. image_factory_resize.tpl.php: do not embed javascript in templates, use dedicated JS files for that. And the JS should use Drupal.behaviors, see https://drupal.org/node/756722

Usage of $form_state['input'] is almost a blocker, but I cannot see how it could be exploited in this case, so it is not a security issue. Please fix it anyway.

Thanks for your contribution, jeffstric!

I updated your account to let you 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 get 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.

yaaay! congrats!