Hi,

CivicActions is reviewing and upgrading multiple modules for use on client sites. Attached are a few patches for the problems identified. There's 4 patches attached:

  • views_ansi_sql.patch: contains one change for making a sql query ansi sql compliant, so it works on all databases.
  • views_placeholders.patch: Drupal 6 saw the introduction of the db_placeholders() function. While the existing method implemented works perfectly, I've modified the code to use db_placeholders() instead so as to avoid code duplication.
  • views_placeholders_minor.patch: Placeholders aren't being used for $view->vid, which is fine since it's a safe integer, but this patch changes it to use placeholders - fairly minor change, more of a style issue.
  • views_misc_codereview.patch: contains a couple of other miscellaneous items.

Cheers,
Stella

CommentFileSizeAuthor
views_placeholders_minor.patch3.33 KBstella
views_placeholders.patch21.02 KBstella
views_misc_codereview.patch1.51 KBstella
views_ansi_sql.patch1023 bytesstella

Comments

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Closed (won't fix)

#1: Views only supports two databases, MySQL and pgsql. It won't work right on other databases, regardless of this fairly frivolous patch.

views_misc_codereview.patch will actually break code. menu_get_object('node') will not work in all circumstances.

Please search the issue queue for my objections to core's db_placeholders. Or don't, I really don't care, but I will not use the function.

merlinofchaos’s picture

merlinofchaos commented

You'll forgive me for venting, but this is roughly the 3rd or 4th time someone has tried to be 'helpful' and try to patch in use of the db_placeholders function. I don't quite get what people's obsession with this function is, but it seems like a giant waste of time for people to be focusing on this. There are real bugs in the system, as well as missing features and inefficiencies.

I appreciate contributions, but contributions that are nothing more than rearranging the furniture but do not provide actual value are just a waste of your time as well as mine.

I am going to relent slightly; the 4th one is actually useful, the convert.inc script probably could use those changes, but please bear in mind that $view->vid was loaded from the database from a field that is numeric, so there is no danger there in just using $view->vid in the queries. That said, it's inconsistent, so is probably worth of being utilized.

I also find it frustrating that code review actually creates bugs rather than fixing them.

stella’s picture

stella commented

To respond:

ansi sql - I didn't realise that Views only supported MySQL and pgsql. I haven't seen it documented anywhere, though I will admit there's a lot of (very good) documentation supplied for Views and I haven't read every single page, so maybe it's mentioned somewhere.

db_placeholders - I did say your implemented solution worked perfectly. It comes down to a matter of style I guess. I see you object to db_placeholders() because it doesn't include a space after each comma, which I agree would be better and more readable. Don't suppose you could point me to any issue requesting this change? I had a quick search and couldn't find any.

$view->vid placeholders - I did say $view->vid was a safe integer and that it was more of a style change rather than there being any real need to do it.

views_misc_codereview.patch - whatever about the 2nd change, doesn't the 1st one still need to be implemented? It's just a typo. I realise views_ui_edit_view_form_validate() doesn't modify $form_state in any way, so it's prob a non-issue, however it is possible that the function may be changed in the future and the missing & may be missed.

Regarding code review creating bugs - it's always a possibility, especially when the reviewer isn't as familiar with the code as he/she would like to be. However, I didn't *think* any of my suggested changes (and they were just that, suggestions) introduced bugs. As for menu_get_object('node') - I thought this only caused problems when called inside node_load(), etc. I'd be interested in learning more about the other scenarios in which it fails.

Cheers,
Stella

merlinofchaos’s picture

merlinofchaos commented

Sorry for the vent there. As for db_placeholders, it's one of those areas where I have spent an inordinate amount of time trying to prettify the queries as much as possible, and db_placeholders undoes that work. While I'm not sure of issue numbers offhand. I know that Moshe posted an issue about using db_placeholders (no patch, just a comment that this should be done) and one other person also posted a patch. Both of them are marked won't fix.

Yes, the & missing from $form_state is a potential bug and should be fixed.

menu_get_object('node') only checks position 1, plus, if using views or panels, there is a very good chance that the placeholder won't be node. For example, views uses %views_arg because %node is not suitable for the data it needs. So if I have some URL, such as gallery/% which has a gallery node and leads to a view which leads to images, menu_get_object() won't pick that up. The code in the argument default finders actually uses both; first it checks menu_get_object (and it cycles 1 through 3 to check several positions) and then it checks args directly to ensure that something overriding the URL isn't eliminating the 'node' object from the menu. (IMO the design of the % keywords turned out to be a little flawed).

In fact, if you look a little above the code that is patched in the #2 patch, you'll see a few menu_get_object() calls where we already tried that. That particular code is just a fallback. Granted, this should probably be commented to state that this is what's going on. Handlers are often undercommented because there are so many to do that I was trying to power through them to get them all done. =(

I've committed the &$form_state fix.

stella’s picture

stella commented

Regarding db_placeholders issue, I did find Moshe's and the other issues in the views queue easily enough. What I was wondering though, is there a Drupal feature request requesting a change to db_placeholders() so it would add a space? It's a small enough change, so maybe there isn't one.

Thanks for taking the time to explain the issues with menu_get_object above, it's really helpful.

merlinofchaos’s picture

merlinofchaos commented

No, I don't usually bother with patches against core for simple aesthetic issues like that. And such a patch would not likely get into Drupal 6 anyhow, or at least not without an amount of political lobbying in which I have zero interest in participating.