[#3170] Adds error to logger and executes stopQuery even on exception.

[jcchavezs]

Q	A
Type	improvement
BC Break	no
Fixed issues	#3170

Summary

This PR adds the stopQuery error for when the query fails and in that case, pass the exception to the stopQuery($ex).

It is important to mention that this does not break existing clients as the interface remains the same while clients can retrieve the exception by using func_get_args.

Ping @morozov

[morozov]

@Ocramius this is something to consider for #3156. In fact, my biggest concern is still that the DBAL's logging API is not extensible (it doesn't follow the open/closed principle). If we decide to log some other query execution details (e.g. number of returned/affected rows), we'll have to modify the SQLLogger signature again.

A more extensible solution would be to extract some Connection methods like executeQuery() and executeUpdate() into a smaller interface which you could implement the way you want and log anything you want about the queries.

Do you have any ideas?

[jcchavezs]

@morozov @Ocramius scrutinizer is failing due to the method that is being called with the $ex parameter (can be solved adding a comment specific to Scrutinizer /** @scrutinizer ignore-call */ ) and because of something unrelated to the changes. Not sure if I should fix them.

[jcchavezs]

@morozov any chance we can merge this quick fix here and continue the discussion about the new logger interface in the other isse?

[Ocramius]

This assumes that SQLLogger#stopQuery() accepts an exception.

IMO this shouldn't go through:

1. the exception parameter is not defined: that's a mess to deal with from a BC perspective (can't add a parameter, have to remind that implementation may have a parameter)
2. the query didn't stop: it failed! stopQuery() is not what is supposed to be called, but rather failed($failure) or such

This is not really a fix, and rather introduces more issues instead.

[jcchavezs]

@Ocramius that is true. My initial idea was tro create a new interface extending the existing one that would allow us to do so. I updated my PR accordingly.

[jcchavezs]

I believe the new interface should be called `SQLLoggerExtended` maybe?

[jcchavezs]

@morozov @Ocramius I did some changes to the PR. Basically now there is a new interface that includes all new methods and then the Connection detects whether the logger fullfill that SQLLoggerExtended before calling the method.

[jcchavezs]

@Ocramius @morozov this is green now, would you consider to merge it?

[morozov]

@jcchavezs the major problem with the proposed solution is that instead of having one non-extensible logger we have two non-extensible loggers and a bunch of new if-statements.

Let's say tomorrow I want to log how many rows the $stmt selected or updated. I'll have to introduce another interface and more if-statements.

We should either implement something like described in #3174 (comment) or just implement the ability to log exceptions in a non-breaking way.

[jcchavezs]

I totally agree we need a more coherent way of logging but with the current setup this is IMO the only way to accept errors in logger without breaking existing clients.

I believe the logger discussion should be kept in the other issue and the release will come in probably a major change. This is an minor change and even if we deprecate or remove SQLLoggerExtended it won't break clients.

Any idea on how we could do it in a better way without requiring a breaking change in logger @morozov? Any thoughts @Ocramius?