Skip to content
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

Drop custom DBAL exceptions #245

Closed
wants to merge 1 commit into from
Closed

Drop custom DBAL exceptions #245

wants to merge 1 commit into from

Conversation

foxycode
Copy link
Contributor

@foxycode foxycode commented Mar 8, 2016

Related to #238

This was little difficult for me. I am not sure about few things. See my comments in code.

* Prepares an SQL statement.
*
* @param string $statement The SQL statement to prepare.
* @throws DBALException
* @return PDOStatement The prepared statement.
* @return Statement The prepared statement.
*/
public function prepare($statement)
{
$this->connect();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In parent, connect is not called. Is it here because of custom exception? Can I drop it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is indeed unneccesary. It was here only because older version of doctrine had it.

@foxycode foxycode changed the title Drop custom DBAL exception Drop custom DBAL exceptions Mar 8, 2016
@fprochazka
Copy link
Member

There is a bit of a problem with this. The panel and exception renderer require to know which connection generated the exception. Because for rendering of the exception, the Platform object of that connection is required.

So those methods that you've removed from Connection need to stay there, but changed, something like this

    public function executeQuery($query, array $params = array(), $types = array(), QueryCacheProfile $qcp = NULL)
    {
        try {
            return parent::executeQuery($query, $params, $types, $qcp);

        } catch (\Exception $e) {
            if ($this->panel) {
                $this->panel->queryFailed($e);
            }
            throw $e;
        }
    }

This way, the custom exceptions are gone, but we still can bind the exception with the relevant connection object.


And as the Panel is growing uncontrollably, I'd preffer breaking it into several classes.

  • Only the SQL panel stuff - which the part that is registered as SQLLogger and renders tracy bar panel
  • Only the exception bluescreen rendering stuff with context (SQL errors, metadata mapping errors, ...)
  • Only rendering context-less exceptions.

That way, we can register the second and third one in production without the first one.

The "context exceptions panel" must also be somehow passed to connection, so connection can mark the exceptions, and also passed to the custom PDOStatement that you've removed, so also that thing can mark the exceptons.

Does it make sense?


Also, it would be beneficial to look first how symfony is solving this - if they ignore the problem completely, or if they somehow mark the exceptions too.

@fprochazka
Copy link
Member

I wouldn't wanna drop the filling parameters back into the SQL, because I consider it way too comfortable, to be able to just copy the SQL statement and run it directly in Adminer.

@foxycode
Copy link
Contributor Author

foxycode commented Mar 9, 2016

@fprochazka I expected it wouldn't be that simple. I try to rework it.

That do you meant by your last comment?

@fprochazka
Copy link
Member

@foxycode well, there is always an option to simply drop the support for rendering SQL queries with parameters inlined, but I wouldn't wanna do that.

@foxycode
Copy link
Contributor Author

foxycode commented Mar 9, 2016

@fprochazka Ok, I understand. But wouldn't be simplier remove panel from this repo and move work to new repository now?

@fprochazka
Copy link
Member

@foxycode I don't see how it would make it simpler :)

The panel itself and context-free bluescreen renderers could in fact be separated right now probably, but the "exceptions bluescreen" would have to stay here, with the extended connection, since there would be no way otherwise to mark the exceptions

@foxycode
Copy link
Contributor Author

@fprochazka I was thinking about separating Tracy integration including extended connection to separate package, but now I see it's not that good idea. I'll think about this more a try to fix it.

@foxycode
Copy link
Contributor Author

@fprochazka I start with Panel reworking in separate PR. Can you please answer to other questions I had in code?

@fprochazka
Copy link
Member

I belive I've answered all your questions now :)

@fprochazka fprochazka added this to the v4.0 milestone Apr 5, 2016
@fprochazka fprochazka force-pushed the master branch 4 times, most recently from 6ac82e7 to 06fd1e8 Compare May 15, 2017 14:37
@foxycode foxycode closed this by deleting the head repository Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants