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

Fix Statement class passing bound objects to SQL logger #127

Merged
merged 4 commits into from
May 4, 2012

Conversation

jimcottrell
Copy link
Contributor

This commit saves converted parameter values in bindValue() so the later call to the SQL logger in execute() will not pass bound objects without type information.

…hout type information: save parameter's converted value
@stof
Copy link
Member

stof commented Apr 4, 2012

@jimcottrell what is the issue you are trying to fix ? There is no link to a ticket in the issue tracker and no tests in your pull request

@jimcottrell
Copy link
Contributor Author

@stof I suppose in my mind the issue was pretty clear, but essentially my problem is that if you want to do anything with bound parameters from an SQL logger (besides echo them, of course), Statement provides no type information to the logger. That is, only parameters are saved, not types.

So, if you bind, say, a DateTime object with an appropriate type set, the query itself will run fine because of course the DBAL type system knows what to do, but the logger just gets an object as one of the parameters and no type to tell it what to do with the parameter.

There are really two solutions: 1) save type information as well, or 2) save the converted parameter value. I chose #2 because I'm unconvinced that where SQL logging is concerned the original parameter is as important as what's actually getting passed to the database, but neither option is complicated, and in retrospect, #1 is really the more robust option if people do need to see the original parameter objects when logging. I suppose it depends on whether you consider the purpose of the logging system to record what DBAL sees or what the DB sees when all is said and done.

Looks like issue DBAL-248 was automatically created, if you'd prefer to have this conversation there.

@stof
Copy link
Member

stof commented Apr 4, 2012

@jimcottrell the logger also receives the types

@stof
Copy link
Member

stof commented Apr 4, 2012

@jimcottrell Btw, Symfony2 uses the types of the parameters to be able to store the queries in a serializable way but to try to keep as much as information as possible to be able to run the query again in most cases (well, we don't run the query itself but we explain it): https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Doctrine/DataCollector/DoctrineDataCollector.php#L117

@jimcottrell
Copy link
Contributor Author

@stof

lib/Doctrine/DBAL/Statement.php, line 125:

    public function execute($params = null)
    {
        $logger = $this->conn->getConfiguration()->getSQLLogger();
        if ($logger) {
            $logger->startQuery($this->sql, $this->params);
        }

        ...

There are no types passed here.

@jimcottrell
Copy link
Contributor Author

@stof This is the more robust way to do it then. I'm still a bit new to Git, so if you are interested in pulling this, I'm not sure if it would be best for me to create a new request so that the original commit isn't a part of the history?

@stof
Copy link
Member

stof commented Apr 4, 2012

@jimcottrell you should reset the type at the end of the execute method when the params are reseted.

@jimcottrell
Copy link
Contributor Author

@stof Oops, yes, you're absolutely right about the reset.

beberlei added a commit that referenced this pull request May 4, 2012
Fix Statement class passing bound objects to SQL logger
@beberlei beberlei merged commit b0f5280 into doctrine:master May 4, 2012
@jimcottrell jimcottrell deleted the feature-statement-fix branch January 29, 2013 22:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants