Delegate quoteIdentifier operation to quoteIdentifierChain #233

alextech · 2017-03-11T06:47:03Z

Clean up identifier quoting. quoteIdentifierChain was doing the same thing quoteIdentifier was doing but without safety of using predefined escape tokens. In turn, quoteIdentifier was producing same result as quoteIdentifierChain by adding quotes around the token, which can be simplified by immediately converting identifier to array, pass it to identiferChain, and achieve same result. Overriding is now necessary only if there are really different rules by the engine. Eg. DB2 seems to be doing something very different.

This will help a lot with consistency in DB engines that have multiple identifiers in the path of a database objects. Eg. PostgreSQL always has trouble between choosing identifier vs chain. This can potentially solve a good portion of bad syntax generation for them.

No unit tests added because this is well covered by existing cases; nothing new or different is generated.

… produce same result. Enforce usage of preconfigured escape tokens.

froschdesign · 2017-03-18T10:16:01Z

src/Adapter/Platform/AbstractPlatform.php

@@ -1,9 +1,11 @@
 <?php
+


Please revert. The topic of this PR is "Delegate quoteIdentifier operation to quoteIdentifierChain" and not fixing the CS!

Are you reviewing old commits by accident? I think this is before you told me not to run outdated CS fixer in another PR. I redid all my other PRs same evening, including this one.

froschdesign · 2017-03-18T10:16:36Z

src/Adapter/Platform/AbstractPlatform.php

