Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix: Identifier Quoting #325

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
4 participants

If no one seen it, identifier quoting dosen't work at all.
This change fixes that, now it works.
I test it on PostgreSQL 8.3, on big project. Now it seem to working correctly.

Contributor

c33s commented Mar 24, 2012

great! just wanted to report it.

the missing quoting breaks the fos user bundle for postgres, they use reserved sql words as tablenames see FriendsOfSymfony/FOSUserBundle#574

also keep in mind:

Quoting an identifier also makes it case-sensitive, whereas unquoted names are always folded to lower case. For example, > the identifiers FOO, foo, and "foo" are considered the same by PostgreSQL, but "Foo" and "FOO" are different from these
three and each other. (The folding of unquoted names to lower case in PostgreSQL is incompatible with the SQL standard, > which says that unquoted names should be folded to upper case. Thus, foo should be equivalent to "FOO" not "foo"
according to the standard. If you want to write portable applications you are advised to always quote a particular name or
never quote it.)

4.1.1. Identifiers and Key Words http://www.postgresql.org/docs/8.2/static/sql-syntax-lexical.html

@willdurand willdurand and 1 other commented on an outdated diff Mar 26, 2012

runtime/lib/om/BaseObject.php
@@ -12,14 +12,14 @@
* This class contains attributes and methods that are used by all
* business objects within the system.
*
- * @method BaseObject fromXML(string $data) Populate the object from an XML string
- * @method BaseObject fromYAML(string $data) Populate the object from a YAML string
- * @method BaseObject fromJSON(string $data) Populate the object from a JSON string
- * @method BaseObject fromCSV(string $data) Populate the object from a CSV string
- * @method string toXML(boolean $includeLazyLoadColumns) Export the object to an XML string
- * @method string toYAML(boolean $includeLazyLoadColumns) Export the object to a YAML string
- * @method string toJSON(boolean $includeLazyLoadColumns) Export the object to a JSON string
- * @method string toCSV(boolean $includeLazyLoadColumns) Export the object to a CSV string
+ * @method BaseObject fromXML() fromXML(string $data) Populate the object from an XML string
@willdurand

willdurand Mar 26, 2012

Owner

why did you change these lines?

@willdurand

willdurand Mar 26, 2012

Owner

Ok but please, just PR useful code for your fix here. It's better to read, review, and comment it.

To fix phpdoc (which is always a good idea) could be done in another PR (by you if you want).

@sedziwoj

sedziwoj Mar 27, 2012

Ok, I remove this changes.

@willdurand willdurand and 1 other commented on an outdated diff Mar 26, 2012

generator/lib/builder/om/PHP5NestedSetPeerBuilder.php
@@ -184,15 +184,15 @@ protected function addConstants(&$script)
foreach ($table->getColumns() as $col) {
if ($col->isNestedSetLeftKey()) {
- $colname['left'] = $tableName . '.' . strtoupper($col->getName());
+ $colname['left'] = $tableName . '.' . ($col->getName());
@willdurand

willdurand Mar 26, 2012

Owner

You should remove all these brackets without sense in your whole fix

@sedziwoj

sedziwoj Mar 26, 2012

I will remove them.

Owner

willdurand commented Mar 26, 2012

Wow... Did you run the test suite?

I now try run test on version in main repo, but it fail...
Eg GeneratedObjectRelTest::testSetterCollectionSavesForeignObjects()
try add book without isbn, this field is not null...

I will try to make it work, and then test it with my changes.

Owner

willdurand commented Mar 27, 2012

The commit ad6aad0 has to be removed from your PR. You have to rebase your PR instead of adding merge commits.

GIT is new to me, thanks for advices.

Owner

willdurand commented Apr 23, 2012

@sedziwoj can you remove your last commit, and rebase your pull request? Then, I'll merge it :)

Owner

willdurand commented Apr 23, 2012

I know it, i try run test on project without my changes it was same.
Now i change job, and I dont have time, but in 2-3weeks i look at it again an correct (remove last commit, rebase, make test pass)

Owner

willdurand commented Apr 23, 2012

No worry. It's probably the best PR since a long time. Quoting identifiers should be enabled..

Owner

willdurand commented Jun 22, 2012

@sedziwoj can you fix the test suite?

I just returned to the change.
And this requires more changes, I discovered that many of SQL is not escaped.
(now try to run the tests without changes, and then with them.)

I ran the tests on the night, on https://github.com/propelorm/Propel.git and get only 96%pass (Fatal error).
Tomorrow I go in and check what is wrong (maybe some missing lib)

Owner

willdurand commented Jun 25, 2012

Good news, thanks!

I think i cant do it on windows... is something not right with test, I cant run on postgresql (I found 1 test not use preper, only query with ", this sign is special for pg), I cant run on windows + mysql, first sh script, and maybe more (do by my self same stuff and dont work).
Tomorrow I will try run on linux server, but it take time.

Owner

willdurand commented Jun 28, 2012

no problem, this is a huge work ;)

What's the latest on this, this is huge! Can we get this merged in, sfPropelORMPlugin updated? PLEASE :)

Owner

willdurand commented Oct 4, 2012

It's still a work in progress...

Would be super awesome if we could get it merged in :) What's needed?

Owner

willdurand commented Oct 4, 2012

fixing tons of unit tests ;)

On Fri, Oct 5, 2012 at 12:32 AM, Jacob Thomason notifications@github.comwrote:

Would be super awesome if we could get it merged in :) What's needed?


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/pull/325#issuecomment-9159675.

Well, sqlite support for reserved keywords sure would be nice :/

Owner

willdurand commented Oct 4, 2012

Sure, it would be a really great improvement but it's a lot of work too.
This PR is quite good but some unit tests need to be fixed.

On Fri, Oct 5, 2012 at 12:35 AM, Jacob Thomason notifications@github.comwrote:

Well, sqlite support for reserved keywords sure would be nice :/


Reply to this email directly or view it on GitHubhttps://github.com/propelorm/Propel/pull/325#issuecomment-9159751.

Well, maybe if I get some time and can figure out how to get a copy of the repo and cherry pick this commit as well, I'll have a look. I'm not sure I know enough about the core codebase, but I can probably figure out most of it. Although, someone with more experience could prob knock it out a lot faster :)

Owner

willdurand commented Nov 4, 2012

I've rewritten this pull request, and merged it into the master branch.

PLEASE test it now.

@willdurand willdurand closed this Nov 4, 2012

c33s pushed a commit to c33s/Propel that referenced this pull request Feb 5, 2014

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