Add ConfigProvider #30
Conversation
composer.json
Outdated
| @@ -50,6 +50,9 @@ | |||
| "dev-master": "2.1.x-dev", | |||
| "dev-develop": "2.2.x-dev", | |||
| "dev-release-3.0.0": "3.0.x-dev" | |||
| }, | |||
| "zf": { | |||
| "config-provider": "Zend\\Expressive\\Router\\ConfigProvider" | |||
There was a problem hiding this comment.
As in the previous review, I would consider changing namespace for the repository.
src/ConfigProvider.php
Outdated
| <?php | ||
| /** | ||
| * @see https://github.com/zendframework/zend-expressive-aurarouter for the canonical source repository | ||
| * @copyright Copyright (c) 2015-2018 Zend Technologies USA Inc. (https://www.zend.com) |
There was a problem hiding this comment.
Only 2018 please for new files.
test/ConfigProviderTest.php
Outdated
| <?php | ||
| /** | ||
| * @see https://github.com/zendframework/zend-expressive-aurarouter for the canonical source repository | ||
| * @copyright Copyright (c) 2015-2018 Zend Technologies USA Inc. (https://www.zend.com) |
test/ConfigProviderTest.php
Outdated
| * @copyright Copyright (c) 2015-2018 Zend Technologies USA Inc. (https://www.zend.com) | ||
| * @license https://github.com/zendframework/zend-expressive-aurarouter/blob/master/LICENSE.md New BSD License | ||
| */ | ||
|
|
There was a problem hiding this comment.
no new line before strict_type declaration
There was a problem hiding this comment.
Have we actually decided on this? The Stratigility repo has no space between the file-level docblock and the declaration, but the zend-expressive repo does (a fact I was going to point out in your review of my latest patch there). We need to decide on a style and stick with one; right now, we have two in as many repos.
Personally, I prefer a line between them, as that makes it clear that the docblock is separate from the declaration.
There was a problem hiding this comment.
Not sure if we decide it here, but I prefer a new line in between as well.
There was a problem hiding this comment.
@weierophinney @xtreamwayz I've jest checked, and I was sure that we shouldn't have the empty line there, but I'm really sorry. I was wrong! I prefer the line as well, and here is a previous comment by @weierophinney:
zendframework/zend-expressive-router#41 (comment)
So lets keep the empty line, update other repositories to add this line, and ignore my all comments about it. Thanks.
src/ConfigProvider.php
Outdated
| public function getDependencies() : array | ||
| { | ||
| return [ | ||
| 'factories' => [ |
There was a problem hiding this comment.
invokables probably...
https://github.com/zendframework/zend-expressive-skeleton/blob/release-3.0.0/src/ExpressiveInstaller/Resources/config/router-aura-router.php#L10
I would consider to have invokables for AuraRouter and aliases for RouterInterface
test/ConfigProviderTest.php
Outdated
| public function testInvocationReturnsArray() : array | ||
| { | ||
| $config = ($this->provider)(); | ||
| self::assertInternalType('array', $config); |
test/ConfigProviderTest.php
Outdated
| self::assertInternalType('array', $config['dependencies']); | ||
| self::assertArrayHasKey('factories', $config['dependencies']); | ||
| self::assertInternalType('array', $config['dependencies']['factories']); | ||
| self::assertArrayHasKey(RouterInterface::class, $config['dependencies']['factories']); |
|
@xtreamwayz Same comments from the fastroute package apply here:
|
@MWOP Not sure what you mean by that. Change the namespace of the entire package to |
I think you meant @weierophinney here. 😄 No, I meant that you should add a subnamespace in this package: Otherwise, we'll have potential conflicts when trying to resolve the |
Moves it to the `Zend\Expressive\Router\AuraRouter` subnamespace, to prevent potential conflicts with other config providers under the `Zend\Expressive\Router` namespace.
We use `$this->` everywhere else, essentially, so `self::` is inconsistent. If we decided to go with the latter in the future, we can do mass updates across all components.
|
Thanks, @xtreamwayz! |
Adds ConfigProvider with basic configuration.