Nodequeue Context

welcome,

I'm not sure whether this is enough code to make a serious review, but regarding the fact that you're handling with ctools it's okay for me. Is it possible that you add a little more functionality ?

Please create a README.txt that follows the guidelines for in-project documentation.
Also take a moment to make your project page follow tips for a great project page.

You are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.

while waiting for an in-depht review of your module you can start out fixing some coding style issues detected by automated tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxmithrandir1489516git

You can also get a review bonus and we will come back to your application sooner.

regards

Hi

I agree, all the formalities must be in place.

That aside, this module does what I need for my Nodequeue integration with Views. I'm not sure what more functionality this module needs to be reviewed? I actually like this modular non-monolithic approach.

I'll look through the module reviewing it also.

We are currently discussing how much code we need, this module is not very big, but as I said regarding the ctools integration - I personally can look over this fact.
If it's not possible/usefull to add more functionality it depends on a second opinion, however, we can still promote this single project manually to a full project. Then you had to have another try with a little more complex module later.

The correct git clone command is
git clone http://git.drupal.org/sandbox/Mithrandir/1489516.git nodequeue_context

Hello ,

Manual Review :

1.Use t() for all texts used in nodequeue_context.module .
ex: function nodequeue_context_menu() contains 'title' => 'Autocomplete' can be 'title' => t('Autocomplete')
2.Use modules/theme names for function to avoid name collisions in nodequeue.inc .
ex : function nodequeue_context_context_convert
other functions looks like function nodequeue_context_context_nodequeue_settings_form_submit.

Thanks

1) Run the code past the review system at:

http://ventral.org/pareview

I noticed a couple of trivial style issues while walking through that you should correct.

2) Speaking as someone who recently climbed the Panels learning curve, I think that the project page and README file could do with a little more documentation to establish context - i.e. why would you use this, what are ctools contexts - aimed at people who don't know their way around the innards of Panels or Views. The project page in particular is sparse; one or two additional paragraphs at minimum.

A screenshot of the module in use on the Panels configuration screens would be excellent, and go a long way to saying exactly what it does for the user.

3) Shouldn't you declare ctools as a dependency in the module .info file?

4) Here:

return drupal_strtolower(str_replace(' ', '-', $context->data->name));

I'd say use preg_replace() instead of str_replace(), even for trivial stuff like this. In my eyes it's better to assume that people are going to run multibyte characters through your code at some point in ways that aren't necessarily going to be safe for the UTF-8/str_replace combination.

5) For large numbers of nodequeues, this is going to be slow:

  else {
    $nodequeue = db_query('SELECT qid FROM {nodequeue_queue} WHERE LOWER(title) = LOWER(:title)', array(':title' => $qid))->fetchObject();
  }

Comparing against a function operation on a column means that existing indexes on the column can't be used. Possible remedies include finding a way of doing the lookup differently, accepting case sensitivity, or putting into the project documentation a recommendation to add a function index for lower(title) - which mysql DBAs will hate you for unless things have become radically better on that front in the past few years.

6) This is the sort of project where unit test code would be great; it gives a great deal of comfort as to proper functioning. Needs a .test file and simpletest class - not that this is a blocker for project promotion, but since by the look of earlier posts you're looking at expanding this small module, that should probably be a priority somewhere in your list.

1.) Nice. I'll try this on my modules, and see just how much they fail :-)

4.) I guess you suggest this for consistency and potential future changes, as the seach/replace characters are within the ASCII range. What about mb_str_replace()? Or is that actually slower than preg_replace()? Perhaps Drupal could use a drupal_str_replace() function?

5.) Hmmm. By forcing lower on both ends, you are essentially eliminating case sensitivity. MySQL is case insensitive to begin with on varchars (and other non-binary types) AFAIR. Does this apply for other db's as well?

4) But $context->data->name may be multibyte.

5) Matters are unpleasant for Oracle and function indexes as I recall, not sure about PostgreSQL, as it's been a while. All of these databases have different ways of managing case insensitivity versus sensitivity in string comparisons via types and default behavior, etc. In general avoiding the need for function indexes is a good thing.

4.) Irrelevant. All utf8 characters have the high bit set in every byte if the codepoint is higher than 127, therefore str_replace(' ', '-', $sdfsdf) will only replace 0x20 bytes in the string with the 0x2D. And the only character a 0x20 byte can represent in UTF8 is ' '.

5.) Ok. Btw, function indexes are not possible in MySQL I think? The only alternative to LOWER() is to just require that the input is in correct case (which is not too unreasonable?). Perhaps the technique used in Term lower name could be used? But that may be overkill. But does this issue really matter? This query is called upon validation when pressing save, not on each request. How much slower are we actually talking here?

4) Fair enough. In the present large internationalized project I'm working on, we've taken the approach of banning all functions like str_replace - it makes for easier code review than considering each case individually. Your situation is not our situation, of course.

5) In MySQL, you create a secondary column with the results of the function in it and index that - which we always called a function index, probably unhelpfully. Not the same thing as an actual function index in Oracle, etc.

You are right in that it isn't horrible in terms of user experience until you have a lot of nodequeues, hundreds or thousands, and that data is changing modestly frequently such that query caching isn't helpful - but you have little idea as to the range of scenarios in which your code will be used, so you have to assume the worst.

I don't think it's at all unreasonable just to strip out the lower()s and let DBAs and developers use their normal assumptions on the default case sensitivity behavior of their database.

Hi

I just tested the query with 2mil nodequeues. The query takes between 1 and 2 secondes to complete. For me it boils down to this: Feature vs performance. Do we want this feature, that the title can be case-insensitive in case of a typo or do we need to optimize for this?

I think this is microoptimization and also optimizing for the "1%". If the query had taken 10 seconds, I could agree that we could eliminate the lower() in favor of being scalable (when a user presses "save"). But I believe it scales fairly well.

Don't get me wrong, I'm a big fan of doing optimizations and doing things "right". But features cost processing time. If we want this feature in the module, is it worth the extra processing time? I believe it is.

Thoughts?

Hard to argue with data. Perhaps I'm just in the 1% when it comes to people who would be irked if I did run into issues and looked through third party code to find database function calls used on string comparisons.

Reasonable people can differ, of course, and it would be nice to see more developers in the world whose first response is to go out and run a test to gather some data as you did.

In any case, you have demonstrated that your code is fine on these two points that we've been discussing.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

Confirmed git clone statement