Introduce Enclose names feature. #27

Metalaka · 2015-06-19T09:42:46Z

Based on #7 (New feature : enclose names)

This PR will allow use of keywords and resolve some case sensitivity problems for table, column and alias names in the query builder.
The PR don't enclose columns names in WHERE and HAVING clause.
The feature is disabled by default.

The Dml interface have 3 new methods: enable/disable feature and set symbol.
A new EncolseNames abstract class is used by Where and Insert classes to provide the code of this 3 methods.

I did this PR because PostgreSQL need enclosed columns names to be case sensitive.
PS: maybe a method isEnabled is missing ?

Hywan · 2015-06-22T11:11:37Z

@osaris Feel free to choose anoter assignee if you can't follow and review this PR.

osaris · 2015-06-22T12:20:25Z

Hello,

Good idea but why don't you enclose names in WHERE / HAVING ? The problem is the same no ?

Metalaka · 2015-06-22T13:37:16Z

HI,
Simply because where and having method take in parameter expressions and not columns name.
To support WHERE and HAVING we need to find the column(s) in each expression.

osaris · 2015-06-22T15:28:45Z

@Metalaka sure, my bad. Can you add a basic test for your feature ?

Metalaka · 2015-06-22T16:46:55Z

Done.

osaris · 2015-06-23T19:20:23Z

Thanks, everything looks good to me. Thoughts @hoaproject/hoackers ?

Jir4 · 2015-06-24T08:07:34Z

👍 Looks good to me too

jubianchi · 2015-06-24T18:16:42Z

👍

Hywan · 2015-06-25T14:12:14Z

Query/Delete.php

@@ -75,6 +75,7 @@ public function from($source)
     */
    public function __toString()
    {
-        return 'DELETE FROM ' . $this->_from . parent::__toString();
+        return 'DELETE FROM ' . $this->enclose($this->_from) .


CS:

return 'DELETE FROM ' . $this->_from . parent::__toString();

When we have multiple concatenation, we prefer this syntax now :-). Had to be formalized though…

Hywan · 2015-06-25T14:21:18Z

Please, add tests when disabling encoding :-).

Good job!

Hywan · 2015-06-26T09:46:32Z

Also, I was thinking, what about renaming “name” into “symbol”? I reckon it's more abstract and then it might be better because more reusable. Thoughts?

Metalaka · 2015-06-26T12:45:19Z

The problem with "enclose symbol" naming is:
we'll enclose symbols(ex-names) with symbols...

Metalaka · 2015-06-26T13:37:16Z

CS & naming fixed.
More tests added and moved to a specific file.

Hywan · 2015-06-29T09:07:02Z

Will review as soon as possible.

Hywan · 2015-08-04T06:36:55Z

Test/Unit/Query/EncloseNames.php

+            ->isEqualTo(
+                'SELECT a, b FROM foo'
+            );
+    }


The default case is missing?

Hywan · 2015-08-04T06:38:49Z

Look really good @Metalaka. I have a last question. Why not having EncloseNames as a trait instead of a class? It makes more sense, isn't it?

Metalaka · 2015-08-04T13:02:42Z

yes, and it's done :)

Hywan · 2015-08-05T06:53:11Z

Query/EncloseNames.php

+     */
+    protected function _enclose($name)
+    {
+        if (0 === preg_match('#\s|\(#', $name)) {


0 !== preg nop?

Nop, we enclose the symbol only if there is NO space (alias) or parentheses (function).

Hywan · 2015-08-05T06:56:33Z

@Metalaka Seems excellent. Maybe a rebase would be welcome (not necessarily one commit). Waiting the go of @osaris, he's the peer-reviewer on this one.

Metalaka · 2015-08-05T10:10:27Z

Also, I was thinking, what about renaming “name” into “symbol”? I reckon it's more abstract and then it might be better because more reusable. Thoughts?

The problem with "enclose symbol" naming is:
we'll enclose symbols(ex-names) with symbols...

We can also rename name into identifier ?

Hywan · 2015-08-05T10:29:29Z

@Metalaka: An identifier is most of the time unique. Name is not all the time correct. Symbol is good nop?

Hywan · 2015-08-05T10:30:16Z

@Metalaka git commit --amend: “Add tests for EncolseNames feature.”, Enclose, not Encolse 😉.

Metalaka · 2015-08-05T10:39:02Z

Thanks for the typo :)
A table, column, alias name is unique in his query.
IMO symbol is too abstract and is not used for the same things in sql1992.

Hywan · 2015-08-05T10:39:41Z

So let's go for identifier?

Metalaka · 2015-08-05T10:40:37Z

If you're ok ;)

Metalaka · 2015-08-05T10:57:26Z

Done, tests are green.

Hywan · 2015-08-05T12:37:29Z

@Metalaka Waiting the peer-review of @osaris 😃.

osaris · 2015-08-06T17:40:34Z

Looks very good. Thanks @Metalaka very good job !

Hywan · 2015-08-10T05:59:13Z

Query/EncloseIdentifier.php

+        $this->_closingSymbol =
+            (null === $closingSymbol)
+                ? $openingSymbol
+                : $closingSymbol;


$this->_closingSymbol = $closingSymbol ?: $openingSymbol;

may be better. Going to do it by myself.

Hywan · 2015-08-10T06:06:09Z

Awesome new feature, thank you very much!

Hywan added the enhancement label Jun 22, 2015

Hywan assigned osaris Jun 22, 2015

Metalaka force-pushed the encloseNames branch 2 times, most recently from 3ff7f56 to 46d2c64 Compare June 22, 2015 16:46

Hywan reviewed Jun 25, 2015
View reviewed changes

Metalaka force-pushed the encloseNames branch from 46d2c64 to 5ec2c6e Compare June 26, 2015 13:32

Hywan reviewed Aug 4, 2015
View reviewed changes

Test/Unit/Query/EncloseNames.php

->isEqualTo(

'SELECT a, b FROM foo'

);

}

