[D7] Wikiloc

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.

Hello jmaties,

Congrats on your 7 years on d.o !

I didn't know wikiloc before trying your module. So I tried your module and I must say it's working very well: I tried wikiloc ids with various field settings and also language detection (fr) and couldn't see any functional problem.

As for the code, it's clean, readable and commented, plus your corrected a lot of coding standards minor issues.

The field settings are already deleted from the field_config table by the field-api when the module is disabled (if none found).

The only problem I can think of is the git link you pasted in this thread, it should be:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/geekia/1970818.git wikiloc

Cheers

Hi jmaties,
It dont show markers on your map. while on real life map there are marker and flags probably put up by user. So i think so they should be on your drupal map too. Example there is a link http://es.wikiloc.com/wikiloc/view.do?id=133624 and min my drupal site it is coming like this image attached.

Hi jmaties,

Thanks for this module. It installed smoothly. Adding the field to a content type and adding a trail also worked fine, the markup shows on the map as configured. Uninstall worked also (after deleting the previously added field, as it should be).

The code looks also fine to me, quite straightforward and with inline comments where things get a bit more complicated (language selection in wikiloc_field_formatter_view(), e.g.).

I'm however not sure if it is ok to include a license link in wikiloc.module. (Including a LICENSE.txt is explicitly discouraged in https://drupal.org/node/1587704).

