[D7] prettyPhoto Formatter

Hi Lona,

Your module seems to be an error and did not passed pareview test see this link

http://pareview.sh/pareview/httpgitdrupalorgsandboxlonalore2215387git

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.

@Lóna Lore, please correct the Git Clone URL on your project application.

As your working branch is 7.x-1.x, you should change it to:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/lonalore/2215387.git prettyphoto_formatter

Currently it refers to a master branch, which is not there anyway.

Hi Lona Lora,

Great module mate.
I reviewed your module and most of the things work like a charm... Great work.

Few suggestions
1. On each of the modules, I can see that a function call is made in hook_init to include js and css files. Is there any reason why this is not added directly to hook_init method? I don't see this function being called from anywhere else.

function prettyphoto_formatter_image_init() {
prettyphoto_formatter_image_add_files();
}

function prettyphoto_formatter_image_add_files() {
$path = drupal_get_path('module', 'prettyphoto_formatter_image');

if ($path) {
$js = $path . '/js/prettyphoto_formatter_image.js';
drupal_add_js($js, array('scope' => 'footer'));
}
}

Also, your current design includes css and js files on all the pages.
Wouldn't it be better to give users an option in admin where they can select pages where these css and js should be included (something like block display settings)

Good luck mate...

Cheers,
Ash

Hey Lona Lora,

That's great... Really appreciate your effort...

Well there is nothing wrong with your approach. The only drawback is that the css/js files will get included in pages which doesn't even use prettyphoto...
Anyway, keep up the great work...

Cheers,
Ash

Great work mate... Will review it this weekend...

Looks good to me!

It passes all tests of Coder Review, and works great. The code suggests that the author understands how the relevant parts of Drupal API works, so I support promoting this module to a full project.

Automated Review.

No issues.

Manual Review.

(+) You have a dependency on Libraries, but you aren't really using the Libraries API. See https://drupal.org/node/1342238

(+) Your JS behavior should use the context and settings that get passed in. I suspect you also want to use .once().

(+) You have an admin settings form that creates variables, but no hook_uninstall to delete them.

(+) Why the require_once and include_once in prettyphoto_formatter_requirements()?

(+) I got the warning message about the missing JS library from prettyphoto_formatter_requirements, but not seeing anything in the status report.
Are you failing the install if the JS isn't found immediately? If so, don't do that.

(+) You have a dependency on Libraries in the .info file, so you don't need to check that it is available in
prettyphoto_formatter_requirements(). This also pops up in the submodules.

Avoid using HTML in translated text. Build up the link and pass that in via the placeholder in prettyphoto_formatter_requirements().

(+) Avoid hook_init(). This gets called for every Drupal request, including non-HTML page requests. hook_page_build() is the best place,
and with that use #attached instead of drupal_add_css and drupal_add_js.

In prettyphoto_formatter_exclude_these_paths(), use current_path() or one of its friends.

In prettyphoto_formatter_add_files(), you should use drupal_static().

Avoid the file_exists() calls for the CSS and JS. They are overkill.

Commented out code should be remomved for release (eg, _prettyphoto_formatter_library_path line 125).

$items['admin/config/user-interface/prettyphoto-formatter'] can use drupal_get_form() for the callback instead of the
inbetween you are using.

Why the escaping in _prettyphoto_formatter_get_tpl()?

The .tpl.php files will end up with untranslated text in them.

You should document the fact that tempalate files are really used by the JS and not part of the normal rendering process.

Wrapping the whole admin form in a fieldset is likely unnecessary.

The wmode setting should use a select w/ the valid options. http://helpx.adobe.com/flash/kb/flash-object-embed-tag-attributes.html

Some of prettyphoto_formatter_settings_form_validate can be replaced with element level validation, like element_validate_number().

Are all of the field formatters cloned off of the ones from the modules?

prettyphoto_formatter_entityreference_menu() may be able to make use of object loaders. Look at the hook_menu docs for details.

In theme_prettyphoto_formatter_image(), don't use use the unstyled image path for the theme argument?

In theme_prettyphoto_formatter_link() and theme_prettyphoto_formatter_youtube(), use the third parameter of l() to set query variables.

