Module for Drupal 7.x.
Project page: https://drupal.org/sandbox/xiukunzhou/2126837
GIT info:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/xiukun.zhou/2126837.git baidu_map

Browse the repository: http://drupalcode.org/sandbox/xiukun.zhou/2126837.git
 

Module Description:

It is sometimes said to be more accurate than the Google Maps in China, more complete than Ali Maps, the Baidu Map module allows geographic and location information to be displayed through the Baidu Map API.
See a live example of Baidu Maps or the map control screenshot.

In short, this module allows to obtain geographic coordinates from textual addresses in China (Geocode) and display any geographic information through Baidu Maps.

Capabilities of the module...

Read more on module's page.

Already went through PAReview.sh validation: Automated Review on PAReview, see also related issue in module's tracker: #2128019: Make baidu_map module pass Coder Review.

The only PAReview errors left that couldn't necessarily be fixed are the ones related with an array key to add JS settings and Views Plugin integration:

 
This module actually integrates with the Baidu Map Third Party API Chinese service which requires to apply for a key and I would assume it wouldn't necessarily be practical for Reviewers/Testers (especially given that everything is in Chinese), so I would definitely be very happy to provide any official reviewers with a testing key that I have already registered.
Since I wish I could make reviewers/testers' life simpler and perhaps save official reviewer's time, I would like to kindly invite you to contact me directly if you would be interested in reviewing/testing this module and would like to use a Testing Baidu Map API Key.

Additionally, I certainly invite you to take a closer look at all commits that have been documented in tracked issues.

Please let me know if you would have any questions on any points/code/aspects mentioned in the project, I would surely be glad to provide more information.

I would greatly appreciate if you could take a bit of time to take a look at the code and give me your feedback on the module.

Lastly, I wanted to extend my greatest thanks to my Mentor @DYdave for taking me through the entire Project Application process and greatly helping me to prepare this module's application.

Thanks in advance to everyone for your testing, reviews, feedback, comments, suggestions or questions on this project application.

Manual reviews of other projects:

[Round 1]

  1. [D7] Node Update Fields: https://drupal.org/comment/8225139#comment-8225139
  2. [D7] Secure form: https://drupal.org/comment/8211771#comment-8211771
  3. [D7] Email Field Verification: https://drupal.org/comment/8225169#comment-8225169

[Round 2]

  1. [D7] Anonymous Date Reminder: https://drupal.org/comment/8231475#comment-8231475
  2. [D7] Jquery Clocks: https://drupal.org/comment/8231495#comment-8231495
  3. [D7] Commerce Attributes Date: https://drupal.org/comment/8231511#comment-8231511

Comments

rogical’s picture

rogical CreditAttribution: rogical commented

Good module! Baidu map is very popular in China, I tested it works great.

Just a small issue, if I haven't set up the api key, but I added the address field, with geofield, the geocoder be baidu geocoder, then the geocoder will be failed.

So, can we add a validation, if the api key isn't added, then we not allow users to choose baidu geocoder.

Thanks!

softescu’s picture

softescu CreditAttribution: softescu commented

Fixed issues from pareview.sh

http://pareview.sh/pareview/httpgitdrupalorgsandboxxiukunzhou2126837git

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
--------------------------------------------------------------------------------

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"
--------------------------------------------------------------------------------

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented

Hi @rogical,

Thanks a lot for your kind feedback, testing and reporting, it's greatly appreciated.

I just wanted to give you a quick update on the issue you reported and let you know it has been followed-up at #2133779: Add validation for the Geofield field settings to select the Baidu Geocoder.
It should have been fixed and changes committed at 69b8d65.

Feel free to take another closer look and let us know if you would still encounter any issues with this item.
Thanks again very much for your useful feedback.

Hi @softescu,

I would greatly appreciate if you could please read more carefully the summary of this post, in particular the part that says:

Already went through PAReview.sh validation: Automated Review on PAReview, see also related issue in module's tracker: #2128019: Make baidu_map module pass Coder Review.

We already know about these issues reported by PAReview, couldn't get them fixed and explained exactly why.
Additionally, after doing a little bit more research we found the exact same issues in other project applications that managed getting approved.
In other words, these errors reported should not be considered as blockers for this application.
 

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the latest code changes or this project application 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!

asghar’s picture

asghar CreditAttribution: asghar commented

Hi

I have found some issues when i passed your module into coder

Line 103: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs) [security_3]

drupal_set_message(t('Baidu Geocoder returned successfully, but failed to find the corresponding geo-coordinates for the address provided.
No geocoded information could be saved because no coordinates were found for the address: !address.
Please check the address provided and try again.', array('!address' => $address)), 'error');

severity: criticalreview: security_3Line 109: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs) [security_3]

drupal_set_message(t('The returned Geocoded information was not precise for the address: !address.
Please check the address provided and try again.', array('!address' => $address)), 'error');

asghar’s picture

asghar CreditAttribution: asghar commented
Status: Needs review » Needs work
xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Hi @asghar,

Thank you very much for your review and reporting these issues detected by the Coder module.

Quick follow-up on this issue to let you know the problems reported have been fixed at ce255f7, where I actually just modified the placeholders !address to @address, since in any case, it should be a plain text string.

Feel free to let me know if you would encounter any other issues reported by Coder or other Code Review/Analysis modules, I would surely be glad to take a closer look.

Alright, this ticket has been in review for two weeks already and I assume it would be a good time to start using my Review Bonuses:
Modified issue summary to add links to the reviews and added the PAReview: review bonus tag to this issue.
 

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on the latest code changes or this project application 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!

SuzhouKada’s picture

SuzhouKada CreditAttribution: SuzhouKada commented

Review report:

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
--------------------------------------------------------------------------------

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"
--------------------------------------------------------------------------------

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented

@SuzhouKada,

Thanks for your comment, but I have already replied to that previously:
Please look at the answer to @softescu at #3, the issue summary and #2128019: Make baidu_map module pass Coder Review.

I'm wondering how many users are going to come up with this exact same comment without reading any of the previous answers ....
 

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

Thank

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue summary: View changes
Issue tags: -PAreview: review bonus

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented
Issue summary: View changes
Issue tags: +PAreview: review bonus

Hi @klausi,

Thank you very much for taking some time to check this project application and letting me know about the validity of the reviews I carried.

I wasn't fully aware I had to also carry Manual Reviews, so I went back in project applications to look manually in code for some rather common mistakes.

Since I have collected three new manual reviews, I allowed myself to update the issue summary once again and added the PAReview: review bonus tag.
 

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on this comment or project application 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!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Needs work
Issue tags: -PAreview: review bonus

manual review:

  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.

Otherwise looks pretty good and almost ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: +PAreview: review bonus

Hi @klausi,

Thank you very much for taking the time to review manually the module and getting back to me with very valuable comments.

I was glad to hear you found the module "pretty good and almost ready", since @DYdave and I did a lot of work up-front to prepare this project application and try to make it as clear, simple and straight forward to review to try to save as much of your time as possible.

I have updated the module (committed at 27bba60) to take into account all of your comments (more information at: #2128019-3: Make baidu_map module pass Coder Review), except 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.

I'm not sure exactly what could have made you think that the file GeoJSON.js could be 3rd party code....
Well in fact, it's not third party code: it was actually written and contributed by @DYdave in the ticket #2127815: Support for GeoPHP Geometries and all Geofield Widgets.
It was greatly inspired and adapted from the code of the file GeoJSON.js from the Geofield module, to support GeoPHP Geometries with Baidu Map's API.

All of this code was written by @DYdave and I and there was absolutely no third party involved.

Feel free to let me know if you would still have any issues, comments or questions on this file, I would certainly be glad to provide more information.
 

As an attempt to keep things moving towards hopefully getting this project application approved, I have added 3 more manual reviews to the issue summary, under [Round 2].
I allowed myself to add the tag PAReview: review bonus again to this issue and switched back the status to needs review.

Please let me know if you would have any further comments, feedback, questions, issues, objections, suggestions or concerns on this comment or project application 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!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Assigned: Unassigned » patrickd
Status: Needs review » Reviewed & tested by the community

Review of the 7.x-1.x branch:

  • 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
--------------------------------------------------------------------------------

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.

Well, the comment on the GeoJSON.js file suggested that this was third-party provided code. Please add to the file comment that this is a modified version of the original code.

So this looks RTBC to me now.

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

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented

Hi @klausi,

Thanks again very much for your prompt feedback and especially for marking this application as RTBC!

I wasn't aware there was an automated script for checking best practices, so thanks a lot for catching this missing string to be wrapped in t().
Just a quick comment to let you know I have updated the comment in the file GeoJSON.js (@file Doc comment block at the top), as you suggested and committed all these changes at ce4aa34 (more information at #2128019-4: Make baidu_map module pass Coder Review).

Feel free to let me know if you would have any further comments, questions or issues on any aspects of this project application or module in general, I would surely be glad to provide more information.

Thanks again to everyone for your reviews, testing and feedback.
Cheers!

patrickd’s picture

patrickd CreditAttribution: patrickd commented

sorry for the delay, I'll take a look at this tonight!

patrickd’s picture

patrickd CreditAttribution: patrickd commented

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.

baidu_map_geofield:
I personally don't like the use of variable within strings ("value: $variable").. people tend to fuck things up when using them and its not always easy to read; I recommend using dots for concatenation in most cases. But that's nothing serious.

Code looks well documented as do the commit messages.

I like how you work, keep it up!

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:

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 for your contribution!

Thanks to the dedicated reviewer(s) as well.

rogical’s picture

rogical CreditAttribution: rogical commented

Congratulations!

Looking forward to the final release.

xiukun.zhou’s picture

xiukun.zhou CreditAttribution: xiukun.zhou commented

Hi @patrickd,

Thank you very much for your prompt follow-up and very helpful feedback.

I am greatly pleased to see you appreciated the work done on this project because my mentor and I actually spent an important amount of time, put a lot of work and efforts in this project and application.

Just a quick comment to let you know I have added a constant variable for the regular expression in the baidu_map.module, as you suggested and committed the changes at 8d90970.
I didn't have enough time yet to modify the strings in baidu_map_geofield.module ("value: $variable"), as you recommended, but hopefully I should be able to get back on this soon.

Thank you so much for granting me enough permissions to be able to create new "full" projects and especially to promote this one out of sandbox.
So I directly went ahead, promoted the module to the "full" project Baidu Map and created the first official release of the module: baidu_map-7.x-1.0 (what an exciting moment!!).

Lastly, I would like to express my gratitude to everyone (@rogical, @asghar, @klausi, @patrickd and all other reviewers/testers) for your time, feedbacks, testing and greatly helpful advice.
Tremendous thanks to my mentor @DYdave, once again, for sticking with me all the way through this project application.

I'm certainly looking forward to working more on the module and hearing everyone else's feedbacks in the issue queue.
Thanks in advance to everyone for your testing, reviews, feedback and comments on this module.
Cheers!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.