Add support for subscription to certain life cycle events #47

swquinn · 2017-03-02T04:00:39Z

This commit adds support to the sentry-symfony library for certain
"lifecycle" events that provide, possibly simpler, extension points for
some of the exception capturing process. Specifically two events:
SET_USER_CONTEXT and PRE_CAPTURE.

SET_USER_CONTEXT can be used to override the default user context
that is captured in the ExceptionListener during onKernelRequest
PRE_CAPTURE can be used to add tags, or other contents, to the
client based on the event and the exception pending capture.

I've also added a "Customization" section to the README to make clear
two options developers have to customize the Sentry context.

This commit adds support to the sentry-symfony library for certain "lifecycle" events that provide, possibly simpler, extension points for some of the exception capturing process. Specifically two events: SET_USER_CONTEXT and PRE_CAPTURE. - SET_USER_CONTEXT can be used to override the default user context that is captured in the ExceptionListener during onKernelRequest - PRE_CAPTURE can be used to add tags, or other contents, to the client based on the event and the exception pending capture. I've also added a "Customization" section to the README to make clear two options developers have to customize the Sentry context.

swquinn · 2017-03-02T04:02:13Z

In addition, this PR probably meets the request presented in #7 for giving projects a way to configure the user context beyond the default ExceptionListener (which could always just be overriden).

swquinn · 2017-03-03T11:48:24Z

Thinking more on this now, a few days after opening the PR, the only thing that I might want to change in the events is that PRE_CAPTURE may be too broad since it is triggered for both console and kernel exceptions which could make setting subscribers a little annoying. This could be broken out to PRE_CAPTURE_KERNEL and PRE_CAPTURE_CONSOLE to make writing event subscribers/listeners more friendly.

I'm not really sure how I feel about that. I like the simplicity of just a single PRE_CAPTURE event, but at the same time see its downside.

I guess, worse case scenario with a single PRE_CAPTURE event, a developer could just write:

class MySentryEventSubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return array(
            // ...
            SentrySymfonyEvents::PRE_CAPTURE => 'onPreCapture',
        );
    }

    // ...

    public function onPreCapture(Event $event)
    {
        if ($event instanceof GetResponseForExceptionEvent) {
            $this->handlePreCaptureForKernelException($event);
        }
        elseif ($event instanceof ConsoleExceptionEvent) {
            $this->handlePreCaptureForConsoleException($event);
        }
    }
}

Thoughts? Feedback?

swquinn · 2017-04-13T15:44:04Z

Any idea when this can be merged in?

Jean85 · 2017-04-28T14:40:50Z

src/Sentry/SentryBundle/EventListener/ExceptionListener.php

@@ -41,6 +47,7 @@ class ExceptionListener
    public function __construct(
        TokenStorageInterface $tokenStorage = null,
        AuthorizationCheckerInterface $authorizationChecker = null,
+        EventDispatcherInterface $dispatcher = null,


Shouldn't we take the dispatcher for granted? Why the = null?

I'm having trouble remembering why I did this explicitly here. I think it may have been for compatibility cases where someone might have already overridden or defined their own exception listener definition around the ExceptionListener in this code base. I didn't necessarily want to break the signature of someone's code if it didn't explicitly include the $dispatcher.

Although, if you think we should take the presence of the dispatcher for granted, I'm all for removing the nullability of it and the null checks.

Upon further inspection of the code, I think I may have also been following precedent since other dependencies (including the Raven client) have the = null. Then again, I guess you're assigning a client if the client is null so the argument of: "It seems like if we're talking about taking the dispatcher for granted, shouldn't we also take the Raven client for granted as well" actually falls over since there should always be a client.

As mentioned I'm happy to make a code change here, just let me know. :)

I agree that I would change both arguments; also, the order of the arguments seems strange to me too! But I would wait for @dcramer approval before doing this change.

You bring up a good point about the order--especially if I were to allow for the nullability of the argument--I probably should have tacked it on as the last argument if I were really doing this to make it as much of a non-breaking change as possible. Before I make further changes, I'll wait for @dcramer's input though. Thanks @Jean85!

i'm lacking a lot of context at this point (been a while since I've touched any of this, and I've never used Symfony). My biggest concerns are always breaking changes and compatibility with various versions of PHP/Symonfy. As long as this doens't cause any concern there, I'll defer ro you both.

Ok thanks @dcramer!
If BC is an issue, and we consider this class as part of the public API, we shouldn't change the order of the arguments, and add the new one as the last one. Maybe in the next major bump we can refactor this.

PS: I've sent you and email ;)

Will do! I'll refactor that sometime today and push up the changes. Thanks for your input guys!

Jean85 · 2017-04-28T14:43:27Z

I would advise against the dual PRE_CAPTURE events, the piece of code that you posted is good to do that difference, maybe you can append it to the README's changes.

swquinn · 2017-04-28T15:48:19Z

Sounds good. I'll add the code posted above to the README as a way of handling the difference in exception events. Thanks for the suggestion @Jean85 !

swquinn · 2017-04-28T16:15:45Z

@Jean85 I added the snippet of code above for handling the difference between the GetResponseForExceptionEvent and ConsoleExceptionEvent with a bit of added context to the README.md, per your suggestion. Thanks!

dcramer

In general this looks good to me.

dcramer · 2017-05-03T17:26:14Z

README.md

+use Symfony\Component\Security\Core\Authentication\Token\Storage\TokenStorageInterface;
+use Symfony\Component\Security\Core\Authorization\AuthorizationCheckerInterface;
+
+class MySentryExceptionListener


would it make sense to have the default example extend the builtin?

I've given this a longer thought, and something popped up: we should enforce the presence of all the three methods (onKernelRequest, onKernelException, onConsoleException) in any substitute, otherwise the bundle will fail, due to the fact that service.yml wires the listener to all three.

I see two solutions:

remove the explicit binding of the methods from the YAML, and use the getSubscribedEvents() to wire the listener

create an SentryExceptionListenerInterface and make the injection of the bundle fail if that's not implemented by the %sentry.exception_listener% class

I would prefer the latter, because it's more guided for the end users: it would give and explicit and clear error when the listener is misconfigured, it requires explicit action to disable one of the three behaviors, and it hints toward extending the original class if only a small modification is required.

I really like this proposal (as I indicated in my previous comment), I think I had considered using an interface before but had hesitated to avoid adding what I thought could have been seen as unnecessary code at the time of the original PR.

Regardless, I've made the first half of the changes here, in this commit: e075f15

It has been a little while since I've done Symfony (I've been bouncing around languages recently because of work), but I'm not sure if I've done something in the service definitions before that would enforce a definition's implementation of an interface. I'll try to search for this today, but if you have the time could you point me to code examples or documentation in Symfony? Thanks!

dcramer · 2017-05-03T17:28:06Z

src/Sentry/SentryBundle/EventListener/ExceptionListener.php

