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

Add default callback functionality #42

Open
wants to merge 5 commits into
base: master
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
53 changes: 53 additions & 0 deletions src/CallbackRegistrar.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Exception;
use Illuminate\Support\Str;
use InvalidArgumentException;
use ReflectionClass;
use ReflectionMethod;

Expand All @@ -16,6 +17,13 @@ class CallbackRegistrar
*/
protected $proxies;

/**
* The default callback and its arguments.
*
* @var array
*/
protected $default = [[Callbacks::class, 'unauthorized'], []];

/**
* Create a new CallbacksRegistrar instance.
*/
Expand Down Expand Up @@ -141,4 +149,49 @@ public function hasProxy(string $proxy)
{
return array_key_exists($proxy, $this->proxies);
}

/**
* Set the default callback and its arguments.
*
* @param string|callable $callback
* @param mixed ...$arguments
*
* @return void
*/
public function setDefault($callback, ...$arguments)
{
if (is_callable($callback)) {
return $this->default = [$callback, $arguments];
Copy link

Choose a reason for hiding this comment

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

According to the doc block @return void but this clearly returns the default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh that is a typo, forgot to change the auto generated return type, i will also add the @throws exception

}

if (is_string($callback)) {
return $this->default = [$this->callback($callback), $arguments];
}

throw new InvalidArgumentException(sprintf(
"%s expects parameter 1 to be string or callable %s given",
__FUNCTION__,
gettype($callback)
));
}

/**
* Get the default callback callable and its arguments.
*
* @return array
*/
public function getDefault()
{
return $this->default;
}

/**
* Invoke the default callback.
*
* @return mixed
*/
public function invokeDefault()
{
return call_user_func_array(...$this->default);
}
}
10 changes: 10 additions & 0 deletions src/Callbacks.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,16 @@

class Callbacks
{
/**
* Return a HTTP 401 Unauthorized response.
*
* @throws \Symfony\Component\HttpKernel\Exception\HttpException
*/
public static function unauthorized()
{
throw new HttpException(401);
}
yak0d3 marked this conversation as resolved.
Show resolved Hide resolved

/**
* Return a HTTP 404 Not Found response.
*
Expand Down
3 changes: 2 additions & 1 deletion src/Http/Middleware/GeoMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Closure;
use Illuminate\Http\Request;
use LaraCrafts\GeoRoutes\DeterminesGeoAccess;
use LaraCrafts\GeoRoutes\Support\Facades\CallbackRegistrar;

class GeoMiddleware
{
Expand All @@ -24,7 +25,7 @@ public function handle(Request $request, Closure $next)
$strategy = config()->get('geo-routes.global.strategy');

if (!$this->shouldHaveAccess($countries, $strategy)) {
abort(401);
return CallbackRegistrar::invokeDefault();
}

return $next($request);
Expand Down
6 changes: 3 additions & 3 deletions src/Http/Middleware/GeoRoutesMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use Illuminate\Http\Request;
use Illuminate\Support\Facades\Validator;
use LaraCrafts\GeoRoutes\DeterminesGeoAccess;
use LaraCrafts\GeoRoutes\Support\Facades\CallbackRegistrar;

class GeoRoutesMiddleware
{
Expand All @@ -25,8 +26,7 @@ public function handle(Request $request, Closure $next)
$route = $request->route();

if (!$route) {
#TODO: Invoke the default callback.
return abort(401);
return CallbackRegistrar::invokeDefault();
}

$constraint = $route->getAction('geo') ?? [];
Expand All @@ -49,6 +49,6 @@ public function handle(Request $request, Closure $next)
return call_user_func_array($callback[0], $callback[1]);
}

return abort(401);
return CallbackRegistrar::invokeDefault();
}
}
3 changes: 3 additions & 0 deletions src/Support/Facades/CallbackRegistrar.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
* @method static mixed callback(string $name, callable $callable = null)
* @method static boolean hasCallback(string $name)
* @method static boolean hasProxy(string $proxy)
* @method static void setDefault(string|callable $callback, ...$arguments)
* @method static array getDefault()
* @method static mixed invokeDefault()
*
* @see \LaraCrafts\GeoRoutes\CallbackRegistrar
*/
Expand Down
90 changes: 65 additions & 25 deletions tests/Unit/CallbackRegistrarTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
namespace LaraCrafts\GeoRoutes\Tests\Unit;

use Closure;
use InvalidArgumentException;
use LaraCrafts\GeoRoutes\CallbackRegistrar;
use LaraCrafts\GeoRoutes\Tests\TestCase;
use Mockery;
Expand All @@ -22,9 +23,14 @@ class CallbackRegistrarTest extends TestCase
/** @var \Illuminate\Routing\Router */
protected $router;

/** @var \LaraCrafts\GeoRoutes\CallbackRegistrar|\Mockery\MockInterface */
protected $registrar;

public function setUp(): void
{
parent::setUp();

$this->registrar = new CallbackRegistrar;
}

public function tearDown(): void
Expand All @@ -39,10 +45,9 @@ public function testIfCallbacksFetchesTheProxiesList()
'bar' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::bar',
];

