[D7] simpletagcanvas

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

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.

Hi,
Found some issues:

1) You added dependencies in .info file and you added function_exists('libraries_get_path') in .module file. If there would be no module libraries your module will not be enabled

2) for file simpletagcanvas.mainconfig.inc it would be better to use the name simpletagcanvas.admin.inc.

3) in simpletagcanvas.mainconfig.inc
If we add the following code at the end of the form:

$form['#validate'][] = 'simpletagcanvas_mainconfig_form_validate';
return system_settings_form($form);

we will be able to delete submit, because all our variables will be saved automatically.

4) file simpletagcanvas.admin.link.inc
on line 69 - 74 - it is t() funtion with empty value in argument, but we are not sure what is it for and do we need it at all.

1) Delete variables in hook_uninstall().
Use variable_del() to delete all variables stored by your module.

2) Maintain content before the template file.
You may use a seperate function to place your php script there. In this way you can just print your variables like:

<?php 
if ($image_uri) {
  print $image_uri;
}
?>

Thus the template file is better readable.

3) $variables in simpletagcanvas_theme().
You declare 'content' as variable, but don't use it in the template file.
In addition to 2) you may update simpletagcanvas_theme() with the variables you need and provide them during impletagcanvas_block_view() and add them to '#variables'.

4) Attach js and libraries with '#attached'
Use '#attached' in simpletagcanvas_block_view() to attach js and libraries like you do with css. In this way simpletagcanvas_add_tagcanvas() and simpletagcanvas_preprocess_html() are obsolete. The '#attached'-attribut will invoke drupal_process_attached() during the render process. drupal_add_js() will be called during drupal_process_attached().

Automated review:
PAreview.sh on ventral.org: http://ventral.org/pareview/httpgitdrupalorgsandboxmedienverbinder196910... 0 problems found.
Coder module: OK, nothing found.

Manual review:
- Install a fresh drupal 7.22
- Move the module package to folder to sites/all/modules
- Install module
- Go to configuration page
- Configured some options.
- Save the settings.
- Entered links / uploaded images.
- Save the settings.
- Result as expected: The images just uploaded appear constellated as a sphere in the html5 canvas.
- Tested and played around with the settings: no problems found, no javascript errors detected.

No issues found in manual review and issues mentioned by all the previous reviewers seems to be fixed very well.

all works fine and the drupal coding conventions inclusive usage of integrated drupal functions were well implemented, so the application status should be "reviewed by the community"
there are only some console.log's in js/simpletagcanvas.js (line 15-19) that you should remove before any release.

wrong sign in git url

manual review:

1. simpletagcanvas.info: dependency to image module is missing.
2. "'access arguments' => array('administer simpletagcanvas'),": that permission is not defined with hook_permission()?
3. "define('DEFAULT_SIMPLETAGCANVAS_MOUSEWHEELZOOM', 'false');": all constants defined by your module need to be prefixed with your module's name to avoid name collisions.
4. simpletagcanvas_admin_form(): doc block is wrong, see http://drupal.org/node/1354#forms . Also elsewhere.
5. "'vcylinder' => 'The vertical cylinder',": all user facing text must run through t() for translation.
6. simpletagcanvas.admin.inc: @file doc block is useless, please provide a description. See https://drupal.org/node/1354#files
7. "drupal_set_message("SimpleTagCavas Item " . $i . " was deleted. (330)");": user facing text, must run through t(). Please check all your strings.
8. simpletagcanvas_block_content(): do not create link markup yourself, use l() instead. Same for the image markup, use theme('image', ...) instead.
9. simpletagcanvas_libraries_info() looks for jquery.tagcanvas.min.js, but I downloaded jquery.tagcanvas.js. Should be corrected in the README.txt
10. simpletagcanvas_link_form_validate(): "Please enter at least one character." is confusing if I enter <script>alert('XSS');</script>, which has more than one character. Why do you even need simpletagcanvas_validate_string() and the preg_match()? The link text should allow for any characters? See also https://drupal.org/node/28984
11. So an admin has to create all the tag links manually and cannot use for example a taxonomy vocabulary with terms? I think you should better describe the use case of your module and the differences to the existing tagcanvas module.
12. "variable_get('simpletagcanvas_storagearray'": so you are storing all links in one giant variable, which gets copied to memory on every single page request. That might not scale if you have many links.
13. simpletagcanvas.mainconfig.inc: this file is never used?
14. simpletagcanvas.tpl.php: this is vulnerable to XSS exploits. If I enter 2"><script>alert('XSS');</script><div height="5 as canvas width on the config page I get a nasty javascript popup when the block is displayed. You need to sanitize user provided text before printing, make sure to read https://drupal.org/node/28984 again.

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

Hello medienverbinder,

Delete line 262 of simpletagcanvas.admin.link.inc

About the user facing text, it'll be better if you use the t() function as follow (in simpletagcanvas.admin.link.inc) :

• LINE 281 : drupal_set_message(t('SimpleTagCanvas managed file @yourvariable was deleted.'), array('@yourvariable' => $returnvalue[0]));
• LINE 285 : drupal_set_message(t('SimpleTagCanvas managed file @submittedvars could not be deleted.', array('@submittedvars' => $submittedvars['file'])), 'error');
• Idem on LINE 290, 295, 303.

The rest of your code looks right to me and API's seem to be used properly.
Good job!

You could create your own DB table where you store the items that should be displayed. But then it feels like you are re-implementing the taxonomy term system ...

Anyway, looks RTBC to me now. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

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

Thanks for your contribution, medienverbinder!

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.