Store query params in driver exceptions

[BenMorel]

Q	A
Type	improvement
BC Break	no
Fixed issues	none

Summary

This is an attempt at #4325 (comment).

This PR removes the text representation of the bound parameters in the query exception message, and stores these params together with their types in the DriverException instead. This shifts the responsibility of (not) storing/displaying bound parameters explicitly to the consumer of the exception.

Just like #4325, this removes the dependency on ext-json (plus a couple lines of code!).

[BenMorel]

Oops, after running Psalm, I realized that DriverException is marked as immutable. I cannot easily change this as it cascades to other errors, which makes me believe that you do want to keep exceptions immutable.

Thus I only see 2 solutions:

• pass the params & types down to the ExceptionConverter and back to the DriverException; this would be really cumbersome but would work;
• change setQueryParams() and setQueryParamTypes() to withQueryParams(), that returns a copy of the exception; I've tried this path in 4ee8fe1 but the exception being uncloneable, this makes the implementation a bit of a mess: I had to suppress phpstan & psalm warnings, so I can't say I'm happy with this.

[BenMorel]

@morozov Please check this new version!

[morozov]

Looks like we're moving in the right direction.

[morozov]

@beberlei do you think we could include this into 3.0 instead of master? While it changes the behavior many developers may be used to, it reduces the possibility of leaking sensitive data by the DBAL which cannot be fixed w/o a BC break.

[morozov]

Logic-wise looks good to me, only the upgrade notes are missing. Could you try to retarget against 3.0.x?

[morozov]

Not sure which "the query" is being referred to since there's no query in the context. @greg0ire what would be the proper wording?

[morozov]

@doctrine/team-dbal, please be informed. With this change, the query and its parameters will be no longer part of the exception message but will be accessible via DriverException::getQuery(). It will help prevent leaking potentially sensitive data to logs, etc. but will require changes in the tooling to display these bits during development.

[BenMorel]

@morozov Rebased on 3.0.x, waiting for feedback on wording!

[morozov]

@BenMorel I believe the wording is not that critical but the upgrade notes are still missing.

[BenMorel]

@morozov Done!

By the way, shouldn't ExceptionConverter and DriverException::__construct() be internal?

[morozov]

By the way, shouldn't ExceptionConverter and DriverException::__construct() be internal?

That would make sense, I think. I believe all exception constructors should be internal (not in this patch) since only the DBAL is supposed to instantiate them.

[BenMorel]

That would make sense, I think. I believe all exception constructors should be internal (not in this patch) since only the DBAL is supposed to instantiate them.

Should I make at least these 2 @internal? Or we'll have to revert the changelog later.

[morozov]

Should I make at least these 2 @internal? Or we'll have to revert the changelog later.

Yeah, let's do, and update the upgrade notes accordingly. The wording changes look good to me as well, so it may be also the time to squash everything.

[BenMorel]

@morozov Done!

[morozov]

I'm sorry. I misread your question in #4387 (comment) and thought you were asking about constructors in both cases.

[BenMorel]

@morozov I fixed this in 06732c5, please let me know if everything's ok now and I'll squash.

[morozov]

Looks good.

[BenMorel]

Squashed!

[morozov]

Thanks, @BenMorel!