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

EnvironmentInterface seems to be a useless type to expect / depend on #2548

Closed
Rarst opened this issue Nov 28, 2018 · 2 comments
Closed

EnvironmentInterface seems to be a useless type to expect / depend on #2548

Rarst opened this issue Nov 28, 2018 · 2 comments

Comments

@Rarst
Copy link
Contributor

Rarst commented Nov 28, 2018

Slim\Container declares default environment service to be an implementation of EnvironmentInterface.

Slim\App expects a container that implements Psr\Container\ContainerInterface
with these service keys configured and ready for use:

 - environment: an instance of \Slim\Interfaces\Http\EnvironmentInterface

@property-read \Slim\Interfaces\Http\EnvironmentInterface environment

Thing is EnvironmentInterface doesn’t contain anything actually needed for it to work, only test–related mock().

This means that this $app->getContainer()->environment->get('foo'); won’t be understood by any tools, because the interface doesn’t have get().

Also the classes that depend on environment like Slim\Http\Request::createFromEnvironment(Environment $environment) all require specific Slim\Environment class, not the interface. So trying to replace environment service with something that implements EnvironmentInterface would break the app.

It feels like either:

  1. EnvironmentInterface should be expanded to comprehensively describe a generic replaceable service, in analogue to ContainerInterface. Other components of the app should be changed to depend on the interface, not the implementation.
  2. Inline documentation should reflect the de–facto dependency on concrete Environment implementation, not the interface.
@Rarst
Copy link
Contributor Author

Rarst commented Mar 24, 2019

Hey, I noticed this was tagged as a workable issue... But I could use some feedback on which approach is desirable here?

@jdrieghe
Copy link

jdrieghe commented Apr 5, 2019

I made a small PR that implements one of your proposals: #2625.
If it's decided to go with this, I'm more than willing to do the same for the documentation.

@akrabat akrabat closed this as completed in f0718f6 Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants