Note that the function update_sql() does not take %s, %d, etc.

The update in the spam filter "Node Age" is not valid.

See patch.

Thank you.
Alexis

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AlexisWilke’s picture

This is actually found in all the sub-modules except surbl.

There is a patch for all of the sub-modules.

gnassar’s picture

Do we have a good reason for using update_sql in the first place instead of db_query?

I would think not -- and since update_sql has been removed in Drupal 7, we should probably be sticking with db_query anyway.

AlexisWilke’s picture

gnassar,

The update_sql() is the standard in D6. It returns an error that you add to the array of errors being return to the update.php script, which in turn presents said errors to the users.

There is no point to be D7 compliant on a D6 update function.

Although I appreciate your comments, and some are really good, you may also want to consider accepting such changes without being to hard on us (as in, the developers) so we can move forward faster to a 1.1 release. In other terms... unless you have plenty of time to do the fixes you are mentioning, why not just accept working fixes? (in place of not working code)

Thank you.
Alexis

gnassar’s picture

Priority: Major » Normal

The update_sql() is the standard in D6. It returns an error that you add to the array of errors being return to the update.php script, which in turn presents said errors to the users.

Please specify where it is the "standard" in D6. It is typical to use in update functions -- unless there is variable substitution involved, in which case one uses db_query for string safety and recreates the single line that update_sql adds inline. ex.: http://www.mc-kenna.com/drupal/2009/05/fixing-updatesql-to-accept-parame... which is much easier than rolling your own string safety and escaping ex.: http://drupalhigh.onsugar.com/updatesql-my-friend-2672756 for problems with using hardcoded substitutions in update_sql.

The proper argument here is probably that we have a static set of strings, so we know the input is clean and don't need the string safety. In that case, update_sql is fine.

There is no point to be D7 compliant on a D6 update function.

The point is to minimize how much needs to be rewritten in the inevitably upcoming D7 port.

Hopefully we'll have a 1.1 before we do D7, so this won't be an issue (no updates needed).

Although I appreciate your comments, and some are really good, you may also want to consider accepting such changes without being to hard on us (as in, the developers) so we can move forward faster to a 1.1 release. In other terms... unless you have plenty of time to do the fixes you are mentioning, why not just accept working fixes? (in place of not working code)

Because discussing takes a (usually very) few minutes of our time, but allows us to submit patches that clean up and improve the code base, instead of tacking a band-aid onto the four previous band-aids.

Besides, if we're concerned about a 1.1 release, this is not the sort of bug to be concerned about. There are plenty of existing ones in the queue.

"Working" fixes aren't sufficient, if they make other things more difficult. Short-term gain isn't worth it when very little more time gives long-term improvement in addition.

And on that note -- please re-roll that last patch, for all sub-modules. It includes code changes which aren't germane to this ticket. (Or we can apply the first patch, which is clean.)

AlexisWilke’s picture

The first patch does not include all the necessary changes as it only takes care of one module...

update_sql() does not take any parameter in account, so the existing doesn't work at all. Personally I don't care since I never ran 1.0, the few users who are on 1.0 may care a tad bit more.

Now, getting an update function perfect is really moot since it is anyway throw away code, but getting it to do what was intended is still a good idea.

Since I'm not too sure what you're intend really is, I'll let you go ahead and fix the code. You could even test that the update actually works.

Thank you.
Alexis

gnassar’s picture

Yes, that's right. The first patch only takes care of one. Taking care of all of them is a good idea -- but the second patch includes code that does not do that. It looks like you got a bunch of extra lines from the Coder review that were accidentally included in that second patch. That was why I was requesting a re-roll.

Hope that clarifies things. If you can re-roll, it'd be appreciated. Otherwise, I'll do this after I finish the other few tickets I'm working on.

AlexisWilke’s picture

Okay... I don't see that as being bad, doing two things at once when one is just syntactical... anyway...

What about the hook_uninstall() calls? Do you like the DELETE of the variables?

Thanks,
Alexis

gnassar’s picture

Status: Active » Needs review

Actually, the hook_uninstall() calls were what I was referring to with regard to adding things outside the scope of the ticket.

That's also much less of a problem if the additions are at least mentioned when the patch is submitted: "I also cleaned up some formatting and added DELETES for the created variables in the hook_uninstall() calls." Just so nobody's considering adding a patch that has unknown effects.

