Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Support for setting PHP settings through configuration #193

Closed
Closed
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
28 changes: 28 additions & 0 deletions src/Container/ApplicationFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Zend\Diactoros\Response\EmitterInterface;
use Zend\Expressive\Application;
use Zend\Expressive\Exception;
use Zend\Expressive\Container\Exception\PhpSettingsFailureException;
use Zend\Expressive\Router\FastRouteRouter;
use Zend\Expressive\Router\Route;
use Zend\Expressive\Router\RouterInterface;
Expand Down Expand Up @@ -136,13 +137,40 @@ public function __invoke(ContainerInterface $container)

$app = new Application($router, $container, $finalHandler, $emitter);

$this->setPhpSettings($container);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maks3w What do you think about moving $this->setPhpSettings($container); at the very beginning of a __invoke() method, because it doesn't need $app at all?

$this->injectPreMiddleware($app, $container);
$this->injectRoutes($app, $container);
$this->injectPostMiddleware($app, $container);

return $app;
}

/**
* Sets provided PHP settings, if any.
*
* @param ContainerInterface $container
* @throws Exception\InvalidArgumentException for invalid PHP settings.
* @throws PhpSettingsFailureException if setting of PHP configuration fails.
*/
private function setPhpSettings(ContainerInterface $container)
{
$config = $container->has('config') ? $container->get('config') : [];

if (!isset($config['php_settings'])) {
return;
}

if (!is_array($config['php_settings'])) {
throw new Exception\InvalidArgumentException('Invalid PHP settings configuration; must be an array');
}

foreach ($config['php_settings'] as $name => $value) {
if (false === ini_set($name, $value)) {
throw PhpSettingsFailureException::forOption($name, $value);
}
}
}

/**
* Inject routes from configuration, if any.
*
Expand Down
23 changes: 23 additions & 0 deletions src/Container/Exception/PhpSettingsFailureException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
<?php
/**
* Zend Framework (http://framework.zend.com/)
*
* @see http://github.com/zendframework/zend-expressive for the canonical source repository
* @copyright Copyright (c) 2015 Zend Technologies USA Inc. (http://www.zend.com)
* @license https://github.com/zendframework/zend-expressive/blob/master/LICENSE.md New BSD License
*/

namespace Zend\Expressive\Container\Exception;

use RuntimeException;

/**
* Exception indicating that setting of some PHP configuration option has failed.
*/
class PhpSettingsFailureException extends RuntimeException implements ExceptionInterface
{
public static function forOption($name, $value)
{
return new self("Setting PHP configuration '$name' with a value of '$value' has failed");
}
}
105 changes: 105 additions & 0 deletions test/Container/ApplicationFactoryTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,68 @@ public function setUp()
$this->factory = new ApplicationFactory();
}

protected function getContainer($config = null, $router = null, $finalHandler = null, $emitter = null)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maks3w What do you think about this approach, of having a helper method for configuring container based on the test specificity? Test cases now look more cleaner. Maybe these should be applied to all the other other ApplicationFactory tests.

Copy link
Member

Choose a reason for hiding this comment

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

While I saw too a helper or many helper methods are needed I disagree with this proposal because add too much logic.

The problem with add logic branches in a test case (if, for, ...) is difficult to trust is bugfree. What test method test getContainer works as expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this would allow us to have only one place for managing and maintaining container configuration and it also solves code duplication in all the test cases in this class. I agree though that it is somewhat unreliable due to its complexity, but after all, every test case which consumes will fail if it does not work as expected.

{
if (null !== $config) {
$this->container
->has('config')
->willReturn(true);

$this->container
->get('config')
->willReturn($config);
} else {
$this->container
->has('config')
->willReturn(false);
}

if (null !== $router) {
$this->container
->has('Zend\Expressive\Router\RouterInterface')
->willReturn(true);
$this->container
->get('Zend\Expressive\Router\RouterInterface')
->will(function () use ($router) {
return $router->reveal();
});
} else {
$this->container
->has('Zend\Expressive\Router\RouterInterface')
->willReturn(false);
}

if (null !== $finalHandler) {
$this->container
->has('Zend\Expressive\FinalHandler')
->willReturn(true);
$this->container
->get('Zend\Expressive\FinalHandler')
->willReturn($finalHandler);
} else {
$this->container
->has('Zend\Expressive\FinalHandler')
->willReturn(false);
}

if (null !== $emitter) {
$this->container
->has('Zend\Diactoros\Response\EmitterInterface')
->willReturn(true);
$this->container
->get('Zend\Diactoros\Response\EmitterInterface')
->will(function () use ($emitter) {
return $emitter->reveal();
});
} else {
$this->container
->has('Zend\Diactoros\Response\EmitterInterface')
->willReturn(false);
}

return $this->container;
}

public function assertRoute($spec, array $routes)
{
$this->assertTrue(array_reduce($routes, function ($found, $route) use ($spec) {
Expand Down Expand Up @@ -65,6 +127,49 @@ public function getRouterFromApplication(Application $app)
return $r->getValue($app);
}

/**
* @expectedException \Zend\Expressive\Exception\InvalidArgumentException
*/
public function testInvalidPhpSettingsRaisesException()
{
$config = ['php_settings' => 'invalid'];

$this->factory->__invoke($this->getContainer($config)->reveal());
}

/**
* @expectedException \Zend\Expressive\Container\Exception\PhpSettingsFailureException
*/
public function testWhenSettingPhpConfigurationFailsExceptionIsRaised()
{
$config = [
'php_settings' => [
'invalid_option' => 1,
],
];

$this->factory->__invoke($this->getContainer($config)->reveal());
}

public function testFactorySetsPhpSettingsFromConfig()
{
$config = [
'php_settings' => [
'display_errors' => 1,
'date.timezone' => 'Europe/Belgrade',
],
];

$this->iniSet('display_errors', 0);
$this->iniSet('date.timezone', 'UTC');

$this->factory->__invoke($this->getContainer($config)->reveal());

foreach ($config['php_settings'] as $name => $value) {
$this->assertEquals($value, ini_get($name));
Copy link
Member

Choose a reason for hiding this comment

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

this foreach should be wrapped in a try/finally where try is this loop and finally is the tear down loop

}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Maks3w Is this in line with what you've mentioned about having only one loop?

Copy link
Member

Choose a reason for hiding this comment

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

PHPUnit assertions throw exceptions when assertion fail. So ini_restore won't be restored when test fail so next tests in the run will be executed with a wrong environment setting.

After $config

   $this->iniSet('display_errors', 0);
   $this->iniSet('date.timezone', 'UTC');

}

public function testFactoryWillPullAllReplaceableDependenciesFromContainerWhenPresent()
{
$router = $this->prophesize('Zend\Expressive\Router\RouterInterface');
Expand Down