Overview

This module actually comes as a helper, to assist developers who wish to create
a custom form with image uploads preview, using the form API.
It is more suitable for the examples project and really after understanding
the code you may snippet to your module the relevant parts.

If you ever wanted to create a image upload field with an image preview on a custom form using the form api, this module may just be the thing you need !
Simple and easy to understand how you can achieve an image preview.

Drupal 7 only.

Features

This module gives you a handicap to set a default image preview for your forms if you wish to use the module's theme process.
- More than that, it will guide you through if you wish to make different image preview for diversion of fields.

Requirements

Image module - Drupal 7 Core.
The module itself has no particular PHP, database, or Drupal requirements, other than it only works with Drupal 7.x.

Configuration

(optional)
Just go to the Form API Image Field settings, under config/user-interface.
- Choose the image style you wish to use for previews.

All you need to do to use this module is to add a managed_file field and apply
the #theme attribute value "fapi_image_preview"

Example (Minimal):

  $form['imagefield'] = array(
    '#type' => 'managed_file',
    '#upload_location' => 'public://YOUR_PATH/',
    '#theme' => 'fapi_image_preview'
  );

More information on the Project Page

Git access:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/ohadsh535/1859942.git fapi_image_field

CommentFileSizeAuthor
#2 Capture.JPG46.52 KBohadsh535

Comments

anwar_max’s picture

anwar_max
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +PAreview: security

README.txt
Check.

project page
Check.

Master Branch
Nope, not using it.

Major coding standards / best practice issues
Automated Review: Everything is fine.
Manual Review: Use proper sanitization function at line 115 in fapi_imagefield.module file

License
Not present, Check!

access administration vs. administer site configuration
Permissions make sense.

PAReview: 3rd party code
No 3rd party stuff here.

Multiple Applications
Yes, you have two applications please put one application on hold.

Already Approved
Nope, but I bet you will be soon.

Idle Application
Nope, you're all set.

Code too short
Nope, you're all set.

ohadsh535’s picture

ohadsh535 commented
StatusFileSize
new Capture.JPG46.52 KB

Hey anwar_max,

Thanks a lot for your insights.

I have a couple of clarifications.
On line 115 on fapi_imagefield.module file, I retrieve the style configured on the settings.
It is basically already sanitized, due to the demands of new style creation (see attached image).
And even if you set manually using variable_set(), just trying malfunction this module, it will always fallback to "thumbnail".
- This case, was resolved in case you've created a custom style -> configured to use it -> then, deleted it.

My questions are:

  • Is it really necessary to sanitize again ?
  • How do I suspend / put on hold a project of mine ?
ohadsh535’s picture

ohadsh535 commented
Status: Needs work » Needs review
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

There's an article about Why Drupal filters on output that might be helpful.

I think another perspective on this is that we've had a few cases where a new module came out and the developers said "There's validation on this field so no need to sanitize it." or "This field is defined by hooks in other modules and then inserted into the database so it's a code level item already and we don't need to sanitize it." Then time goes by and all of a sudden there are new ways of adding that data, a GUI appears (maybe as a separate module) and the permissions to use the GUI are not that strict and maybe the data is imported from an RSS stream off of twitter and...now...whoops there's an XSS.

It's probably not a requirement to do filtering, but a very strong suggestion. Basically, you should only not do filtering if it somehow breaks your module (and even then, maybe consider how to make the module work with the filtering).

Jordan_Fei’s picture

Jordan_Fei commented

Your code looks pretty good. Since not to much code lines, just one comment here for you:
As a module which would be used by others, the example should not be included in the fapi_imagefield.module file. Especially the code lines in the hook_menu as below:
$items['fapi-imagefield/example'] = array(
'title' => 'Form API Image Field Example',
'page callback' => 'drupal_get_form',
'page arguments' => array('fapi_imagefield_example_form'),
'access callback' => TRUE,
'description' => 'An example of fapi imagefield usage',
'type' => MENU_NORMAL_ITEM,
);
You can write an indendent example module, I think. Or just put your example in the Readme.

ohadsh535’s picture

ohadsh535 commented
Issue tags: -PAreview: security

Actually it did came to consideration to deprecate it to an indent example module. (- And even add some more examples: Such as One Click Upload).

But it's such a small module and it really exist as a tutorial for developers who want to get an image preview on custom forms.
- In my opinion, it should be a part of bigger examples project.

BTW I find the example and it's documentation necessary for the jump start user.

I am no expert, and I really rather that someone will enlighten me regarding such cases.

  1. Should I move the examples to an indent module ?
  2. Should I move the settings to an admin.inc file ?
  3. Should I just apply this as a contribution to a different project ?
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1859944
Project 2: http://drupal.org/node/1858950

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.

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 I'll take a look at your project right away :-)

Aleksey Zubko’s picture

Aleksey Zubko commented

Hello fellas.

The code looks neat and transperent.

Here are my comments about the stuff.

  1. Thumbnails must be in a list, not in radio boxes.
  2. There is no default thumbnail selected after install. It sets in 115-117 lines in .module file, but seems not working at all. I think the best place to set a default style is hook_install instead. You can get a list of all image styles and chose the first one.
  3. fapi_imagefield_permission() - index of a permission item must be lowercase.
  4. Best practice is to keep all theme functions in a separate files (use 'file' to do that).
  5. And I really think you need to theme an image styles as a drop-down list not as radio boxes in case if a user have a lot of them.

Thanks.

cloudbull’s picture

cloudbull commented

Hello fellas.

The code looks good. Just some comment below

1. Better example with code preview with multiple options.
2. I see there is hook_uninstall but better with default value.
3. Radio button will just accept 1 selection, it is better to support multiple options.

thanks
Keith

ain’s picture

ain commented

Automated review

All clean, no problems.

Manual review

  1. The code looks cool, everything seems clean and nice
  2. theme_fapi_image_preview() @param/@return not documented.

Recommendations

  1. Better inline documentation with @see or Does something for another_function().
  2. Please don't use nested ternary operators, it's bad readability, e.g. theme_fapi_image_preview() ln 116-117.

Generally I think the project is RTBC, please consider doing 3 reviews of other modules to get a Review bonus.

ain’s picture

ain commented
Status: Needs review » Needs work

In respect to the last review, setting status to needs work.

PA robot’s picture

PA robot commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

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