[D7] MyCharts module

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

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.

Installed the module on a clean Drupal-7.28 installation and found several things that need to be resolved:

1. Module uses functionality that is provided by the Entity module but Entity module is not listed as a dependency in the .info file.
2. Fatal error is thrown wheh a node of type "Chart" (provided by the module) is viewed: Failed opening required [files]. The page is loaded only halfway because of this.
3. Multiple errors are thrown regarding undefined variables when a node of type "Chart" is viewed.

Hi.

Please fix git link in description.

Change
git clone --branch 7.x-1.x git.drupal.org:sandbox/maris.abols/1952088.git
to
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/maris.abols/1952088.git mycharts

That is not an application blocker, please do a real review of the source code.

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

pages.inc:
theme_mycharts_settings_fields() -- Please don't call drupal_render within a theme function. That way the render array can still be modified in a page alter.

.info:
Maybe you should sanitize the .info file a little more...

name = "Wunderkraut Chart module"
description = "Complex module for any charting library"
package = "Wunderkraut"

I don't see a dependency on library api module, but there's a call to libraries_get_path() in several places. And while you are adding a dependency for libraries, don't forget to add a hook_requirements in your .install file.

Several places: I think that libraries API will also do the require_once for you, if configured correctly. At the very least, use module_load_include().

form.js:
Invalid number of arguments, expected 2 (at line 33)
Unnecessary semicolon at line 18

mycharts.views.inc
Duplicate array key (at line 30)

forms.inc:
Unused parameter $wrapper_id (at line 157)
Unused parameter $series_count (at line 185)

.module:
The hook_menu seems to be a little broad from a security perspective. What if a node isn't supposed to be viewable? This will allow chart data from it to appear. I've tentatively added PAReview: security tag. At the very least, I feel that more documentation should be provided in the README and/or the project page if open security is the intended functionality. But probably, more attention should be given to the permissions model.

basepluginclass.inc:
http://www.php.net/manual/en/language.oop5.abstract.php
For functions that are really supposed to be implemented by a concrete class, mark the function as abstract and don't return data from them. I'm specifically looking at:

  public function getLoadJs()
  public function setChartWrapper($id)
  public function setAxisTitle()
  public function setXAxisTitle($title) {

Some of these may be duplicates of #13 above; I had the review 75% done last night before I could finish it up.

Manual Review

Master Branch

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

1. mycharts.css has rules that may interfere with other themes that are installed (eg, .clear, html.js fieldset).
These need to be properly scoped.

2. In colorpicker.js and form.js, your behaviors aren't using the context that have been provided.

3. In form.js, you probably want to assign the object with the attach property directly, instead of making
an empty object, and then adding the property.

4. You are calling module_load_include from the global context. See https://api.drupal.org/api/drupal/includes!module.inc/function/module_lo...

5. mycharts.basepluginclass.inc has an abstract class, but isn't in the files[] section.

6. mycharts_chart_details_form() has calls to drupal_add_js() and drupal_add_css(). As these are now gone from Drupal 8, you should
really use #attached. See https://drupal.org/node/2169605 This pops up a few other places in the module.

7. mycharts_chart_details_form:42, use field_get_items() instead of directly accessing the data. Since you have a
dependency on the Entity API, you could also use EntityMetadataWrapper. This also occurs in a few different places.

8. In mycharts_chart_details_form() around line 36, you can use field_info_instances() instead of a for-in to get the fields.

9. mycharts_chart_details_form() adds colorpicker.js twice.

10. mycharts_chart_details_form_submit() is using $form_state['input'] instead of $form_state['values']. This data is unfiltered and considered unsafe.

11. I am having trouble totally tracing this out, but it looks like all of the form configuration is being output back
to the form on settings w/o any filtering. For example, _mycharts_form_config:214. Please don't remove the security tag,
we keep that for statistics and to show examples of security problems.

12. Can you use drupal_html_id() instead of mycharts_id_auto()?

13. mycharts.help.inc needs proper docblock w/ parameter and return values for your helper functions.

14. mycharts_buildobject needs proper docblock w/ parameter and return values for your helper functions. Can you
consolidate these two files?

15. _mycharts_load_yaml(), rather than hitting the filesystem each time, you could also use a class_exists() for Spyc

16. theme_mychart(), you can use module_load_include instead of an explicit require_once.

17. mycharts_is_charts_content_type() Use menu_get_object() instead of arg().

18. mycharts_ajax_load_engine_types(). Use drupal_json_output() to render a JSON page request.

ADDENDUM:

19. mycharts_ajax_load_engine_types(). Use drupal_exit() instead of exit() so all of the Drupal request cleanup can be done.

It isn't a release blocker, but hook_requirements isn't typically done at install but at runtime. See htmlpurifier as an example.
Also, libraries will do the require_once for you. See https://drupal.org/node/1342238, specifically libraries_load(). htmlpurifier has an example of that as well.

However, this is a release blocker:

_mycharts_form_config
...
      // LOCKED values can't be edited.
      if (isset($engine_config['locked']) && isset($engine_config['locked'][$setting_name])) {
        $element[$key] = array(
          '#type' => 'item',
          '#title' => ucfirst($key),
          '#markup' => !is_null($config_value) ? $config_value : $value,
          '#prefix' => '<div class="float-left">',
          '#suffix' => '</div>',
        );
        continue;
      }

If someone put nasty xss into a field value, this will simply get printed to the screen. Please run the data through some form of data sanitization. e.g. check_plain if this should be simple text values.

You have pretty thorough documentation in the readme.txt minus the module dependencies. Your project page should have a much better overview and at the very least indicate the dependent modules and libraries ie; Jquery_colorpicker, Tablefield. I would like to see what is required for usage, a more descriptive overview, and screenshots or link to demo.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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