One additional suggestion re. the project page: Not everybody is familiar with Wikiloc (I wasn't - thanks for helping me discover this service!). It could be useful to add a sentence or two about the service itself and it's coverage, including keywords like "trail" or "mountain biking", which could help Drupal users to find your module. An image of the result of including a wikiloc field could be instructive, too. The fact that the service is available in different languages and that you take care of that could be worth mentioning also (the link to the Spanish site was a bit misleading to me at the first glance).

But this of course is completly to your discretion.

Thanks, and all the best - Joachim

I tested it on ubuntu chrome 29 and tested on two different drupal installation but couldn't able to see marker and flags. plz will you confirm it again on live demo or on browserstack. thanks

Testing on chrome 29 and Ubuntu 12.04.1 LTS, still not able to see maps and markers while on firefox 23.0 and with same platform, its working fine.Screenshot attached

Thanks jmaties, it could be my local browser problem. thanks for your contribution.

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2196361

Project 2: https://drupal.org/node/2200289

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: https://drupal.org/node/2196361

Project 2: https://drupal.org/node/2200289

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

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

wikiloc_field_schema() includes a lot of comments that aren't necessary when you list Implements hook_foo(). Below is sufficient to include additional details about your module's specific implementation of hook_field_schema().

/**
 * Implements hook_field_schema().

 * The data we will store here is just one 7-character element, even
 * though the widget presents the three portions separately.
 */

See also wikiloc_field_info() and wikiloc_field_widget_form() for similar feedback.

License details in .module file should go away.

function wikiloc_field_formatter_view($entity_type, $entity, $field, $instance, $langcode, $items, $display) 
          $wik_url_headers = @get_headers($wik_url);
          if (empty($wik_url_headers) or $wik_url_headers[0] == 'HTTP/1.1 404 Not Found') {

Consider using drupal_http_request().

              src="' . $wik_url . '/spatialArtifacts.do?event=view&id='
              . check_plain($wik_id)
              . check_plain($wik_measures)
              . check_plain($wik_near)
              . check_plain($wik_images)
              . '&maptype=' . check_plain($wik_maptype)
              . '" width="' . check_plain($wik_width)
              . '" height="' . check_plain($wik_height) . '"

Why don't you use url() to fabricate the URL? It already does a lot of the heavy lifting for crafting the URL securely and passes it through hook_url_outbound_alter (in case someone needs to adjust the URL).

Take a look at the doc for url(). No need to run checkplain and you should add the query parameters as an array.

This project is ready for review by a Git administrator. RTBC.

I've learned a few more things since I started doing reviews of projects a few months ago. So there are few items that should be addressed then this project is good to go.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder. There are findings at http://pareview.sh/pareview/httpgitdrupalorgsandboxgeekia1970818git

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

(+) No: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes. If "no", list security issues identified.

Coding style & Drupal API usage

1. (*) This update function assumes that the fields exist. And nothing actually creates those fields. There's a good write-up at https://www.drupal.org/node/691932 on the absence of the ability to alter field schema. Basically, you need to follow http://drupal.stackexchange.com/questions/30301/update-field-schema.
   wikiloc_update_7000

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This last time through the code I also noticed a strange comment structure in the @file definition. Open source is maintained and nurtured by a community. So putting in comments the author while fine for personal projects really shouldn't be used for a Drupal project. I'm not going to block RTBC on that, but it should be removed.

 * Geekia
 * @since 02-04-2013
 * @version 1.0
 */

Assigning to mpdonadio for another look through the code (if he has time) and to make a final call on the author comment.

Automated Review

Review of the 7.x-1.x branch (commit 13455ae):

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

  
  FILE: /home/matt/PAR/pareview_temp/wikiloc.install
  --------------------------------------------------------------------------------
  FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
  --------------------------------------------------------------------------------
   75 | WARNING | Unused variable $field_name.
  --------------------------------------------------------------------------------
  
  Time: 103ms; Memory: 5.25Mb
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

1. (*) wikiloc_field_formatter_view() is XSS prone. $wik_width and $wik_height go straight to output w/o any filtering. This is migigated by the fact that there are length limits on the fields, but these are alterable. I would use drupal_attributes() to build up
   the attribues, as it will check_plain() each of them. Or, you can use the #html_tag type for the $element render tree. See https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_at... and https://api.drupal.org/api/drupal/includes%21theme.inc/function/theme_ht...

Coding style & Drupal API usage

1. (+) The project page is pretty lean. You will need to improve this before you promote the module. Mirroring the README is a good start.
2. wikiloc_field_schema() should have a 'description' entry for each column, and for the table itself.
3. A Wikiloc block would be a nice addition in the future.
4. (+) wikiloc_field_formatter_view() should use the $langcode parameter instead of the global $language

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Automated Review

Review of the 7.x-1.x branch (commit 32af318):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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

(*) wikiloc_field_formatter_view() is XSS prone. $wik_width and $wik_height go straight to output w/o any filtering. This is migigated by the fact that there are length limits on the fields, but these are alterable. I would use drupal_attributes() to build up
the attribues, as it will check_plain() each of them.

This issue has addressed but still needs some correction. Why are you using check_plain explicitly for $wik_width and $wik_height variable as drupal_attributes() internally use this for each element, just remove it.

(+) You should fix this before stable release of this application.

 $wik_width = check_plain($item['width']);
 $wik_height = check_plain($item['height']); 

(+) wikiloc_field_formatter_view() should use the $langcode parameter instead of the global $language

This point has addressed and looks good now.

(+) The project page is pretty lean. You will need to improve this before you promote the module. Mirroring the README is a good start.

I still feel, Project Page can be extended little bit more, better to go through https://www.drupal.org/node/997024 again.

wikiloc_field_widget_error(): the switch() statement does not make sense here since you only have one case? Use if() instead?

if (!empty($item['id']) and is_numeric($item['id'])) {: IMHO, rather using and here for AND condition, better to use &&. See https://www.drupal.org/coding-standards#operators.

Other than module looks good to me after a manual review. I have also tested functionality of this module w.r.t XSS & other possibilities and it works fine as intended.

Inline comments and docs looks good.

Not seeing any further blockers... so moving to RTBC.

As I am not a git administrator, so I would recommend you, please help to review other project applications to get a review bonus. This will put you on the high priority list, then git administrators will take a look at your project right away :-)

Great Job jmaties!

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.

Non-blocking issues:

• Your .info file contains the clearest explanation of the module, consider using that on your module page
• In hook_schema() can width and height be integers instead?
• Instead of is_numeric(), which allows floats, octal, hex, &c, consider using ctype_digit()
• get_headers() will return FALSE on failure, there's no need to suppress error messages with @
• Instead of and/or, use && and || to follow Drupal coding standards
• Use single quotes when possible, like for "no" and "0"

Checked for security, licensing, Drupal API, individual account, performance and duplication, no other issues found.

Thanks for your contribution, jmaties!

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.

I am giving credits to the users who participated in this issue.