Posted by gatoth on November 4, 2009 at 8:26am
| Project: | Feeds |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Novice |
Issue Summary
In PostgreSQL queries, strings has to be enclosed in apostrophes ('), quotation marks (") do not work. Please fix it.
Thank you,
Gabor
Comments
#1
We should do that soon. This is a novice task.
#2
Patches below replace use of double quote with (escaped) single quote.
#3
Great.
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 ? : )
#4
Bumping to critical as this should go into the next alpha release. Any answers for 3.1 ?
#5
@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.
#6
I 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.
#7
#6: agree, I'll keep the reference to PostgreSQL in the title to avoid duplicates.
#8
Thanks :)
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.
#9
#8 great. As I stated in #3 - I'd really like to avoid escape sequences.
So
"SELECT * FROM {node} WHERE type = 'story'"instead of
'SELECT * FROM {node} WHERE type = \'story\''The first version reads much better, unfortunately all patches in #2 use escape sequences.
#10
#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!
#11
Did 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:
cd /tmpwget http://ftp.drupal.org/files/projects/feeds-6.x-1.0-alpha10.tar.gz
tar xvzf feeds-6.x-1.0-alpha10.tar.gz
cp -a feeds feeds.orig
sed -rf patching_db_query.sed tests/feeds.test -i feeds/`$(find feeds/ -type f)` > feeds-standard_sql.patch
diff -urN feeds.orig feeds
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
#12
*Nice* - are all tests passing?
For instructions on running tests for Feeds see http://drupal.org/node/622700 at the bottom.
#13
Running 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.
#14
#13: I need to take a look at the tests.
#15
I 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.:
+ $result = db_query("select feed_nid, last_scheduled_time, scheduled from {feeds_schedule} WHERE last_scheduled_time <> 0");Could become:
+ $result = db_query("SELECT feed_nid, last_scheduled_time, scheduled FROM {feeds_schedule} WHERE last_scheduled_time <> 0");#16
#15 Yes, I'm fine with that.
#17
#15 Agree too. This is a good pratice.
@alex_b Did you run the tests on your side?
#18
#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.
#19
@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.
#20
I'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?
#21
Ok 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.
#22
#20 I saw reading my patches afterwards that the sed script seems to have missed some queries.
#23
Did 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:
cp -a feeds feeds.origsed -rf patching_db_query.sed -i `find feeds/ -type f`
sed -rf patching_db_capitalize.sed -i `find feeds/ -type f`
diff -urN feeds.orig feeds > my.patch
#24
I'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?
#25
Committed.
http://drupal.org/cvs?commit=348308 and http://drupal.org/cvs?commit=348310
#26
w00t!
#27
In 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.
#28
Fixed, thank you.
#29
Automatically closed -- issue fixed for 2 weeks with no activity.
#30
There are three more of these in alpha14:
includes/FeedsScheduler.inc:
<?php$result = db_query_range('SELECT feed_nid, id, callback, last_executed_time FROM {feeds_schedule} WHERE id = "%s" AND callback = "%s" AND scheduled = 0 AND (last_executed_time < %d OR last_executed_time = 0) ORDER BY last_executed_time ASC', $importer->id, $callback, FEEDS_REQUEST_TIME - $period, 0, $num);
?>
plugins/FeedsNodeProcessor.inc:
<?php$result = db_query_range('SELECT n.nid FROM {node} n JOIN {feeds_node_item} fni ON n.nid = fni.nid WHERE fni.id = "%s" AND n.created < %d', $this->id, FEEDS_REQUEST_TIME - $time, 0, variable_get('feeds_node_batch_size', FEEDS_NODE_BATCH_SIZE));
?>
plugins/FeedsNodeProcessor.inc:
<?phpif (db_result(db_query_range('SELECT n.nid FROM {node} n JOIN {feeds_node_item} fni ON n.nid = fni.nid WHERE fni.id = "%s" AND n.created < %d', $this->id, FEEDS_REQUEST_TIME - $time, 0, 1))) {
?>
#31
Nice catch. Thank you.
#32
Committed. Thank you.
#33
Automatically closed -- issue fixed for 2 weeks with no activity.