-
Notifications
You must be signed in to change notification settings - Fork 0
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
Chrome logger #9
Comments
We do have this integrated in our stack - I'm not sure how though. Digging through some code now to see how it was integrated - maybe there's another component that could be moved to this package... |
I looked into it - we don't even have a component for this, because it's so trivial. class ChromeLoggerMiddleware implements ServerMiddlewareInterface
{
private $logger;
public function __construct(ChromeLogger $logger)
{
$this->logger = $logger;
}
public function process(ServerRequestInterface $request, DelegateInterface $delegate)
{
return $this->logger->writeToResponse($response);
}
} In our stack, it's a little bit different because we have more dependencies. But I suppose this simple implementation would work for most cases, so maybe I should just check that in and add a simple test? |
Oh, now I recall why this isn't in there - for the same reason I hold off anything else that depends on PSR-15. It's unstable. That means I can't tag a I don't want the logger package itself to have a BC break for no reason other than the middleware-signature changing, so if I were to publish the middleware now, it would need to be a separate package, so that the break is there, an not in the logger component itself... Hmm.... |
Ok, I see. I recomend to tag the version |
In my very own opinion, Or in other words: Why must use Symfony\Component\HttpFoundation\Response;
$response = new Response();
// this needs getHeaderValue() to become public:
$response->headers->set(ChromeLogger::HEADER_NAME, $chromeLogger->getHeaderValue()); – Just my two cents. |
Well, there's emitHeader for non-PSR projects. If you want Symfony-integration, then getHeaderValue is It's designed for PSR-7 and I'm not going to BC break to remove that. I doubt anyone would care if the PSR-7 interfaces get installed - if that makes the package unattractive, then, meh.
@oscarotero I agree, but don't want to inherit BC breaks from PSR-15, so I'll wait until it's stable. I know that BC are allowed in |
In my experience, both npm and composer are semver compilant. For example, a version |
I believe @oscarotero is right. For example, see madewithlove/elasticsearcher ^0.4.0 You can also test npm semver at https://semver.npmjs.com |
Guys, I know how npm and Composer work :-) What I'm saying is, I believe they attribute more meaning to Per semver.org:
I take this as meaning "any release may contain breaking changes".
I take this as meaning "the public API isn't defined at all prior to 1.0.0", and "version numbers prior to this release are not dependent on the public API". If that's not what they meant, it's extremely ambiguous, confusing and poorly written. Furthermore, sections 6, 7 and 8, which define the version increment rules, define these explicitly for
Well, they're not not compliant, they're just assigning a more specific meaning to
Well, exactly - according to my understanding of sections 4 and 5, under semver, Essentially, for
If the second digit is the minor version, and you think npm and Composer work according to the spec, essentially they're saying "every release should be incompatible with the previous". I don't know, maybe it's just really poorly written, but I had enough of the ambiguities and complexity, and finally decided to rewrite the damn thing - our developers need clear guidelines they can actually follow. :-) |
Yes, you are right, the
|
This is not entirely true, Maybe semver.org is a bit confusing about the Anyway, I still don't understand why not tag the 0.1.0 version of ChromeLogger although it does not include support for PSR-15. At least, is better than relying in a |
@mindplay-dk created this package to send log messages to google chrome.
Maybe, the package should provide the middleware, but I leave it here anyway, just in case.
The text was updated successfully, but these errors were encountered: