Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
In PostgreSQL queries, strings has to be enclosed in apostrophes ('), quotation marks (") do not work. Please fix it.
Thank you,
Gabor
Comment | File | Size | Author |
---|---|---|---|
#31 | 622932-31_standard_sql.patch | 3.01 KB | alex_b |
#28 | 622932-28_sql.patch | 1.72 KB | alex_b |
#23 | patching_db_capitalize.sed_.txt | 315 bytes | pounard |
#23 | feeds-sql_capitalize.patch | 4.42 KB | pounard |
#23 | feeds-sql_capitalize_standard.patch | 38.12 KB | pounard |
Comments
Comment #1
alex_b CreditAttribution: alex_b commentedWe should do that soon. This is a novice task.
Comment #2
Otaci CreditAttribution: Otaci commentedPatches below replace use of double quote with (escaped) single quote.
Comment #3
alex_b CreditAttribution: alex_b commentedGreat.
2 questions:
1) Wouldn't it be better practice to use double quotes for the query string and single ' quotes for strings in it?
2) One patch instead of 10 ? : )
Comment #4
alex_b CreditAttribution: alex_b commentedBumping to critical as this should go into the next alpha release. Any answers for 3.1 ?
Comment #5
pounard@alex_b
' delimiter is the only SQL standard string delimiter, " is a MySQL specialty which help all PHP young developpers to anti-learn what is really SQL.
Comment #6
pounardI would rather rename this issue "Non standard SQL spotted" rather than let young developers believe that's a PostgreSQL specific error, in this case it's the opposite, it's a MySQL specific wrong syntax.
Comment #7
alex_b CreditAttribution: alex_b commented#6: agree, I'll keep the reference to PostgreSQL in the title to avoid duplicates.
Comment #8
pounardThanks :)
I see that the patches I done in the duplicate bug entry I made have already been done by someone else here. If I see some which not have been fixed here I'll post it.
Comment #9
alex_b CreditAttribution: alex_b commented#8 great. As I stated in #3 - I'd really like to avoid escape sequences.
So
instead of
The first version reads much better, unfortunately all patches in #2 use escape sequences.
Comment #10
pounard#9 Totally agree. Patches can be edited manually :)
May be sed be our friend, a good regex would do the trick.
I'll try
Edit: I mean a regex on the original code!
Comment #11
pounardDid it.
Attached the patch (reviewed with my eyes but not tested yet), it patches all module files recursively. Because it took me some time to figure out how to use correctly sed, I also attach my sed script here, it coud be usefull to a lot of people.
PS: use the script with:
And you have your patch!
EDIT: Because I'm lucky, you don't wrote any multiline query strings, and it seems that there is none such things as having ' and " in other stuff than query itself on those lines! Else the sed script wouldn't have worked out and would have done bad things.
RE-EDIT: Wrote the full procedure I used, can be usefull for people
Comment #12
alex_b CreditAttribution: alex_b commented*Nice* - are all tests passing?
For instructions on running tests for Feeds see http://drupal.org/node/622700 at the bottom.
Comment #13
pounardRunning the tests, I have *a lot* of errors.
Most of them are PHP 5.3 notices, so they wouldn't hurt.
I have also some unique key constraints violations, which for the most make the following tests fail.
And event worst, I had some fatal errors.
I don't really think so that all are due to my patches. Fatal errors are due to simpletests itself (may be my environment is the cause, see #692358: In Drupal 6, ST 2.10, error on files include when running a batch with more than one file.
Could you try and run by your side? I'm not sure my environment is fully functional (clean Drupal 6.15 installation, latest SimpleTests stable, all installed modules are downloaded using Drush, using only stable versions, PostgreSQL 8.4, PHP 5.3, Apache2 on a Fedora 12 Linux distribution).
Edit: after a second look at my patch, it never could trigger those duplicate entries error. I think the latest feeds version does not pass these tests at all. This is only an assumption, I need to try with a non patched version on MySQL.
Comment #14
alex_b CreditAttribution: alex_b commented#13: I need to take a look at the tests.
Comment #15
drewish CreditAttribution: drewish commentedI visually looked over this and it looks fine but I wonder if we could go ahead and standardize the capitalization of the SQL while we're touching it, i.e.:
Could become:
Comment #16
alex_b CreditAttribution: alex_b commented#15 Yes, I'm fine with that.
Comment #17
pounard#15 Agree too. This is a good pratice.
@alex_b Did you run the tests on your side?
Comment #18
alex_b CreditAttribution: alex_b commented#17, all tests passing with or without this patch - see instructions at: http://drupal.org/node/622700#testing
My setup:
- PHP 5.2.6
- Drupal 6.15
- Latest release versions of all dependent modules (downloaded them all with drush dl)
I had to fix a small problem with the date mapper, that's all.
I'd still like to see the captalization standardized as drewish suggested in #15, setting to NW therefore. pounard: let me know if you're up for this task, otherwise, I'll put it on my backlog.
Comment #19
pounard@alex_b
#18
For capitalization that another small sed script should be enough.
Could you test with the patch, on MySQL?
PS: Which version of SimpleTests are you using? I'm currently using 2.x-dev or 2.10 depending on the site I'm working on. I'm also using PHP 5.3 which could be the source of my problems.
Comment #20
alex_b CreditAttribution: alex_b commentedI'm using Simpletest 2.10. PHP 5.3 is very likely a source of your problems. I tested with the patch, on MySQL and it worked fine.
If you could whip up a sed script for capitalization, that would be *awesome*.
Given the fact that there are 2 large patches in the queue that touch on queries (#617054: PubSubHubbub support and #600584: Use Batch API) what do you think of running this script again once they landed?
Comment #21
pounardOk my errors are mostly exceptions (which are PHP 5.3 warnings about undefined variables or indexes). Always initialiaze your variables :)
I'll do my best to fix some of your tests for PHP 5.3 before continuing submitting patches.
Comment #22
pounard#20 I saw reading my patches afterwards that the sed script seems to have missed some queries.
Comment #23
pounardDid a sed script for capitalization. I think it might be a bit too selective.
Attached 3 files:
- the sed script (btw my code upper is wrong, I was really tired this night)
- the patch (single)
- both patches in the same file
Always have my SimpleTest problems, but there wasn't more errors, so I think the patch is good:)
For curious people, the right shell code:
Comment #24
krisis CreditAttribution: krisis commentedI'm still having this issue in the 1.0-alpha12 release (using drupal 6.9) and not a big patching fan. On which release will this be fixed?
Comment #25
Will White CreditAttribution: Will White commentedCommitted.
http://drupal.org/cvs?commit=348308 and http://drupal.org/cvs?commit=348310
Comment #26
alex_b CreditAttribution: alex_b commentedw00t!
Comment #27
pongtawat CreditAttribution: pongtawat commentedIn alpha13, there is still a PostgreSQL compatibility problem in SQL commands in function clear in FeedsNodeProcessor.inc . Two SQL commands there still use " as string delimiter. This is trivial to fix.
Comment #28
alex_b CreditAttribution: alex_b commentedFixed, thank you.
Comment #30
mikl CreditAttribution: mikl commentedThere are three more of these in alpha14:
includes/FeedsScheduler.inc:
plugins/FeedsNodeProcessor.inc:
plugins/FeedsNodeProcessor.inc:
Comment #31
alex_b CreditAttribution: alex_b commentedNice catch. Thank you.
Comment #32
alex_b CreditAttribution: alex_b commentedCommitted. Thank you.