Copy link

Member

Hywan Aug 4, 2015

Choose a reason for hiding this comment

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

The default case is missing?

Hywan added the difficulty: medium label Aug 4, 2015

Metalaka force-pushed the encloseNames branch 3 times, most recently from 12a4d26 to 282a727 Compare August 4, 2015 12:55

Hywan reviewed Aug 5, 2015
View reviewed changes

Metalaka force-pushed the encloseNames branch from 282a727 to 4e0aebb Compare August 5, 2015 10:29

Metalaka force-pushed the encloseNames branch from 4e0aebb to 9dced68 Compare August 5, 2015 10:30

Metalaka added 2 commits August 5, 2015 12:43

Introduce enclose identifier feature.

e08c09d

Add tests for enclose identifier feature.

9376a28

Metalaka force-pushed the encloseNames branch from 9dced68 to 9376a28 Compare August 5, 2015 10:54

Hywan reviewed Aug 10, 2015
View reviewed changes

Bhoat merged commit 9376a28 into hoaproject:master Aug 10, 2015

Hywan removed the in progress label Aug 10, 2015

Hywan mentioned this pull request Aug 10, 2015

Fix CS #30

Merged

Metalaka deleted the encloseNames branch August 10, 2015 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Enclose names feature. #27

Introduce Enclose names feature. #27

Metalaka commented Jun 19, 2015

Hywan commented Jun 22, 2015

osaris commented Jun 22, 2015

Metalaka commented Jun 22, 2015

osaris commented Jun 22, 2015

Metalaka commented Jun 22, 2015

osaris commented Jun 23, 2015

Jir4 commented Jun 24, 2015

jubianchi commented Jun 24, 2015

Hywan Jun 25, 2015

Hywan commented Jun 25, 2015

Hywan commented Jun 26, 2015

Metalaka commented Jun 26, 2015

Metalaka commented Jun 26, 2015

Hywan commented Jun 29, 2015

Hywan Aug 4, 2015

Hywan commented Aug 4, 2015

Metalaka commented Aug 4, 2015

Hywan Aug 5, 2015

Metalaka Aug 5, 2015

Hywan Aug 5, 2015

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

osaris commented Aug 6, 2015

Hywan Aug 10, 2015

Hywan commented Aug 10, 2015

Introduce Enclose names feature. #27

Introduce Enclose names feature. #27

Conversation

Metalaka commented Jun 19, 2015

Hywan commented Jun 22, 2015

osaris commented Jun 22, 2015

Metalaka commented Jun 22, 2015

osaris commented Jun 22, 2015

Metalaka commented Jun 22, 2015

osaris commented Jun 23, 2015

Jir4 commented Jun 24, 2015

jubianchi commented Jun 24, 2015

Hywan Jun 25, 2015

Choose a reason for hiding this comment

Hywan commented Jun 25, 2015

Hywan commented Jun 26, 2015

Metalaka commented Jun 26, 2015

Metalaka commented Jun 26, 2015

Hywan commented Jun 29, 2015

Hywan Aug 4, 2015

Choose a reason for hiding this comment

Hywan commented Aug 4, 2015

Metalaka commented Aug 4, 2015

Hywan Aug 5, 2015

Choose a reason for hiding this comment

Metalaka Aug 5, 2015

Choose a reason for hiding this comment

Hywan Aug 5, 2015

Choose a reason for hiding this comment

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Metalaka commented Aug 5, 2015

Hywan commented Aug 5, 2015

osaris commented Aug 6, 2015

Hywan Aug 10, 2015

Choose a reason for hiding this comment

Hywan commented Aug 10, 2015