Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alex_b’s picture

Version: 6.x-1.0-alpha6 » 6.x-1.x-dev
Issue tags: +Novice

We should do that soon. This is a novice task.

Otaci’s picture

alex_b’s picture

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 ? : )

alex_b’s picture

Priority: Normal » Critical

Bumping to critical as this should go into the next alpha release. Any answers for 3.1 ?

pounard’s picture

@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.

Note: A lot of people says that using "" to delimit strings is bad because '' do not make PHP parse the string to eventually include variables so the result is faster, I would say that in real world, you will never see the difference.

pounard’s picture

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.

alex_b’s picture

Title: PostgreSQL compatibility » Fix non-standard SQL (PostgreSQL compatibility)

#6: agree, I'll keep the reference to PostgreSQL in the title to avoid duplicates.

pounard’s picture

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.

alex_b’s picture

#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.

pounard’s picture

#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!

pounard’s picture

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 /tmp
wget 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

alex_b’s picture

*Nice* - are all tests passing?

For instructions on running tests for Feeds see http://drupal.org/node/622700 at the bottom.

pounard’s picture

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.

alex_b’s picture

#13: I need to take a look at the tests.

drewish’s picture

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");
alex_b’s picture

#15 Yes, I'm fine with that.

pounard’s picture

#15 Agree too. This is a good pratice.
@alex_b Did you run the tests on your side?

alex_b’s picture

Status: Needs review » Needs work

#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.

pounard’s picture

@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.

alex_b’s picture

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?

pounard’s picture

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.

pounard’s picture

#20 I saw reading my patches afterwards that the sed script seems to have missed some queries.

pounard’s picture

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.orig
sed -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
krisis’s picture

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?

Will White’s picture

alex_b’s picture

w00t!

pongtawat’s picture

Status: Fixed » Needs work

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.

alex_b’s picture

Status: Needs work » Fixed
FileSize
1.72 KB

Fixed, thank you.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

mikl’s picture

Status: Closed (fixed) » Needs work

There are three more of these in alpha14:

includes/FeedsScheduler.inc:

$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:

$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:

if (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))) {
alex_b’s picture

Status: Needs work » Needs review
FileSize
3.01 KB

Nice catch. Thank you.

alex_b’s picture

Status: Needs review » Fixed

Committed. Thank you.

Status: Fixed » Closed (fixed)
Issue tags: -Novice

Automatically closed -- issue fixed for 2 weeks with no activity.