Quote create update and delete methods #552

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

mmuruev commented Mar 25, 2014

Add quote for field names.

Hello,

thank you for creating this pull request. However did not open it on the "master"
branch. Our Git workflow requires all pull requests to go through "master" branch
and the release masters then merge them back into stable branches, if they are
bug fixes.

Please open the pull request again for the "master" branch and close
this one.

Nevertheless I have opened a Jira ticket for this Pull Request to track this
issue:

http://www.doctrine-project.org/jira/browse/DBAL-847

We use Jira to track the state of pull requests and the versions they got
included in.

Owner

See http://docs.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/security.html#sql-injection-safe-and-unsafe-apis-for-user-input

We can't escape identifiers in this location because it would be a major BC break (any consumers that do escaping on their side would be affected).

@Ocramius Ocramius closed this Mar 25, 2014
mmuruev commented Mar 25, 2014

How about make it with addition parameter and disabled by default? Without escape DBAL make not to much sense.

Owner

@mmuruev context-based switches are a pain - if you are using the DBAL with this setting on and the ORM with a quoting strategy then it is going to crash badly. This needs to be re-designed, but can't be done in 2.x

mmuruev commented Mar 25, 2014

Obvious it contain less pain then do custom crunches for any field values inside client code. Also how it will affect ORM if it will have no idea about this option and will do it in old fashion way.

Owner

Obvious it contain less pain then do custom crunches for any field values inside client code.

Not really - it is trivial to write a small wrapper that does this for you

mmuruev commented Mar 25, 2014

Of course not you just have to remember use it everywhere. Also if make
public function update ($tableName, array $data, array $identifier, array $types - array(), $quoted= false); // Does it really bring pain?

Owner

Yeah, we can't clutter the connection with more parameters without breaking subclasses/wrappers.

mmuruev commented Mar 25, 2014

I can see that only class MasterSlaveConnection use Connection. And Just checked, additional parameter bring no harm because is have default value. Well what about use addition parameter in constructor? It receive array $params if add 'quoted' => bool pair it can be the solution.

Owner

I can see that only class MasterSlaveConnection use Connection

That's true only if you look at DBAL, and not at consumer libraries. The Connection class is re-implemented in a lot of larger (non-oss) projects.

mmuruev commented Mar 25, 2014

But that about __construct(array('quoted' = true), ...); It will harm no body. All interfaces will be in the same condition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment