T172892 Determine skin to use based on request #955
Conversation
tests/EdgeToEdge/SkinTest.php
Outdated
| continue; | ||
| } | ||
| $this->assertSame( $expected, $cookie->getValue() ); | ||
| } |
There was a problem hiding this comment.
If it is not found, no assertion will be made. While this results in a warning (or error?) when using PHPUnit strict mode, it is a bit dodgy. You could return after the assert and put a $this->fail() at the end of the method.
tests/EdgeToEdge/SkinTest.php
Outdated
| $client->request( 'GET', '/' ); | ||
|
|
||
| $this->assertContains( '10h16', $client->getResponse()->getContent() ); | ||
| $this->assertNoSkinResponseCookie( $client->getResponse() ); |
There was a problem hiding this comment.
If you put the assert methods (and all methods really) right below where they are first used, less vertical scrolling is needed to real the class.
|
Looks good so far apart from minor CS-ish stuff |
src/Factories/FunFunFactory.php
Outdated
| @@ -468,6 +471,10 @@ function( string $name, array $parameters = [] ): string { | |||
| return new SofortClient( $config['config-key'] ); | |||
| }; | |||
|
|
|||
| $pimple['skin'] = function () { | |||
There was a problem hiding this comment.
Since skin is more of a parameter than a service I think it's sufficient to assign it directly instead of using a function with a return value.
wiese
force-pushed
the
skin-on-request
branch
from
6e6b8ac
to
c16ccc6
Compare
made cookie builder a reusable service to avoid redundant instances
src/Presentation/SkinManager.php
Outdated
|
|
||
| use InvalidArgumentException; | ||
|
|
||
| class SkinManager { |
There was a problem hiding this comment.
I'm not sure if this class "manages" anything, it looks like a fancy mutable value object to me. Maybe call it SkinSettings, SkinConfiguration or just Skins?
wiese
changed the title
Improve descriptions in config schema doc
|
|
||
| public function testDefaultSkinIsSet(): void { | ||
| $manager = new SkinSettings( ['a', 'b'], 'a', 500 ); | ||
| $this->assertSame( 'a', $manager->getDefaultSkin() ); |
There was a problem hiding this comment.
I suppose SkinSettings was renamed, cause $manager is kinda weird like this
Works best with additional
config.prod.jsonResources: