search

dbtlr / php-airbrake

A PHP 5.3 library for sending errors to the Airbrake.io service.
http://airbrake.io
MIT License
121 stars 44 forks source link

Update configuration options after calling EventDispatcher::start #28

Closed leonszkliniarz closed 9 years ago

leonszkliniarz commented 10 years ago

My problem basically boils down to the configuration options not being accessible after calling EventDispatcher::start(). While I can understand that letting me mess with the Client's internals at runtime isn't necessarily a good idea, there's two cases in particular where this is causing me pain.

I'm instantiating the library as part of my bootstrapping process (I want it loaded and running as early as possible). Controller/Action dispatching hasn't happened yet so I can't populate the controller/action params until later on in the startup process. It would be nice to be able to do this after the dispatch has happened.

The other issue I'm having is that post parameters sometimes contain sensitive information - the use of a server side payment integration means that there are times when $_POST is going to contain credit card details. I have no way of removing these from the error reports since the whole of $_POST is copied into the inaccessible Configuration object upon calling EventDispatcher::start().

Now I could just ignore EventDispatch::start and go through the trouble of creating my own Client, Configuration and EventHandler objects and then just update the Configuration object as I need to (relying on how php passes objects by reference to be able to change the values), however doing this would also cause problems. The EventDispatcher class seems to have been written with the intention of being a singleton - creating it without going through the static start method would leave the static $instance variable as null. This would cause the onShutdown and reset methods to break.

dbtlr commented 10 years ago

@leonszkliniarz You're totally right. When I wrote this it was in the context of a Symfony2 application and it was able to get all of its configuration right at the beginning. You should absolutely be able to add in more data about the user and additional meta information as it makes sense.

To that end, I've added a Client::getConfiguration() option that should allow this to be more easily accessible.

In regard to filtering the POST variables, that is definitely something that should be added. As this isn't a project that I'm actively using/maintaining, I'm reticent to make a bigger change like that, given that I don't have a readily available test case. If you'd like to submit a pull request that adds a 'filteredKeywords' array that act on the POST array on load, I'd be more than willing to review it and merge it into here.

leonszkliniarz commented 10 years ago

Thanks for your reply (and quick fix!), I was a bit worried I was coming off as a bit "everything is terrible, do my work for me, waah" in my bug report - I was trying to remove any waffle and just keep it to the important details. Aside from the issues with the configuration I've mentioned, absolutely everything has been a breeze and I've been really happy with it so far.

The only suggestion I would make with your fix is that EventDispatcher::start returns the EventDispatcher instance, so there's no way to get access to its internal Client object to call its getConfiguration method. Quickest and easiest thing I can think of to get around this would be to add a getClient method on the EventDispatcher.

Anyway! I'll definitely get a merge request over to you in the next few days to sort out the sensitive post parameters. I think it would probably be better to make the filter work on post rather than on load as it would allow people (me) to specify what constitutes sensitive information as it becomes known rather than having a banned list of $_POST key names.

--EDIT--

Ignore the spiel above about the getClient method. I only looked at the commit registered against this bug and didn't look at the rest of the commits you did yesterday. Having just cloned the repo to do a fork I've just noticed my error.

dbtlr commented 10 years ago

If you wanted to make it up to me, you could give my new 1.1.0rc1 (branch: travis) release a try. :)

I cleaned things up and bit and added in automated testing. I'm working up a plan to overhaul the backend a bit, but want to get some solid testing in there first. Everything should be working, but given the horrible state of testing that I left this in 3 years ago when I wrote it, I don't have a ton of confidence that I didn't break anything super obvious. :)