The Social Geolocation module extends the Open Social distribution to add geolocation functionality for Groups, Events and Users. Additionally it provides a submodule that can display a map of the events that have been given a location.
This feature is released as a separate module because we recognize that not all users of the Open Social distribution may require this functionality. The module takes care of setting up the required configuration for Open Social's default content types and uses the geolocation module to perform the actual geocoding. It also integrates with search for both SQL and SOLR back-ends.

My application for the IMCE Plupload module was approved 7 years ago, but I seem not to have been given the vetted role.

Project link

https://www.drupal.org/project/social_geolocation/

Git instructions

git clone --branch 8.x-1.x https://git.drupalcode.org/project/social_geolocation.git

PAReview checklist

https://pareview.sh/pareview/https-git.drupal.org-project-social_geoloca...

Comments

Kingdutch created an issue. See original summary.

kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Assigned: Unassigned » kingdutch
Issue summary: View changes
Status: Needs review » Needs work

Some more work on our side is needed to satisfy PA Review :)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: kingdutch » Unassigned
Issue summary: View changes
kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Needs work » Needs review

Fixed issues raised by PAReview. DrupalPractice has some warnings that can be safely ignored:

  • The defined constants don't make sense in a class and are better than magic numbers in the code.
  • The undefined variables are actually not undefined. PHPCS just doesn't understand array destructuring which was introduced in PHP 7.1 (and is thus safe to use on supported PHP versions).
vuil’s picture

vuil
Location Bulgaria 🇧🇬 🇪🇺 🌍
commented
Status: Needs review » Needs work

Thank you for your contribution!

- I can not understand why you have the following (not-present) modules as dependencies in .info.yml file?

dependencies:
  - ...
  - social_event
  - social_group
  - social_profile

Please replace them with:

  - social:social_event
  - social:social_group
  - social:social_profile

- Please add the following modules in your .info.yml file (some of them will be also in composer.json):

  - geocoder:geocoder
  - group:group
  - profile:profile
  - search_api_location:search_api_location
kingdutch’s picture

kingdutch
he/him
Primary language English
commented
Status: Needs work » Needs review

Hi vuil,

Thanks for your review.

We have on purpose removed the namespace from the dependencies. You can see the change here: https://github.com/goalgorilla/social_geolocation/commit/04ef81d2fb2490a...

Namespacing modules in an installation profile causes issues. Drupal.org will try to create a composer.json module that downloads the modules directly. However, there is no drupal/social_event module. Drupal does not expose modules from an installation profile as separate composer packages. I've already filed a detailed issue about this here: #3099771: Don't expose submodules in Drupal's packagist at all

The geocoder and search_api_location moduels should not be added to the .info.yml file. They're not actually dependencies of the current version of the module. They are still in the composer.json to ensure that sites that upgraded from an earlier internal version didn't suddenly break. I've created a follow-up here: #3100305: Remove geocoder and search_api_location dependencies and update README.md

The group and profile module dependencies are taken care of through social_profile and social_group.

I'm moving this back to needs review, I hope I've addressed your concerns.

klausi’s picture

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

Thanks for your contribution!

  1. SocialGeolocationUserAdminPeopleConfigOverride: doc block on the class seems wrong?
  2. social_geolocation_autocomplete.js: why is this JS needed? Please add a code comment.

Did not see any security issues, looks good to me!

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno
Status: Reviewed & tested by the community » Fixed

Thank you for your contribution! I am going to update your account.

These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Status: Fixed » Closed (fixed)

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