Make baidu_map module pass Coder Review

Glad this module is still in Sandbox because bringing it back in compliance with standards really required a major change:

• README.txt is missing, see the guidelines for in-project documentation. Added at 1b17f16.
• ./geofield_baidu_map/includes/geofield_baidu_map.views.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  

  <br>
  function geofield_baidu_map_views_plugins() {<br>
  

• ./geofield_baidu_map/plugins/geocoder_handler/baidu.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  

  <br>
  function geocoder_baidu($address, $options = array()) {<br>
  function geocoder_baidu_field($field, $field_item, $options = array()) {<br>
  function geocoder_baidu_form($default_values = array()) {<br>
  

• ./geofield_baidu_map/geofield_baidu_map.install: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  

  <br>
  function geofield_baidu_map_uninstall() {<br>
  

• ./geofield_baidu_map/geofield_baidu_map.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
  
  

  <br>
  function geofield_baidu_map_views_api() {<br>
  function geofield_baidu_map_ctools_plugin_api() {<br>
  function geofield_baidu_map_ctools_plugin_directory($module, $plugin) {<br>
  function geofield_baidu_map_field_formatter_info() {<br>
  function geofield_baidu_map_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) {<br>
  function geofield_baidu_map_field_formatter_settings_form($field, $instance, $view_mode, $form, &amp;$form_state) {<br>
  function geofield_baidu_map_field_formatter_settings_summary($field, $instance, $view_mode) {<br>
  function geofield_baidu_map_settings_form($settings, $element = array()) {<br>
  function geofield_baidu_map_settings_do($settings) {<br>
  function geofield_baidu_map_form_baidu_map_settings_alter(&amp;$form, &amp;$form_state, $form_id) {<br>
  function geocoder_widget_parse_addressfield_china($field_item) {<br>
  function geofield_baidu_map_theme() {<br>
  

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  
  

  <p></p>
  

  baidu_map.install<br>
  baidu_map.variable.inc<br>
  geofield_baidu_map/includes/geofield_baidu_map.views.inc<br>
  geofield_baidu_map/includes/geofield_baidu_map_plugin_style_map.inc<br>
  geofield_baidu_map/geofield_baidu_map.install<br>
  

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives.
  
  

  <p></p>
  <p>FILE: ...l-7-pareview/pareview_temp/geofield_baidu_map/geofield_baidu_map.module<br>
  --------------------------------------------------------------------------------<br>
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)<br>
  --------------------------------------------------------------------------------<br>
   370 | WARNING | All variables defined by your module must be prefixed with<br>
       |         | your module's name to avoid name collisions with others.<br>
       |         | Expected start with "geofield_baidu_map" but found<br>
       |         | "geocoder_baidu_delay"<br>
  --------------------------------------------------------------------------------</p>
  

  FILE: ...emp/geofield_baidu_map/includes/geofield_baidu_map_plugin_style_map.inc<br>
  --------------------------------------------------------------------------------<br>
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)<br>
  --------------------------------------------------------------------------------<br>
   45 | WARNING | Unused variable $container_id.<br>
  --------------------------------------------------------------------------------<br>
  

For the geofield_baidu_map entire module:

All functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming

So basically, the name of the module and all function names were not following the standards, as per #2085529-1: False positives on submodule hooks:

all sub-modules within one project should be prefixed with the main module name to avoid name clashes. If somebody creates a page_manager project there will be fatal errors because of ctools ...

In other words, to bring back module in compliance with Coding Standards, the sub-module geofield_baidu_map would have to be renamed to include the name of the main module.
Therefore, the sub-module was renamed: baidu_map_geofied to be prefixed with baidu_map, which is the name of the main module and all function/variables names had to be updated accordingly.

See also: Resolving automated errors: submodules.

This major change was committed against the 7.x-1.x branch at f82bd90.
 

Most of other issues (related with character encoding for line endings, white spaces, indents and several other issues) could be fixed properly and were committed against the 7.x-1.x branch at 2cb3b7e, a45a9a9 and b1d1c4d.
 

Now, the only remaining issues reported are:

FILE: ...l-7-pareview/pareview_temp/baidu_map_geofield/baidu_map_geofield.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 150 | ERROR | No key specified for array entry; first entry specifies key
--------------------------------------------------------------------------------

This error is caused by a key that's missing in an array to add JavaScript for the JS Settings.
It seems this is the exact same error discussed and reporting on the Project Application #1360332-3: QueryLoader2 integration where it didn't seem to be a problem for the Project Application to be accepted.
Unfortunately, currently, I was unable to find any proper way this issue could be resolved.
 

FILE: ...emp/baidu_map_geofield/includes/baidu_map_geofield_plugin_style_map.inc
--------------------------------------------------------------------------------
FOUND 7 ERROR(S) AFFECTING 4 LINE(S)
--------------------------------------------------------------------------------
 14 | ERROR | Class name must begin with a capital letter
 14 | ERROR | Class name must use UpperCamel naming without underscores
 18 | ERROR | Method name
    |       | "baidu_map_geofield_plugin_style_map::option_definition" is not
    |       | in lowerCamel format, it must not contain underscores
 18 | ERROR | No scope modifier specified for function "option_definition"
 42 | ERROR | Method name "baidu_map_geofield_plugin_style_map::options_form"
    |       | is not in lowerCamel format, it must not contain underscores
 42 | ERROR | No scope modifier specified for function "options_form"
 96 | ERROR | No scope modifier specified for function "render"
--------------------------------------------------------------------------------

This is the same problem as reported at #2035305-2: [D7] Views App Folders and it would seem these issues would be minor coding standard errors which are not application blockers (see #2035305-3: [D7] Views App Folders).
Unfortunately, currently, I was unable to find any proper way this issue could be resolved.
 

Since I believe we have gone as far as we could with Coding Standards compliance, I allowed myself to mark this issue as fixed for now, but feel free to re-open it, or create a new ticket, at any time if you have any further objections with this issue or any of the related commits (f82bd90 2cb3b7e, a45a9a9 and b1d1c4d - we would surely be happy to hear your feedback).

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the commits or this task in general, I would be glad to provide more information or explain in more details.

Thanks in advance to everyone for your testing, reviews, feedback and comments on this issue.
Cheers!

Following up with @klausi's comments in the project application at #2128839-11: [D7] Baidu Map:

1. "$settings['baidu_map_geofield_scrollwheel'] ? 'Yes' : 'No')": yes and no should also run through t() for translation. Same for "'#markup' => 'Please add at least 1 geofield to the view',", please check all your strings.
2. GeoJSON.js 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.
3. baidu_map_geofield_plugin_style_map.inc: render() looks vulnerable to XSS exploits. $style_options['alt_text'] is user provided text and printed to HTML without sanitization. Make sure to read https://drupal.org/node/28984 again. This should at least use filter_xss_admin() or similar. This is not a security issue according to our policy since an attacker would need the "administer views" permission, but this should be fixed anyway. Same for the width and height settings.

Answers:
1. Added calls to t() for the strings mentioned: Yes/No and for the #markup in Views (see related commit below for more information).
I also went through all the strings of the module again to double check if there were any more strings that would potentially be missing a call to a t function and I was unable to find any more.
2. To be discussed with @klausi, but this is not third party code and was mostly written/contributed by DYdave, see #2127815: Support for GeoPHP Geometries and all Geofield Widgets
3. Replaced the returned aggregated string with a call to format_string, which should already sanitize replaced values for @variable, in this case: @width, @height, @container_id and @alt_text - They should all be plain text to be replaced in the returned HTML string (see related commit below for more information).

I went ahead and committed the changes against the 7.x-1.x branch at 27bba60.

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the latest commit or this task in general, I would be glad to provide more information or explain in more details.

Thanks in advance to everyone for your testing, reviews, feedback and comments on this issue.
Cheers!

Another round of reviews at #2128839-13: [D7] Baidu Map where @klausi caught another missing call to t() with the DrupalPractice sniffer module (interesting: I wasn't aware of this project):

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

  FILE: ...usi/pareview_temp/baidu_map_geofield/plugins/geocoder_handler/baidu.inc
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   164 | WARNING | #description values usually have to run through t() for
       |         | translation
  --------------------------------------------------------------------------------

Additionally, it was suggested to try to improve the @file Doc comment block in the GeoJSON.js file to let users know it is not third party code and it was actually adapted and inspired from the Geofield Map GeoJSON.js file.

I went ahead and committed the changes against the 7.x-1.x branch at ce4aa34.

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the latest commit or this task in general, I would be glad to provide more information or explain in more details.

Thanks in advance to everyone for your testing, reviews, feedback and comments on this issue.
Cheers!

Following up with @patrickd's comment at #2128839-16: [D7] Baidu Map:

baidu_map module: Looks good.. the only thing you might think about is to find a central place for the regular expression for checking the API key. I found it in 3 different files, that makes it a little ugly to adjust. A constant defined in the .module file would do the job.

which is was a very good suggestion, so a new constant variable was added BAIDU_MAP_API_KEY_VALIDATE_REGEX to be able to re-use the same regular expression in 3 different files.

I went ahead and committed the changes against the 7.x-1.x branch at 8d90970.

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the latest commit or this task in general, I would be glad to provide more information or explain in more details.

Thanks in advance to everyone for your testing, reviews, feedback and comments on this issue.
Cheers!