@@ -41,6 +47,7 @@ class ExceptionListener
    public function __construct(
        TokenStorageInterface $tokenStorage = null,
        AuthorizationCheckerInterface $authorizationChecker = null,
+        EventDispatcherInterface $dispatcher = null,


i'm lacking a lot of context at this point (been a while since I've touched any of this, and I've never used Symfony). My biggest concerns are always breaking changes and compatibility with various versions of PHP/Symonfy. As long as this doens't cause any concern there, I'll defer ro you both.

This moves the $dispatcher argument to the end of the constructor arguments to reduce BC failures.

swquinn · 2017-05-04T19:40:54Z

I've refactored the code so that the $dispatcher is declared last in constructor's dependency list as discussed above. I've also made changes to the README.md example, though instead of explicitly showing the example extending the ExceptionListener defined in the library, I made a note of it the possibility of, instead of writing your listener from scratch, you can extend from the default to inherit default behavior where it makes sense.

Jean85 · 2017-05-05T07:25:07Z

Thanks for the hard work! I've found an issue with the replacing of the listener, can you address it? I've explained myself in the inline comment here

Addvilz · 2017-05-05T09:23:21Z

src/Sentry/SentryBundle/Event/SentryUserContextEvent.php

+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+
+/**
+ * The SentryUserContextEvent.


I see these kinds of docblocks a lot. For me, this is just extra lines, as they don't really comment or document anything. We see the class name and I don't think it's any good to repeat that here in comment. You can keep the @author and @package but I would remove these kinds of comment lines that don't really explain anything - across the entire PR. Same with methods below - they just repeat something we already clearly see in the code.

@Jean85 @dcramer thoughts?

I agree. Comments should be nearly always become irrelevant thanks to clean code..

Good call, I cleaned up and removed comments that seemed to be the most unnecessary.

Please see commit: bc1648f

Addvilz · 2017-05-05T09:24:39Z

src/Sentry/SentryBundle/EventListener/ExceptionListener.php

@@ -81,6 +90,11 @@ public function onKernelRequest(GetResponseEvent $event)

        if (null !== $token && $this->authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED)) {
            $this->setUserValue($token->getUser());
+
+            if (null !== $this->dispatcher) {


You can simplify this to !$this->dispatcher. Up to you tho.

It's the opposite, no !. In any case, I don't think it's an important change.

Disregard this, it's Friday and I didn't think clearly 👯‍♂️

Addvilz · 2017-05-05T09:26:47Z

src/Sentry/SentryBundle/Resources/config/services.yml

@@ -17,6 +17,7 @@ services:
          - '@?security.authorization_checker'
          - '@sentry.client'
          - '%sentry.skip_capture%'
+          - '@event_dispatcher'


You are adding a mandatory reference to service but permit service to be null in class itself. This is contradictory because Symfony will error out if event_dispatcher is not present in this case. Either make this service optional or disallow null in the constructor.

I already commented on that up above. I agree, but the service should be taken for granted since we require symfony/symfony, so I would prefer no nullable in the class.

Sorry missed it. Yes, I reckon non-nullable parameter in the constructor would be the correct way to go then.

I made the recommended change to the code (removing the null checks since we're requiring the event dispatcher). I guess I wasn't really thinking, and I also didn't notice the @? prefix for services. I don't know if I've ever used that before, but I do most of my service definitions in XML in my own projects, so it is possible I just never knew about that with the YAML. I'm assuming that the @? allows for a non-mandatory reference and null values to be passed into the function/constructor/whatever?

Any way, the changes can be found in this commit: d921cc5

Yes, the @? is used to silence failures for missing references and passing a null instead.

swquinn · 2017-05-05T11:21:27Z

Just seeing the comments now, haven't had a chance to go through all of them. I'll try to tackle them some time today and make the changes requested. I like the idea of adding the interface as well, to enforce those functions so I'll do that. Thanks for your attention on this!

The SentryExceptionListenerInterface serves as a contract enforcing the shape of the exception listener. The shape is important because, for Sentry exception listeners, it is necessary to capture exceptions coming from both web requests (GetResponseForExceptionEvents) and the console (ConsoleExceptionEvents).

Originally the EventDispatcher was added with the intent that it could be nullable, however a disconnect in how it was coded in the ExceptionListener and how it was being injected left the code in a sort of paradox with itself: we were always requiring it, but we were null-checking it in the code as if it were possible to leave the dispatcher null. Ultimately, after a few other changes and suggestions from other participants in the code review it made the most sense just to require the EventDispatcher and, within the code, make the assumption that it would always be there rather than checking to see if there was a null value.

swquinn · 2017-05-05T12:21:19Z

I've made a number of changes as requested by both @Jean85 and @Addvilz and I think that I got most of them. I'm doing this from a machine I don't have set up for running Symfony tests right now (let alone PHP), so I hadn't been able to run the style-checker before committing. Sp, I apologize for the various stupid-prone build errors that required a few additional commits towards the end there.

I think the only change I haven't been able to make is one @Jean85 requested which is to make the injection of the bundle fail if the %sentry.exception_listener% class doesn't implement the newly added interface.

I'll try to do some research into how to do this, but its not something I've commonly done--or if it is I'm simply forgetting how to do it--and would appreciate any advice you guys have on handling that!

Jean85 · 2017-05-05T12:43:27Z

src/Sentry/SentryBundle/EventListener/ExceptionListener.php

@@ -81,6 +90,11 @@ public function onKernelRequest(GetResponseEvent $event)

        if (null !== $token && $this->authorizationChecker->isGranted(AuthenticatedVoter::IS_AUTHENTICATED_REMEMBERED)) {
            $this->setUserValue($token->getUser());
+
+            if (null !== $this->dispatcher) {


Since the dispatcher is always there, this check is now futile, needs to be removed

Shoot! Missed that line of code. This has been removed in the following commit: 812007b

Jean85 · 2017-05-05T12:47:09Z

src/Sentry/SentryBundle/Event/SentryContextEventInterface.php

@@ -0,0 +1,10 @@
+<?php


This interface is currently unused, it's empty and it has no usages apart from the implementing single class. Maybe we should delete it.

Hmm. Good call. I thought I was using that for something else--even if it wasn't in this bundle's code per-se. Although, I could have sworn I was using that as a contract for examples in the README.md file but clearly I'm not.

I guess if we find ourselves in need of a context event interface, we can always add it back in at a later point in time. Before I remove it though, I'm going to see if I can't find why I thought I needed this. Thanks for pointing this out!

This still needs to be deleted.

Jean85 · 2017-05-05T13:05:17Z

@swquinn I think I can help you toward the validation: you should change the Configuration class like this:

                ->scalarNode('exception_listener')
                    ->defaultValue('Sentry\SentryBundle\EventListener\ExceptionListener')
                    ->validate()
                    ->ifTrue($this->getExceptionListenerInvalidationClosure())
                        ->thenInvalid('The "sentry.exception_listener" parameter should be a FQCN of a class implementing the SentryExceptionListenerInterface interface')
                    ->end()
                ->end()
[...]
    /**
     * @return \Closure
     */
    private function getExceptionListenerInvalidationClosure()
    {
        return function ($value) {
            $implements = class_implements($value);
            if ($implements === false) {
                return true;
            }

            return ! in_array('Sentry\SentryBundle\EventListener\SentryExceptionListenerInterface', $implements, true);
        };
    }

I'm not sure this works, so probably you should write a few tests about this in the ExtensionTest

Per suggestions from others in the PR, and with their help, I've added in a check that will validate that the exception listener implements the required interface. In addition, I've updated the tests and added a new test to verify that this works as expected.

Jean85 · 2017-05-16T08:14:05Z

Thanks @swquinn, you did an astounding job! The only remaining fix is to delete the unused interface. Once you do that, I will merge this.

swquinn · 2017-05-16T12:38:42Z

Thanks @Jean85! I made the changes last night but didn't get a chance to comment (it was way too late! Hah!). Thanks also for pointing out that the interface still needed to be deleted. I thought I had already done that.

Also, I noticed that there are conflicts between this branch and master's README.md file. Do you want me to fix that by merging master into this PR's branch? I just want to make sure I'm going about it the right way for this project.

Jean85 · 2017-05-16T13:01:58Z

If you have time to solve the conflicts now, go for it! Otherwise, I will do that later this afternoon before merging.

swquinn · 2017-05-16T13:34:46Z

I will try to do this shortly after getting into work this morning! Should be fixed in the next couple of hours. Thanks!

swquinn · 2017-05-16T16:46:27Z

Got to the conflict resolution a little later than I thought I would. I merged master in to resolve the conflict with README.md, Travis seems to be running a bit slow though. I'll check back in to verify all the checks completed successfully!

Jean85 · 2017-05-17T07:19:04Z

That's all green, good!

Thanks again for your contribution, it was a long stretch but it was worthwhile!

swquinn added 3 commits March 1, 2017 23:04

Fix bad white space that failed due to PHP CS checker

9a5aec3

Use getMock instead of createMock for compatibility with PHPUnit

50b1ca7

Remove unneccessary space for marker interface definition

ebe0e3c

swquinn mentioned this pull request Mar 2, 2017

Set Current User #7

Closed

duxet approved these changes Mar 30, 2017

View reviewed changes

Jean85 suggested changes Apr 28, 2017

View reviewed changes

Include PRE_CAPTURE example in README.md

4a4ff4b

dcramer reviewed May 3, 2017

View reviewed changes

swquinn added 2 commits May 4, 2017 13:29

Change order of arguments for exception listener

cd17601

This moves the $dispatcher argument to the end of the constructor arguments to reduce BC failures.

Change example exception listener and add note

f7c8d8d

Addvilz suggested changes May 5, 2017

View reviewed changes

swquinn added 5 commits May 5, 2017 07:43

Clean up comments, remove unnecessary comments

bc1648f

Fix failing build due to missing public modifiers on interface

0ec27dd

Fix extraneous new line causing linter to fail

5068851

Jean85 suggested changes May 5, 2017

View reviewed changes

Remove null check that was missed in previous commit

812007b

Remove currently unnecessary interface

5eb1a36

Jean85 approved these changes May 16, 2017

View reviewed changes

Merge branch 'master' into add-support-for-event-subscribers

78ebf11

Jean85 merged commit 0d2a008 into getsentry:master May 17, 2017

Jean85 added a commit that referenced this pull request May 17, 2017

Add info about #47 in the changelog

453b446

Jean85 mentioned this pull request May 17, 2017

Fix small issue and update changelog #56

Merged

Add support for subscription to certain life cycle events #47

Add support for subscription to certain life cycle events #47

Uh oh!

Conversation

swquinn commented Mar 2, 2017

Uh oh!

swquinn commented Mar 2, 2017

Uh oh!

swquinn commented Mar 3, 2017

Uh oh!

swquinn commented Apr 13, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swquinn Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swquinn Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jean85 commented Apr 28, 2017

Uh oh!

swquinn commented Apr 28, 2017

Uh oh!

swquinn commented Apr 28, 2017

Uh oh!

dcramer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swquinn commented May 4, 2017

Uh oh!

Jean85 commented May 5, 2017

Uh oh!

Addvilz May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jean85 May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swquinn commented May 5, 2017

swquinn Apr 28, 2017 •

edited

Loading

swquinn Apr 28, 2017 •

edited

Loading

Addvilz May 5, 2017 •

edited

Loading

Jean85 May 5, 2017 •

edited

Loading

swquinn commented May 16, 2017 •

edited

Loading