Problem/Motivation

Split out of MR !9 on #3618910: Expose Surface for use with OpenStreetmap Module et al., per comment #10 there.

  • The root composer.json requires drupal/geocoder, drupal/leaflet and geocoder-php/nominatim-provider. drupal.org packaging only reads the root file, so every consumer installs them whether or not it uses the UI submodule.
  • The default endpoint is http://overpass-api.de/api/.
  • There is no CI configuration, so the module's tests do not run on drupal.org.
  • tests/src/PHPCodeSniffer/Standard.php extends a class that does not exist in any PHP_CodeSniffer release, is referenced by no ruleset, and is the source of 56 coding-standards errors.

Proposed resolution

  • Move those three packages to suggest and require-dev, alongside drupal/feeds. The submodules' .info.yml dependencies already enforce them where they are used.
  • core_version_requirement: ^10.3 || ^11 and php: 8.1 on every .info.yml.
  • Default the endpoint to https://overpass-api.de/api/, with an update hook that moves sites still on the old http default.
  • Add .gitlab-ci.yml, phpcs.xml.dist, phpstan.neon and a .gitignore.
  • Rewrite the README.
  • Group the modules under the OpenStreetMap package on the Extend page.
  • Delete tests/src/PHPCodeSniffer/Standard.php.

Remaining tasks

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

tbcs created an issue. See original summary.

orkutmuratyilmaz made their first commit to this issue’s fork.

orkutmuratyilmaz’s picture

Status: Active » Needs review

@tbcs, thanks for splitting this out. With #3618910: Expose Surface for use with OpenStreetmap Module et al. merged I rebased the branch onto 1.0.x (no conflicts; your two commits keep your authorship) and pushed three commits on top. The packaging part is what I hoped for: geocoder, leaflet and the Nominatim provider move to suggest, overpass_api_ui.info.yml still refuses to enable without them, and overpass_api_update_10002() only touches sites still on the exact old default.

"Trim the docs and composer.json to what 1.0.x ships": the branch was written on top of the preset and Feeds work, so a few things described code that is not on 1.0.x, or changed after you wrote them:

"Fix the cspell and phpcs findings" and "Fix the phpcs errors coder 8.3 reports": since this issue introduces the CI config, I cleaned up what it reports (the second commit because the pipeline runs coder 8.3.31 and I had first checked with 9.0.1; the two disagree on a few lines):

  • cspell: the module's domain words in _CSPELL_WORDS, a cspell:ignore for "İzmir" in QueryBuilderTest, the betweeen typo, "synchronized", and "Please" (forbidden by Drupal's dictionary) out of the flood message and a test remark. The cspell job is green now.
  • phpcs: all errors fixed (permissions file newline, use Url in overpass_api_ui.install, the lock message indentation in SettingsForm, fall-through comments in Utils::returnBytes(), two nested arrays in QueryBuilderTest). Checked with coder 8.3.31, which the pipeline uses, and with 9.0.1. The job stays red only on the 23 DrupalPractice "\Drupal calls should be avoided" warnings in the two forms, which go away with the dependency injection in #3630442: Inject services into SettingsForm and QueryBuilderForm, and lower the retry defaults.

eslint (query-builder-map.js) and phpstan (mostly \Drupal calls in the two forms) are bigger and overlap with #3630442: Inject services into SettingsForm and QueryBuilderForm, and lower the retry defaults, so they stay allow_failure for now.

On the ^10.3 floor: I could not find anything on 1.0.x that needs 10.3, so I read it as a support-policy change. That's fine with me, since 10.3 is out of security support; it goes into the 1.0.0-beta5 release notes.

Please have a look at the three commits. I'd like this to land before 1.0.0-beta5, so the https default goes out with the #3618910: Expose Surface for use with OpenStreetmap Module et al. changes in one set of release notes. If nothing comes up, I'll merge it in a few days.

orkutmuratyilmaz’s picture

@tbcs, I've just updated the MR !10's title and description too. Since my last comment, three more commits make every pipeline job green, eslint, phpcs and phpstan included:

Squash is off, so your two commits stay as they are in 1.0.x. If nothing comes up, I'll merge in a few days.