smartystreets / smartystreets-php-sdk

The official client libraries for accessing SmartyStreets APIs from the PHP Hypertext Processor.
https://smartystreets.com/docs/sdk/php
Apache License 2.0
28 stars 15 forks source link

[CRITICAL] Constant GEOLOCATE_TYPE_CITY already defined #28

Closed rk closed 3 years ago

rk commented 3 years ago

Hi,

The regular and pro US_Autocomplete classes cannot be loaded simultaneously. The issue comes from these sections conflicting:

https://github.com/smartystreets/smartystreets-php-sdk/blob/b1bef64f04a4434572b477cc38835796e3b10470/src/US_Autocomplete/GeolocateType.php#L5-L7

https://github.com/smartystreets/smartystreets-php-sdk/blob/b1bef64f04a4434572b477cc38835796e3b10470/src/US_Autocomplete_Pro/GeolocateType.php#L5-L6

The way to fix this is easy:

if (!defined('GEOLOCATE_TYPE_CITY')) {
  define('GEOLOCATE_TYPE_CITY', 'city', false);
}

if (!defined('GEOLOCATE_TYPE_NONE')) {
  define('GEOLOCATE_TYPE_NONE', null, false);
}

Also, the 3rd parameter of define() has been deprecated since PHP 7.3. It is strongly recommended that it be removed, as your library requires PHP 5.6 - PHP 8 in composer.json, which lets it get pulled into all current/modern versions of PHP and can automatically break them.


As a side note, if these suggested changes cause issues for your code, then you will need to rewrite it so that these external constants aren't used. Looking at the usage pattern, these constants serve absolutely no purpose to the code:

https://github.com/smartystreets/smartystreets-php-sdk/blob/b1bef64f04a4434572b477cc38835796e3b10470/src/US_Autocomplete_Pro/Lookup.php#L46

This is the only use. You could even add a public const TYPE = "city"; on the GeolocateType class itself.

But, using a global constant in place of a hard-coded string isn't an advisable pattern. This can easily break linters, static analyzers, etc. These conflicting define() calls is all it takes to break a framework like Laravel, as it needs to run certain tasks for auto-discovery of packages and features. So when this update arrives it'll break each app that requires it, preventing package autodiscovery and certain IDE helpers from running (this goes for anyone using PHPStorm and the corresponding IDE helper package(s)).

DuncanBeutler commented 3 years ago

Fix can be accessed on 4.13.3