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
wants to merge 6 commits into from
Closed

Support for setting PHP settings through configuration #193

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0282f2f
Mechanism for setting PHP settings though configuration.
nikolaposa Nov 21, 2015
4c73602
Line-ending fixes.
nikolaposa Nov 21, 2015
6851310
Improving setting of PHP configuration options.
nikolaposa Nov 22, 2015
4ae7ff6
Fixed coding style violations.
nikolaposa Nov 22, 2015
7bd9632
Auto-resoring PHP ini settings after a test.
nikolaposa Nov 22, 2015
bade4e6
Fixed coding standard violations.
nikolaposa Nov 22, 2015
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
19 changes: 19 additions & 0 deletions src/Container/ApplicationFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,32 @@ 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
*/
private function setPhpSettings(ContainerInterface $container)
{
$config = $container->has('config') ? $container->get('config') : [];

if (!isset($config['php_settings']) || !is_array($config['php_settings'])) {
Copy link
Member

Choose a reason for hiding this comment

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

if the value is not an array I guess an exception should be raised instead silently ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kinda imitated injectPreMiddleware() flow in this case, but I guess exception should be thrown.

Copy link
Member

Choose a reason for hiding this comment

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

/cc @weierophinney I think all those type checks should throw Exception instead of silently ignore.

return;
}

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

Choose a reason for hiding this comment

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

Should raise an exception if ini_set fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I agree, exception can be raised there.

}
}

/**
* Inject routes from configuration, if any.
*
Expand Down
54 changes: 54 additions & 0 deletions test/Container/ApplicationFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,60 @@ public function getRouterFromApplication(Application $app)
return $r->getValue($app);
}

public function testFactorySetsPhpSettingsFromConfig()
{
$this->container
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed?

->has('Zend\Expressive\Router\RouterInterface')
->willReturn(false);
$this->container
Copy link
Member

Choose a reason for hiding this comment

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

This expectation is unrelated with the feature under test. Only adds noise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it adds noise, but for some reason I couldn't bypass those opening lines within the ApplicationFactory::__invoke(). I'll revisit that anyhow.

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 Any idea how to bypass this error when I remove all those expectations:

Prophecy\Exception\Call\UnexpectedCallException: Method call:
  - has("Zend\Expressive\Router\RouterInterface")
on Double\ContainerInterface\P65 was not expected,

?
It seems like stub yields error when unexpected method call is made.

Copy link
Member

Choose a reason for hiding this comment

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

This is related with the previous statement. https://github.com/zendframework/zend-expressive/pull/193/files#r45551040

Ok, it must be as you wrote.

->get('Zend\Expressive\Router\RouterInterface')
->shouldNotBeCalled();

$this->container
->has('Zend\Diactoros\Response\EmitterInterface')
->willReturn(false);
$this->container
Copy link
Member

Choose a reason for hiding this comment

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

Remove this expectation.

->get('Zend\Diactoros\Response\EmitterInterface')
->shouldNotBeCalled();

$this->container
->has('Zend\Expressive\FinalHandler')
->willReturn(false);
$this->container
Copy link
Member

Choose a reason for hiding this comment

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

Remove this expectation.

->get('Zend\Expressive\FinalHandler')
->shouldNotBeCalled();

$config = [
'php_settings' => [
'display_errors' => 1,
'date.timezone' => 'Europe/Belgrade',
],
];

$currentPhpSettings = [];
Copy link
Member

Choose a reason for hiding this comment

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

It's better use PHPUnit $this->iniSet() and set a different value than the expected so:

  • You can verify the ini set really has changed
  • You can remove both loops (this one and the restore one) and simplify the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip

foreach ($config['php_settings'] as $name => $value) {
$currentPhpSettings[$name] = ini_get($name);
}

$this->container
->has('config')
->willReturn(true);

$this->container
->get('config')
->willReturn($config);

$this->factory->__invoke($this->container->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

}

foreach ($currentPhpSettings as $name => $value) {
ini_set($name, $value);
}
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