Just looked over them -- I think I'm fine with the additions. I'll bump the status forward from "active" to "needs review," and I'll see if I have a bit of time later today to apply the patch and see how it works. (Honestly, since we really haven't had most of these variables, I'm a bit concerned about weird unintended side effects from the code actually being able to *use* these.) :)

AlexisWilke’s picture

Ah! The DELETEs were not part of the Coder review. The Coder changes are just the 'null' => 'NULL' and a few spaces added/removed here and there.

There was one sub-module with an uninstall and the intend of deleting the variables, but the code was not complete and the use of an array to list all the variables is not very strong (as you add new variables you need to remember to add it to that one array and 99% of the time you forget...), so the DELETE with the namespace is a better solution, to my point of view (there should be a Drupal Core var_module_del('module name'); function!)

Thank you.
Alexis

gnassar’s picture

Status: Needs review » Needs work

Yup, it seems like perhaps we should, after all, break this into smaller sub-patches like I was saying.

I can't seem to find drupal_strcmp() listed anywhere in the API. I'm guessing that's a Coder module change?

See? There *are* good reasons for going through the process rigorously instead of trying to take shortcuts. :) They often end up causing more work in the end.

AlexisWilke’s picture

So you want two patches, one for the variable deletion and one for the 6101 upgrade?

Thank you.
Alexis

gnassar’s picture

The drupal_strcmp() problem is neither with variable deletion nor with the 6101 upgrade. It's a totally unrelated issue that I'm guessing is related to the Coder module cleanup.

So -- one *clean* (nothing else) patch for variable deletion, because that's a bug. Another clean patch for the 6101 upgrade, because that's a different bug. And if you really want to do all that code cleanup (*and* are willing to take the time to fix any resultant issues, like the one I found), that's fair too -- but that goes in a separate patch. Bug fixes should not include cleanup of code outside the scope of the affected bug.

gnassar’s picture

In fact, since the variable deletion and the 6101 upgrade have both been discussed in this issue, you could probably do one (clean) patch for both -- though that should be the exception, and not the rule, and we should look to conform to the patch and commit guidelines in the future. But the extra, unrelated stuff definitely needs to be separate. Changes to unrelated functions falls outside of the scope of the ticket.

AlexisWilke’s picture

Well... After careful review, I guess my patch was not fixing anything!

There is a newer version and a few words about it:

1) $test = variable_get($var, null); + if (!empty($test) ...

This was wrong since $test could be set to 0 and !empty($test) would "think" that it is not set.

2) !strcmp(substr($var,0,12),"spam_filter_")

What does that do?! See the 2 problems?? (not talking about the missing spaces)

Yeah... substr($var, 0, 12) is NEVER going to be equal to "spam_filter_". It is 12 characters long, but none of the $old_vars start with "spam_filter_". Also, notice the ! in front of the strcmp(). This means it is true when the variable is equal. In other words NEVER. This means what's inside the if() block is never executed.

3) UPDATE {variables} ...

Yeah... this one is harder to notice. The variable table is singular. So even if I had fixed point (2) [which in my first sloppy review I had not done] it would not have done much since {variables} would not be found... Ain't that funny?!

4) So, now I do that in a different way. What I do not like is that the code is 100% the same in all the filters .install file. The only difference is the $old_var. I think that ought to be in a separate file that's included by the .install files and called as required. This would be easier to fix. I know that updates are kind of like throw away code, but whenever there is a bug in one of those you have to remember to fix all 5!

5) Just in case, I reviewed the code from 1.0 to make sure all the variables were included in the $old_vars. I did not see any discrepancy, except maybe for the spam_blacklist_ip which may be a duplicate filter specific variable and not a spam wide variable? (I have another issue open in regard to that variable since it is not documented nor presented in any UI.)

Thank you.
Alexis Wilke

AlexisWilke’s picture

Status: Needs work » Needs review

Meant to mark this one as needs review.

gnassar’s picture

That is a *much* better code review than the first time around. :)

One more problem, though: the Bayesian variable delete probably won't work, because "_bayesian" is misspelled. (Bet I would've caught that the first time around if this had been in two separate, smaller patches for each issue!) ;)

AlexisWilke’s picture

Here is a fixed DELETE.

Let me know if the changes work for you. The best is obviously to install 1.0 and then 1.x with this patch and see that the update.php does what it is expected to do.

Thank you.
Alexis