prettyphoto_formatter_image_check_filefield_image_extension() isn't needed. Use the allowed extensions from the field instance itself.

Your config path should be in the .info. I also think putting it under admin/config/media is more logical.

(*) I was able to inject XSS by setting the alt text on an image to <script>alert('XSS1');</script> Add this to an image field,
set the alt and text to the snippet above, then open the popup. You should see the alert. You have a lot of places where you
have user input that will make its way into output. All of the PHP paths look OK, but double check all of the JS, including your settings
form. Try things like <script>alert('XSS1');</script> and "><script>alert('XSS1');</script> in your settings, especially
where they goto HTML element attributes.

I am not seeing any third-party code.

As for module duplication, there are lots of lightbox modules. You may want to mention others on your project page, and
in the description above. However (and this is mainly for the admins), this isn't just another JS-to-libraries module, and there
are some nice features in this.

The security issue (*) mentioned above need to be addressed. This is a blocking issue. Please don't remove the security tag, we keep that for statistics and to show examples of security problems.

There are no major API issues, though the ones I marked with (+) are fairly big. If it weren't for the security issue, I
would be on the fence for pushing back to Needs Work for the ones I marked.

Sorry if there is some duplication here. This is a cross post with the other comment.

Automated review:
$ drush dcs .

FILE: ...ttyphoto_formatter/templates/prettyphoto_formatter_image_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...typhoto_formatter/templates/prettyphoto_formatter_inline_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...typhoto_formatter/templates/prettyphoto_formatter_iframe_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...hoto_formatter/templates/prettyphoto_formatter_quicktime_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...ttyphoto_formatter/templates/prettyphoto_formatter_flash_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...yphoto_formatter/templates/prettyphoto_formatter_gallery_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...ts/prettyphoto_formatter/templates/prettyphoto_formatter_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

FILE: ...typhoto_formatter/templates/prettyphoto_formatter_custom_markup.tpl.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
1 | WARNING | No PHP code was found in this file and short open tags are not
| | allowed by this install of PHP. This file may be using short
| | open tags but PHP does not allow them.
--------------------------------------------------------------------------------

Manual review:
hook_init is removed in D8 in favor of hook_page_build when adding js and css to specific pages. Use of it in D7 is also encouraged.

