hard coupling to zend/diactoros and slim
ppetermann opened this issue · 11 comments
the composer.json requires zend-diactoros, and require-dev's slim
PhpDebugBarMiddleware.php actually directly uses zend-diactoros, and makes use in an instanceof comparison of Slims Uri class.
- the uses of zend-diactoros seem to be about building the response, this should be done through a Psr-17 factory, it should then also be moved to the require-dev's
- the use of slim framework seems to be to deal with an incompatibility (which should go away with slim/http) which would be better worked with if the slim uri was wrapped into psr-7 compatible class, which would do the translation, and only used when used within a slim context.
I agree with @ppetermann , hard coupling creates more issues than solves.
I'm using a new version of zend/diactoros (2.0) and I can not install this package with error:
Your requirements could not be resolved to an installable set of packages.
We can collaborate on this issue and make a PR for it. @snapshotpl will you merge that PR?
@spotman no problem man :) However first of all I prefer to release fix with support zend-diactoros 1 and 2. Your PR will be very welcome!
@snapshotpl Thanks for quick response! I need some time to check your lib and to dive in the code. I'll let you know when I'll be ready to start working on this issue.
@ppetermann I think slim dependency isn't a problem since is in require-dev. I don't see any benefit for end-user to create wrapper for slim users. My goal was create package working out-of-the-box.
I'm sorry, but I have no idea why having slim in require-dev would fix the hard coupling issue your package has.
You use Slim here, aliasing it as SlimUri
https://github.com/php-middleware/phpdebugbar/blob/master/src/PhpDebugBarMiddleware.php#L13-L17
You use the aliased SlimUri here:
https://github.com/php-middleware/phpdebugbar/blob/master/src/PhpDebugBarMiddleware.php#L132-L142
which is used here:
https://github.com/php-middleware/phpdebugbar/blob/master/src/PhpDebugBarMiddleware.php#L110
which is called here:
https://github.com/php-middleware/phpdebugbar/blob/master/src/PhpDebugBarMiddleware.php#L40
which is basically your main function, being called from __invoke.
thus, you've created a dependency which means slim must be installed in order for your lib to work. Which means:
everywhere else where your code is run, but slim is not explicitly installed, your lib will fail hard.
Which means:
- slim being in require-dev is a mistake, as it definitely needs slim during runtime
- everyone who is not using slim needs to have slim installed just to use your middleware.
slim dependency is define only in require-dev because I need it in tests. However if your code base missing slim, there is no problem because instanceof doesn't need autoloading (more here http://php.net/manual/en/language.operators.type.php)
apologies, I wasn't aware that use and instanceof both don't need the class to be existing.
However I'd still consider abstracting it away.
@ppetermann :)
Can you make PR for that? Also look at #27
@snapshotpl #27 seems a great improvement 👍
I'll see if i can prepare a PR as soon as i have some spare time for that, but its not on the highest priority looking at all the things I got on my plate right now - sorry for that.
since we support psr-17 and slim is not required to install to lib work I think that we can close this issue