$registrar = new CallbackRegistrar;
$registrar->loadCallbacks($callbacks);
$this->registrar->loadCallbacks($callbacks);

$proxies = $registrar->callbacks();
$proxies = $this->registrar->callbacks();

foreach ($callbacks as $key => $callback) {
$this->assertArrayHasKey('or' . ucfirst($key), $proxies);
Expand All @@ -57,11 +62,11 @@ public function testIfCallbacksInvokesLoadCallbacksWhenArrayArgIsPresent()
'bar' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::bar',
];

$registrar = Mockery::mock(CallbackRegistrar::class)->makePartial();
$this->registrar = Mockery::mock(CallbackRegistrar::class)->makePartial();

$registrar->shouldReceive('loadCallbacks')->with($callbacks)->once();
$this->registrar->shouldReceive('loadCallbacks')->with($callbacks)->once();

$registrar->callbacks($callbacks);
$this->registrar->callbacks($callbacks);
}

public function testIfParseCallbacksLoadsCallbacksFromClass()
Expand All @@ -71,11 +76,9 @@ public function testIfParseCallbacksLoadsCallbacksFromClass()
'orBar' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::bar',
];

$registrar = new CallbackRegistrar;

$registrar->parseCallbacks(\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::class);
$this->registrar->parseCallbacks(\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::class);

$proxies = $registrar->callbacks();
$proxies = $this->registrar->callbacks();

foreach ($expected as $proxy => $callable) {
$this->assertArrayHasKey($proxy, $proxies);
Expand All @@ -85,41 +88,78 @@ public function testIfParseCallbacksLoadsCallbacksFromClass()

public function testIfCallbackReturnsCallable()
{
$registrar = new CallbackRegistrar;
$registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);
$this->registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);

$this->assertEquals('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', $registrar->callback('foo'));
$this->assertEquals('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', $registrar->callback('orFoo'));
$this->assertEquals('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', $this->registrar->callback('foo'));
$this->assertEquals('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', $this->registrar->callback('orFoo'));
}

public function testIfHasCallbackReturnsTrueIfCallbackExists()
{
$registrar = new CallbackRegistrar;
$registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);
$this->registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);

$this->assertTrue($registrar->hasCallback('foo'));
$this->assertTrue($this->registrar->hasCallback('foo'));
}

public function testIfHasCallbackReturnsFalseIfCallbackDoesNotExist()
{
$registrar = new CallbackRegistrar;

$this->assertFalse($registrar->hasCallback('foo'));
$this->assertFalse($this->registrar->hasCallback('foo'));
}

public function testIfHasProxyReturnsTrueIfProxyExists()
{
$registrar = new CallbackRegistrar;
$registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);
$this->registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);

$this->assertTrue($registrar->hasProxy('orFoo'));
$this->assertTrue($this->registrar->hasProxy('orFoo'));
}

public function testIfHasProxyReturnsFalseIfProxyDoesNotExist()
{
$registrar = new CallbackRegistrar;
$this->assertFalse($this->registrar->hasProxy('orFoo'));
}

public function testIfSetDefaultInvokesCallbackIfArgIsString()
{
$this->registrar = Mockery::mock(CallbackRegistrar::class)->makePartial();
$this->registrar->loadCallbacks(['foo' => '\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo']);

$this->registrar->shouldReceive('callback')->with('foo')->once();

$this->registrar->setDefault('foo');
}
Comment on lines +121 to +129
Copy link

Choose a reason for hiding this comment

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

In my opinion this test is unnecessary. You should test the functionality, not the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Omitting this test means that we'll have non-test covered code, also this test assures that the method does not interpret callables (e.g. FooClass::bar) as strings which would result in an exception.

Copy link

Choose a reason for hiding this comment

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

In my opinion you should test setDefault with getDefault. As I said test the functionality not the implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have thought of that during the testing phase, but that would make the tests tightly coupled and would cause both to fail if the getDefault method is failing. In addition, in my own humble opinion, this is actually testing the functionality which is accepting both callables and strings and in return setting the default value.

Copy link

Choose a reason for hiding this comment

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

To be exact my problem is at line 126. With that you are testing the implementation. However, as far as I know you made setDefault to return something. in that case you can test the return of that function. Lastly, getters and setters are usually "tightly coupled" similarly to push and pop.


public function testIfSetDefaultSetsDefaultPropertyValueIfArgIsCallable()
{
$this->registrar->setDefault('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', 'arg1');

$default = $this->getProperty($this->registrar, 'default')->getValue($this->registrar);

$this->assertEquals(['\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', ['arg1']], $default);
}

public function testIfSetDefaultThrowsExceptionIfArgIsInvalid()
{
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('setDefault expects parameter 1 to be string or callable integer given');

$this->registrar->setDefault(321314);
}

public function testIfGetDefaultReturnsDefaultCallbackArray()
{
$this->registrar->setDefault('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', 'arg1');

$this->assertEquals(
['\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo', ['arg1']],
$this->registrar->getDefault()
);
}

public function testIfInvokeDefaultExecutesDefault()
{
$this->registrar->setDefault('\LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks::foo');

$this->assertFalse($registrar->hasProxy('orFoo'));
$this->assertEquals('foo', $this->registrar->invokeDefault());
}

/**
Expand Down
18 changes: 18 additions & 0 deletions tests/Unit/GeoMiddlewareTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

use Illuminate\Contracts\Http\Kernel;
use Illuminate\Http\Request;
use LaraCrafts\GeoRoutes\Support\Facades\CallbackRegistrar;
use LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks;
use LaraCrafts\GeoRoutes\Tests\TestCase;
use Mockery;

Expand Down Expand Up @@ -58,4 +60,20 @@ public function testIfAllowedCountryIsLetThrough()
$response = $this->kernel->handle($this->request);
$this->assertEquals(200, $response->getStatusCode());
}

public function testIfMiddlewareExecutesDefaultCallback()
{
$callbacks = Mockery::mock(Callbacks::class);
$callbacks->shouldReceive('foo')->once()->with('bar')->andReturn('foo');

CallbackRegistrar::setDefault([$callbacks, 'foo'], 'bar');

$this->location->shouldReceive('get')
->once()
->andReturn((object)['countryCode' => 'ch']);

$response = $this->kernel->handle($this->request);

$this->assertEquals('foo', $response);
}
}
31 changes: 25 additions & 6 deletions tests/Unit/GeoRoutesMiddlewareTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
use Illuminate\Http\Request;
use Illuminate\Routing\Route;
use LaraCrafts\GeoRoutes\Http\Middleware\GeoRoutesMiddleware;
use LaraCrafts\GeoRoutes\Support\Facades\CallbackRegistrar;
use LaraCrafts\GeoRoutes\Tests\Mocks\Callbacks;
use LaraCrafts\GeoRoutes\Tests\TestCase;
use Mockery;
use Mockery\Adapter\Phpunit\MockeryPHPUnitIntegration;
Expand Down Expand Up @@ -78,23 +80,23 @@ public function middlewareAllowsAccess()
/** @test */
public function middlewareExecutesCallback()
{
$mockClass = Mockery::mock('alias:mockClass');
$mockClass->shouldReceive('callback')
$callbacks = Mockery::mock(Callbacks::class);
$callbacks->shouldReceive('foo')
->once()
->with('arg')
->andReturn('MockCallback');
->andReturn('foo');

$this->location->shouldReceive('get')
->once()
->andReturn((object)['countryCode' => 'ca']);

$callback = ['mockClass::callback', ['arg']];
$callback = [[$callbacks, 'foo'], ['arg']];

$this->setGeoConstraint('allow', 'us', $callback);

$output = $this->middleware->handle($this->request, $this->next);

$this->assertEquals('MockCallback', $output);
$this->assertEquals('foo', $output);
}

/** @test */
Expand All @@ -112,7 +114,24 @@ public function middlewareThrowsExceptionIfTheGeoAttributeIsInvalid()
->once()
->andReturn([]);

$output = $this->middleware->handle($this->request, $this->next);
$this->middleware->handle($this->request, $this->next);
}

/** @test */
public function middlewareExecutesDefaultCallbackIfNoCallbackIsFound()
{
$callbacks = Mockery::mock(Callbacks::class);
$callbacks->shouldReceive('foo')->once()->with('bar');

CallbackRegistrar::setDefault([$callbacks, 'foo'], 'bar');

$this->location->shouldReceive('get')
->once()
->andReturn((object)['countryCode' => 'ca']);

$this->setGeoConstraint('deny', 'ca');

$this->middleware->handle($this->request, $this->next);
}

/**
Expand Down