Query coder

You're still working on the master branch.

Please see the project application checklist: http://drupal.org/node/1587704

In particular please see 2.2 and 4.2 in the checklist.

It looks like you have non-GPL code in the php-sql-parser directory.

Good luck with your project and thanks for submitting.

@denisz -- I'm no expert on the licensing, but I think you have the right idea. If it is non-GPL, you have to remove it from your repo and include instructions for downloading the library in your README or INSTALL files. That's what I would do. I will change this back to "needs work" until you indicate that you have removed the non-GPL code.

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxdenisz1939706git

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and put yourself on the PAReview: review bonus high priority list. Then I'll take a look at your project right away :-)

@denisz
I tried this query...

SELECT users.* FROM users

Your form output...

$query = db_select('users', '');
$query->fields('users', array('*'));
$result = $query->execute();

...I pasted that code into a module file and ran it. I got this PDO error:

PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS FROM users users' at line 1: SELECT users. AS FROM {users} users; Array ( )

The same MySQL query runs fine if I paste it into the sql form in phpMyAdmin.

What is your use case for this module? Getting something like this to work consistently seems like a very difficult task, so I wonder what your motivation is? How will you use this? Is this module maintainable?

Thank you.

For me is regular task

I suppose that's good enough for me. It's not part of my workflow, but that doesn't mean it isn't part of yours.

Please provide some mention in the README and project page re: which databases the underlying library works with. You could provide one sentence about it, so people immediately understand the module uses a parser that aims to work with numerous databases, but only fully supports MySQL. Just to save someone a minute.

Here is a good example of a README.txt: http://drupal.org/node/1271064
Yours doesn't have to be as long, but you could at least add a bit more information, such as a link to the project page.

The module fails for some queries. I fetched your latest code and re-installed the module. I tried the following query...

INSERT INTO shop VALUES (1,'A',3.45),(1,'B',3.99),(2,'A',10.99),(3,'B',1.45), (3,'C',1.69),(3,'D',1.25),(4,'D',19.95);

The result shown was "INSERT queries not supported now". I think you intend to support insert queries. Anything regarding queries you don't intend to support should probably not be displayed below a heading that says "Results". Another way to display such a message would be to use drupal_set_message(). That might be more appropriate in a case like this. And of course, queries you don't support must be mentioned in the README and project page.

You need to write more tests. For your tests, be sure you are using a wide variety of queries you didn't think of yourself, from places like http://dev.mysql.com/doc/refman/5.0/en/examples.html
It's hard to say how many tests you need for a project like this, but you likely need many more than you've provided.

The module code appears to be quite clean, and it's certainly a difficult task you've set for yourself. But this module has enough remaining issues at this point that removing the bonus tag seems appropriate.

The result shown was "INSERT queries not supported now".

I was wrong about this part. I didn't have the latest version of your module. I see now that the form does output some code for the query, but the code is not correct.

You could also work this query into your tests:
SELECT field1_index, field2_index FROM test_table WHERE field1_index = '1' OR field2_index = '1'
At the moment the form isn't handling that query correctly.

Looks good to me.

You might consider making the query form smaller. I doubt the textarea needs to be 15 rows. Maybe three or four rows would work?

Given the nature of the module, consider writing more tests. But it's nice that you've already included some.

In step 2 of the installation instructions, you have a typo: "Dwonload..."

One more minor thing. You should put periods at the end of comments.

Right:

/**
 * @file
 * Pages for query_coder module.
 */

Wrong:

/**
 * @file
 * Pages for query_coder module
 */

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects.

removed non-manual review.

manual review:

1. query_coder.module: why do you need to globally include the .functions.inc file? This will be included on every single page request, are you sure that you need to expose those functions as API? I t seems to me that they should only be included from the form submission handler?
2. query_coder_coder_form(): when building forms use #attached instead of drupal_add_library/js/css.
3. "variable_set('query_coder_counters' ...": looks like you are abusing the variable system here, which should be used for persistent variables. I assume that the contents of the variable is only useful within one request, so you should use a global variable instead. But remember: global variables are a code smell, if you need to access state information then you should pass that state around explicitly, i.e. as function parameter.
4. query_coder.functions.inc: almost all function doc blocks are useless, as they just repeat what the function name already says. They should provide additional useful information like "Preprocess the query to count SQL BETWEEN statements." or whatever.
5. I think your module should have a note somewhere that db_query() is preferred over db_select() for simple static queries, because it adds just a bunch of method calls to the runtime. See http://drupal.org/node/310075

But that are not blockers, so ...

Thanks for your contribution, denisz!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.