Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to specify default location in a static Closure #33

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions psalm-baseline.xml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
<DeprecatedProperty>
<code><![CDATA[$this->remote_ip]]></code>
<code><![CDATA[$this->remote_ip]]></code>
<code><![CDATA[$this->remote_ip]]></code>
</DeprecatedProperty>
<DocblockTypeContradiction>
<code><![CDATA[$this->currencies === null]]></code>
Expand Down Expand Up @@ -146,6 +147,7 @@
</PropertyTypeCoercion>
<RiskyTruthyFalsyComparison>
<code><![CDATA[! $location->currency]]></code>
<code><![CDATA[! ($this->default_location['ip'] ?? false)]]></code>
<code><![CDATA[$address = getenv($key)]]></code>
<code><![CDATA[$ip]]></code>
</RiskyTruthyFalsyComparison>
Expand Down
25 changes: 19 additions & 6 deletions src/GeoIP.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,12 @@ class GeoIP
'cached' => false,
];

/**
* Resolver for the default Location.
* @var (\Closure():\InteractionDesignFoundation\GeoIP\Location)|null
*/
public static ?\Closure $defaultLocationResolver = null;

/**
* Create a new GeoIP instance.
*
Expand All @@ -98,13 +104,20 @@ public function __construct(array $config, CacheManager $cache)
$this->cache->setPrefix((string) $this->config('cache_prefix'));

// Set custom default location
$this->default_location = array_merge(
$this->default_location,
$this->config('default_location', [])
);
$this->default_location = is_callable(self::$defaultLocationResolver)
? call_user_func(self::$defaultLocationResolver)->toArray()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @alies-dev,

Does this mean that we now have two places to change this value?
Also, this discards one of the usecases that we have, which is changing the country for local testing & in tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mithredate
yes, this is a public repo, I implemented this resolver and will remove other options on next major version of the package

Copy link
Member Author

@alies-dev alies-dev Mar 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this discards one of the usecases that we have, which is changing the country for local testing & in tests.

I'm not sure it exactly discards, but changes a way on how to fake/set the response, right? For tests, we can use a fake implementation of the IpGeoLocator.

For local testing: well, it's more interesting. It's not an issue of this package, but by GEO_DEFAULT_COUNTRY_ISO_CODE we can change only iso_code what is currently ok for us. But what if we need to change e.g. postal_code? Should we also introduce a new .env var? Such a resolver is more flexible, but the downside is that we need to touch committed to git code to change the default result, what is also not ideal. I don't see an ideal solution here.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is that this code is being committed to git. Currently, we use the env to change the default location for local development. This new snippet doesn't allow that, unless you ignore the file (unstage it). This is error-prone and not really a configuration to enhance DX IMO.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give me some time to think about this. I also like the new approach better, but it doesn't help since we might accidentally commit things and then we need to roll-them back, etc.

: array_merge($this->default_location, $this->config('default_location', []));

// Set IP
$this->remote_ip = $this->default_location['ip'] = $this->getClientIP();
$this->remote_ip = $this->getClientIP();
if (! ($this->default_location['ip'] ?? false)) { // backward compatibility hack
$this->default_location['ip'] = $this->remote_ip;
}
}

/** @param (\Closure():\InteractionDesignFoundation\GeoIP\Location)|null $defaultLocationResolver */
public static function resolveDefaultLocationUsing(?\Closure $defaultLocationResolver): void
{
self::$defaultLocationResolver = $defaultLocationResolver;
}

/**
Expand Down
20 changes: 20 additions & 0 deletions tests/GeoIPTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@

namespace InteractionDesignFoundation\GeoIP\Tests;

use InteractionDesignFoundation\GeoIP\GeoIP;
use InteractionDesignFoundation\GeoIP\Location;

/**
* @covers \InteractionDesignFoundation\GeoIP\GeoIP
*/
Expand Down Expand Up @@ -38,4 +41,21 @@ public function get_cache_returns_cache_instance(): void

$this->assertInstanceOf(\InteractionDesignFoundation\GeoIP\Cache::class, $geoIp->getCache());
}

/** @test */
public function it_gets_default_location_using_resolver_if_it_is_specified(): void
{
GeoIP::resolveDefaultLocationUsing(static function (): Location {
return new Location([
'ip' => '192.168.0.42',
'iso_code' => 'CA',
]);
});
$geoIp = $this->makeGeoIP();

$defaultLocation = $geoIp->getLocation();

$this->assertSame($defaultLocation['ip'], '192.168.0.42');
$this->assertSame($defaultLocation['iso_code'], 'CA');
}
}