function prettyphoto_formatter_add_files() {
  static $loaded = FALSE;
  if ($loaded) {
    return;
  }
  $loaded = TRUE;

  $library_path = _prettyphoto_formatter_library_path();
  $module_path = drupal_get_path('module', 'prettyphoto_formatter');

  if ($library_path) {
    $css = $library_path . '/css/prettyPhoto.css';
    if (file_exists($css)) {
      drupal_add_css($css);
    }

    $js = $library_path . '/js/jquery.prettyPhoto.js';
    if (file_exists($js)) {
      drupal_add_js($js);
    }
  }

Static loading and file_exists logic is already handled by drupal_add_css/drupal_add_js. No need to duplicate.

That logic is still needed for the settings though. Drupal will only add files once (the js array is keyed by the full path to the file to make absolutely sure of that).

if ($module_path) {
    $js = $module_path . '/js/prettyphoto_formatter.js';
    if (file_exists($js)) {
      drupal_add_js($js, array('scope' => 'footer'));
    }
  }

Under what conditions would the module not exist? I think there is no need to check for file_exists and the boolean check on $module_path.

libraries_get_path does all the heavy lifting. No need to wrap it in _prettyphoto_formatter_library_path.

Use of _prettyphoto_formatter_get_tpl() is really weird. In cases where you aren't passing in any variables to the theme, just give it the HTML. The lone exception I see to that is prettyphoto_formatter_social_tools(), which still makes sense to be a theme callback.

.info file:
Need to mention the configuration path for admin/config/user-interface/prettyphoto-formatter: https://drupal.org/node/542202#configure

.install file:

if (!drupal_get_path('module', 'libraries')) {
      $requirements['prettyphoto_formatter'] = array(
        'title' => $t('Libraries module missing'),
        'severity' => REQUIREMENT_ERROR,
        'value' => $t('Libraries module required for prettyPhoto Formatter'),
        'description' => $t('prettyPhoto Formatter module requires the <a href="@url">Libraries module</a> to be installed.', array('@url' => 'http://drupal.org/project/libraries')),
      );
    }

No need to put libraries in the hook_requirements. That happens automatically by listing in the .info file.
The whole hook_requirements looks rather funny. Please review the documentation again.

.js file:
The attach should pass in settings and context. Then they should be used. See https://drupal.org/node/756722#behaviors

prettyphoto_formatter_entityreference:
No need to call module_exists(). The .info file lists entityreference as a dependency. Same applies to image, link and youtube

image:
instead of array_pop & explode, use pathinfo()

youtube:
The theme function doesn't use underscores between words in variable names.

Sándor, I won't have a time to see the actual changes and review this until later, but filter_xss() is used when you know you need to output user-input that contains HTML. check_plain() should be used when plain text is expected, which is the normal use for alt/title text on images.

Sándor, I think the latest changes introduce a bug. The JS settings are on the page, but not js/prettyphoto_formatter.js. I can't test the XSS thing, though it looks like it was done properly for the titles. I do want to test the iframe attributes to make sure they are XSS safe. Try to set a height / width in the settings to get <script>alert('XSS');</script> in there by prematurely closing the iframe.

The rest of my (+) changes look good, as do the ones that looked important from @heddn's list.

The JS settings that your module are there, as is the library. But the .js file isn't loading, so nothing is working for me.

I have uninstalled, deleted the module, and recloned from git. I can configure the content type and choose the formatter (I'm testing w/ images). The page renders out, I see the class on the image, and it is now a link. When I click on the image, it just opens the JPG. When I do a view-source, prettyphoto_formatter.js isn't in any script element.

Sanitization in prettyphoto_formatter_image looks OK.

(*) Sanitization in prettyphoto_formatter_link() doesn't look right. The title for l() already gets plained inside the function. First, you should
sanitize what you need in the formatter view function and not the theme function. However, since you may output the URL directly,
you need to handle this in a different manner. Refer to the link.module field formatters, and check_url(). I think you just need
to drupal_strip_dangerous_protocols() the URL, and then l() will plain it for you, but I didn't fully trace all of your code out in
this regard. The check_plain() on the iframe options look OK (assuming you move to the formatter view).

Sanitization in prettyphoto_formatter_entityreference() looks OK.

(*) I am not seeing any sanitization in prettyphoto_formatter_youtube(), but the JS library builds this for you (it doesn't use your templates). I would
plain the textfields from the settings as a precaution in cause they get used as-is in the library.

Not using #attached isn't a blocker, though explicitly adding to a certain place isn't the best idea unless you really need to. In this case, though, you are
still adding the JS and library to every page. Upon further thought, doing this in the formatter view function may be better. Just
think about this long term.

Starred items (*) are blockers.

The last set of changes look good. Setting this as RTBC so someone else can take one more look at it. Since the final HTML assembly is done by the third-party library that you are using, how that gets done is a little opaque. I would prefer a second set of eyes on sanitization.

Review of the 7.x-1.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. _prettyphoto_formatter_requirements_library_installed(): @return data type is wrong, this should be "string|false" instead of mixed, see https://drupal.org/node/1354#types
2. prettyphoto_formatter_page_build(): this is a bit excessive to add the JS on every page? Shouldn't you only add it if an actual link of your formatter is displayed? Please add a comment.
3. prettyphoto_formatter_page_build(): you are still using drupal_add_js() instead of #attached. You have a render array at hand, namely $page, so you should use #attached in it.
4. prettyphoto_formatter_exclude_these_paths(): @return is misleading, this function does not return booleans? I think you should fix your code to explicitly use TRUE and FALSE instead of 0 and 1.
5. prettyphoto_formatter_settings_form_validate(): why do you have to check $form_state['values']['op']? Is there a second submit button? Please add a comment or remove the if statement.
6. prettyphoto_formatter_flash_markup.tpl.php: do the {width} placeholders work? I thought you need to have print statements in phptemplate files to insert the variables?
7. prettyphoto_formatter_entityreference_menu_callback(): do not call drupal_render() here, you can just insert your div wrapper as #prefix and #suffix on it, right?

But that are not critical application blockers, so ...

Thanks for your contribution, Lóna Lore!

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.

And of course I forgot to attach the file.