GEOCODER_45 : composer adjustments

[heitzjm]

It seems to me that composer.json caused problems during a drupal/civicrm/geocoder install using composer. The replace section might not be appropriate as the extension itself does not implement the composer packages it claims to replace.

Furthermore, replacing \Http\Adapter\Guzzle6\Client with Http\Discovery\Psr18Client might be more generic (I encountered composer install failures because of conflicting required versions of Guzzle). However, my experiments made me think that php-http/discovery plugin sometimes does changes to composer.json, so I think it might be better to disable the plugin.

This PR is an excerpt of PR #46 . As I wrote there ( https://github.com/eileenmcnaughton/org.wikimedia.geocoder/pull/46#issue-1926121372), I did not add the resulting vendor directory, as it should be produced by composer : however, this might be needed if someone install the extension directly and is relying on the included vendor directory (I guess it depends on the way an extension is packaged/released - I have no experience on that point).

[artfulrobot]

This looks interesting!

The replace bit was added by me because Civi provides Guzzle and this is a Civi extension; therefore it's unhelpful for this extension to provide Guzzle as well (leading to a race condition about which package gets used).

What I can't get my head around is that Discovery, while being a proxy for finding Guzzle (say), might suffer the same issues, were (a different version of) Discovery to be installed by another package (extension/civi/cms/cms plugin...)?

[heitzjm]

@artfulrobot , I am only a beginner in Composer and Civi, and this is a question I also wonder about when I consider a packaged Civi Extension outside of Composer. I only know that some projects try to mitigate dependency conflicts using a "box approach" ( for example in cv ( https://github.com/civicrm/cv/tree/master) ).

However, if you consider the Civi Extension as a composer package, it is composer's responsibility to fetch all the required dependencies that fit together (in a global directory), but of course, it means that you have to use composer to manage your whole application ( for example, as described for installing CiviCRM on Drupal 9, in the installation guide - https://docs.civicrm.org/installation/en/latest/drupal/ ). It also means that it might involve a procedure to build the extension in order to release it as a packaged Civi Extension.

Composer made local changes using upgrade in my local vendor directory. I did not commit these changes because I was not sure about it ( In fact this PR is part of PR #46 which mentions this point in https://github.com/eileenmcnaughton/org.wikimedia.geocoder/pull/46#issue-1926121372). I am going to edit the first comment to add this information.

[artfulrobot]

Well if you're a beginner, @heitzjm then welcome! and great to have you join us :wave:

I'm not a pro with composer either. I think this is a perennial problem with packaging for mixed environments (with/out composer). I'm not sure we have a solution yet (though I have some memory that @totten might have come up with something of late?).

To run the phpunit tests you need a buildkit environment - see: https://docs.civicrm.org/dev/en/latest/tools/buildkit/

Here's my output :cry: (without your PR):

1) GeocoderTest::testOpenStreetMapsFailsFallsBackToUSLookup
Undefined index: geo_code_1

/buildkit/build/so1/web/upload/ext/org.wikimedia.geocoder/tests/phpunit/GeocoderTest.php:139
/buildkit/extern/phpunit9/phpunit9.phar:2307

2) GeocoderTest::testShortPostalCode
Undefined index: geo_code_1

/buildkit/build/so1/web/upload/ext/org.wikimedia.geocoder/tests/phpunit/GeocoderTest.php:171
/buildkit/extern/phpunit9/phpunit9.phar:2307

3) GeocoderTest::testGeoName
Undefined index: geo_code_1

/buildkit/build/so1/web/upload/ext/org.wikimedia.geocoder/tests/phpunit/GeocoderTest.php:196
/buildkit/extern/phpunit9/phpunit9.phar:2307

--

There was 1 failure:

1) GeocoderTest::testUK
Failed asserting that null matches expected '51.49984'.

/buildkit/build/so1/web/upload/ext/org.wikimedia.geocoder/tests/phpunit/GeocoderTest.php:237
/buildkit/extern/phpunit9/phpunit9.phar:2307

With your PR applied, and after a composer update (was I supposed to do that?) inside this ext's dir, I get 5 errors:

Error: Class 'Http\Adapter\Guzzle6\Client' not found

So I'm a bit stumped. It's hard to improve something when the tests aren't working before you start!

[eileenmcnaughton]

FWIW we have tests running as part of our CI - but we install using the composer_merge_plugin & git ignore the vendor directory in the extension https://github.com/wikimedia/composer-merge-plugin

[heitzjm]

Thanks for your time. I was able to reproduce the failures with phpunit and the drupal-clean profile. I didn't notice that the tests also used Guzzle - I am going to try to fix it ( There is a section about mocks in php-http/discovery at https://docs.php-http.org/en/latest/discovery.html ).

[heitzjm]

The error about the Guzzle6 Client should be fixed now. However, the errors described in #51 still exist in this branch.

[github-actions[bot]]

Stale pull request message

[eileenmcnaughton]

I have brought across your main commit & it passed tests so I have pushed it in - I don't know if there will be further issues but it seemed like the right direction & if there is any fall out then better to fix that fall out than to fix the 'old way' - which I agree this improves upon in terms of managing compatibility.

I didn't do the second commit because tests passed without it & I thought that was probably the reason for it?