@@ -31,7 +33,7 @@
     */
    public function quoteIdentifierInFragment($identifier, array $safeWords = [])
    {
-        if (! $this->quoteIdentifiers) {
+        if (!$this->quoteIdentifiers) {


Please revert, the white space was correct.

froschdesign · 2017-03-18T10:16:47Z

src/Adapter/Platform/AbstractPlatform.php

@@ -54,8 +56,8 @@ public function quoteIdentifierInFragment($identifier, array $safeWords = [])
            $identifier .= isset($safeWordsInt[strtolower($part)])
                ? $part
                : $this->quoteIdentifier[0]
-                . str_replace($this->quoteIdentifier[0], $this->quoteIdentifierTo, $part)
-                . $this->quoteIdentifier[1];
+                .str_replace($this->quoteIdentifier[0], $this->quoteIdentifierTo, $part)


Please revert, the white space was correct.

froschdesign · 2017-03-18T10:17:06Z

src/Adapter/Platform/AbstractPlatform.php

@@ -105,18 +115,19 @@ public function getQuoteValueSymbol()
    public function quoteValue($value)
    {
        trigger_error(
-            'Attempting to quote a value in ' . get_class($this) .
+            'Attempting to quote a value in '.get_class($this).


Please revert, the white space was correct.

froschdesign · 2017-03-18T10:17:14Z

src/Adapter/Platform/AbstractPlatform.php

            ' without extension/driver support can introduce security vulnerabilities in a production environment'
        );
-        return '\'' . addcslashes((string) $value, "\x00\n\r\\'\"\x1a") . '\'';
+
+        return '\''.addcslashes((string) $value, "\x00\n\r\\'\"\x1a").'\'';


Please revert, the white space was correct.

froschdesign · 2017-03-18T10:17:20Z

src/Adapter/Platform/AbstractPlatform.php

    }

    /**
     * {@inheritDoc}
     */
    public function quoteTrustedValue($value)
    {
-        return '\'' . addcslashes((string) $value, "\x00\n\r\\'\"\x1a") . '\'';
+        return '\''.addcslashes((string) $value, "\x00\n\r\\'\"\x1a").'\'';


Please revert, the white space was correct.

froschdesign · 2017-03-18T10:18:29Z

src/Adapter/Platform/AbstractPlatform.php

+            $identifierChain = [$identifierChain];
+        }
+
+        if (!$this->quoteIdentifiers) {


White space after exclamation mark is missing.

froschdesign · 2017-03-18T10:19:02Z

src/Adapter/Platform/AbstractPlatform.php

+        foreach ($identifierChain as $key => $identifier) {
+            $identifierChain[$key] = str_replace($this->quoteIdentifier[0], $this->quoteIdentifierTo, $identifier);
+        }
+        $chainGlue = $this->quoteIdentifier[1].$this->getIdentifierSeparator().$this->quoteIdentifier[0];


White space before and after the dots is missing.

froschdesign · 2017-03-18T10:19:09Z

src/Adapter/Platform/AbstractPlatform.php

+        }
+        $chainGlue = $this->quoteIdentifier[1].$this->getIdentifierSeparator().$this->quoteIdentifier[0];
+
+        return $this->quoteIdentifier[0].implode($chainGlue, $identifierChain).$this->quoteIdentifier[1];


White space before and after the dots is missing.

froschdesign · 2017-03-18T10:25:07Z

@alextech

No unit tests added because this is well covered by existing cases; nothing new or different is generated.

Thanks for the hint!

froschdesign · 2017-03-18T10:26:48Z

src/Adapter/Platform/AbstractPlatform.php

@@ -115,19 +111,18 @@ public function getQuoteValueSymbol()
    public function quoteValue($value)
    {
        trigger_error(
-            'Attempting to quote a value in '.get_class($this).
+            'Attempting to quote a value in ' . get_class($this) .


Can we use sprintf here?

Yes we can. Was not sure if you wanted it saved in variable or not. Made it into parameter. And code standard for dot at beginning of line does not seem to apply here, since no equal sign so I put it at the end.

alextech · 2017-03-19T22:04:50Z

How does that one small sprintf() change lowers unit test coverage across all files?

ezimuel · 2017-12-04T16:31:11Z

@alextech you mentioned that this PR fixes some potential issue in SQL syntax generation. Can we found and example? We need to prove with a unit test that this PR fixes something. Now, we have the same unit test of before and this PR seems not necessary.

alextech · 2017-12-04T18:46:09Z

@ezimuel pretty much every quoteIdentifier call across the codebase when using PostgreSQL or SQL Server including userland usage in other libraries and application code

https://github.com/zendframework/zend-db/pull/247/files/bfb73c8bf4be5d98207a9911eca14aabdadd79c8#diff-0113773e7838ad267d6360c243edc298R235
Or #161 (Ignore the sequence part)

Or related random example from other library zendframework/zend-log#63,
some of my DDL PRs...

The reoccurring theme is the need to check database type and if an object path was given for every attempt to quote an identifier to decide if quoteIdentifier or quoteIdentifierChain should be used, both within zend db codebase, and in application code using zend db. Any place where this is forgotten, such as pull request I linked, results in crashes that @xorock brought up in several places. As I was investigating why that is, I noticed how both functions essentially do the same thing with minor difference so I consolidated the usage. The lack of unit tests in itself proves that the PR does not break any existing usage; the whole point is to fix this inconsistency that makes all existing code work as it did before, while not needing to check database type and existence of path in the future.

For explanation of what I mean by "object path", look last comment in #206 and a typical usage example in the issue description. ( I cannot remember if this PR can fix that issue directly, but its a step towards it, as with it, identifiers can be expressed as an array or if string given, explode on object path delimiter to create needed array, and be able to quote it without having to worry about it being chain or what)

weierophinney · 2019-12-31T22:44:34Z

This repository has been moved to laminas/laminas-db. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

Squash all commits in your branch (git rebase -i origin/{branch})
Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
Run the laminas/laminas-migration tool on the code.
Clone laminas/laminas-db to another directory.
Copy the files from the second bullet point to the clone of laminas/laminas-db.
In your clone of laminas/laminas-db, commit the files, push to your fork, and open the new PR.
We will be providing tooling via laminas/laminas-migration soon to help automate the process.

michalbundyra · 2020-01-16T19:24:50Z

This repository has been closed and moved to laminas/laminas-db; a new issue has been opened at laminas/laminas-db#77.

alextech added 2 commits March 11, 2017 01:36

Delegate quoteIdentifier operation to quoteIdentifierChain since they…

916f752

… produce same result. Enforce usage of preconfigured escape tokens.

Delegate quoteIdentifier operation to quoteIdentifierChain since they…

fe973b4

… produce same result. Enforce usage of preconfigured escape tokens.

froschdesign suggested changes Mar 18, 2017

View reviewed changes

froschdesign reviewed Mar 18, 2017

View reviewed changes

froschdesign assigned ezimuel Mar 18, 2017

froschdesign added the enhancement label Mar 18, 2017

froschdesign added this to the 2.9.0 milestone Mar 18, 2017

String formatter instead of concatenation.

70b37f3

alextech mentioned this pull request May 22, 2017

Fix AlterTable::dropConstraint() by allowing the use of decorator #247

Open

alextech mentioned this pull request Sep 26, 2017

Postgresql indexes #163

Open

alextech mentioned this pull request Nov 24, 2017

#186 [RFC][WIP] Sequence Support #187

Open

12 tasks

alextech mentioned this pull request Dec 4, 2017

Fixed invalid quoting for postgresql adapter #162

Open

froschdesign removed this from the 2.9.0 milestone Dec 7, 2017

michalbundyra mentioned this pull request Jan 16, 2020

Delegate quoteIdentifier operation to quoteIdentifierChain laminas/laminas-db#77

Closed

michalbundyra mentioned this pull request Jan 16, 2020

Postgresql indexes laminas/laminas-db#97

Closed

alextech mentioned this pull request Nov 9, 2021

Quoting behaves not as expected laminas/laminas-db#226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delegate quoteIdentifier operation to quoteIdentifierChain #233

Delegate quoteIdentifier operation to quoteIdentifierChain #233

alextech commented Mar 11, 2017 •

edited

Loading

froschdesign Mar 18, 2017

alextech Mar 19, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign Mar 18, 2017

froschdesign commented Mar 18, 2017

froschdesign Mar 18, 2017

alextech Mar 19, 2017

alextech commented Mar 19, 2017

ezimuel commented Dec 4, 2017

alextech commented Dec 4, 2017 •

edited

Loading

weierophinney commented Dec 31, 2019

michalbundyra commented Jan 16, 2020

Delegate quoteIdentifier operation to quoteIdentifierChain #233

Are you sure you want to change the base?

Delegate quoteIdentifier operation to quoteIdentifierChain #233

Conversation

alextech commented Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign commented Mar 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextech commented Mar 19, 2017

ezimuel commented Dec 4, 2017

alextech commented Dec 4, 2017 • edited Loading

weierophinney commented Dec 31, 2019

michalbundyra commented Jan 16, 2020

alextech commented Mar 11, 2017 •

edited

Loading

alextech commented Dec 4, 2017 •

edited

Loading