Machine names

Just tested this to deploy nodequeue changes using deploy module and it works beautifully - it would be incredibly awesome to get this in.

Actually, it would be even more awesome if the update hook would generate nicer names for existing nodequeues, i.e. use a machine-name version of the title if it has one (which probably most do), otherwise resort to this "queue{$qid}" naming scheme.

I'm really excited to see work in this direction -- Thank you.

I need to set aside time to review the patch in more detail, hopefully this weekend, but making sure machine names don't clash is very important.

Overall, I want to help to get this committed, even if it is in an early stage, as long as it gives us a solid foundation to build off of without breaking existing functionality.

Thanks :D!

OK, points taken. I guess I could live with the 'uglier' names and change them manually as you said if necessary.

I just tried machine-names2.patch and It contains a bug which makes it impossible to edit nodequeues because of the validation, in nodequeue.module on line 724 it shows:
'#default_value' => isset($queue->name) ? $queue->name : NULL,

I would remove it and place the following two lines below the array:
if(isset($queue->name))
$form['name']['#value'] = $queue->name;

That way you won't get any validation errors.

subscribe

Wow, this is a very thorough patch! Most of it looks great, however I had a few suggestions after looking through it. I'm sure you had reasons for doing things the way you did, and you have a better understanding of the underlying architecture than I do, so take it all with a grain of salt. :)

+++ nodequeue.install	3 Jun 2010 20:10:40 -0000
@@ -424,6 +434,44 @@ function nodequeue_update_6005() {
+  if ($db_type == 'mysql' || $db_type == 'mysqli') {
+    $queue_update_sql = "UPDATE {nodequeue_queue} SET name = CONCAT('queue', qid)";
+    $subqueue_update_sql = "UPDATE {nodequeue_subqueue} SET name = CONCAT('queue', qid, '_subqueue', sqid)";
+  }
+  else {
+    $queue_update_sql = "UPDATE {nodequeue_queue} SET name = 'queue'||qid";
+    $subqueue_update_sql = "UPDATE {nodequeue_subqueue} SET name = 'queue'||qid||'_subqueue'||sqid";
+  }
+
+  $ret[] = update_sql($queue_update_sql);
+  $ret[] = update_sql($subqueue_update_sql);

It seems like it might be safer to just load up all existing queues using the API and then use php to generate the new name. Then you wouldn't have to worry about the $db_type.

+++ nodequeue.module	3 Jun 2010 20:10:40 -0000
@@ -1480,6 +1497,20 @@ function nodequeue_load_queues_by_type($
+function nodequeue_load_queue_by_name($name) {
+  $queue = db_fetch_object(db_query("SELECT qid FROM {nodequeue_queue} WHERE name = '%s'", $name) );
+  return isset($queue->qid) ? nodequeue_load_queues(array($queue->qid) ) : array(); ¶
+}

Seems like this could be simplified to:

return ($qid = db_result(db_query("SELECT qid FROM {nodequeue_queue} WHERE name = '%s'", $name))) ? nodequeue_load_queues(array($qid)) : array();

+++ nodequeue.module	3 Jun 2010 20:10:40 -0000
@@ -1782,6 +1813,20 @@ function nodequeue_load_subqueue($sqid, 
+function nodequeue_load_subqueue_by_name($name) {
+  $subqueue = db_fetch_object(db_query("SELECT sqid FROM {nodequeue_subqueue} WHERE name = '%s'", $name) );
+  return isset($subqueue->sqid) ? nodequeue_load_subqueues(array($subqueue->sqid) ) : array(); ¶
+}

See above.

+++ nodequeue.module	3 Jun 2010 20:10:40 -0000
@@ -1972,6 +2021,13 @@ function nodequeue_add_subqueue(&$queue,
+  // If $title is null, we'll use the sqid. This will guarantee us uniqueness
+  // on this particular install.
+  if (!$title) {
+    $subqueue->name = sprintf('%s_%s', $queue->name, 'queue' . $subqueue->squid);
+    db_query("UPDATE {nodequeue_subqueue} SET name = '%s' WHERE sqid = %d", $subqueue->name, $subqueue->squid);
+  }
+

Can you explain a bit more what's going on here? The name field isn't required on subqueues or something?

+++ includes/views/nodequeue.views_default.inc	3 Jun 2010 20:10:40 -0000
@@ -16,7 +16,7 @@ function nodequeue_views_default_views()
-    $view->name = "nodequeue_$queue->qid";
+    $view->name = "nodequeue_$queue->name";

This change would seem to potentially cause lots of problems on existing sites. For instance any php acting on the view name, or any CSS styling or javascript fanciness as the block ids and view classes would all change.

+++ includes/views/nodequeue_handler_relationship_nodequeue.inc	3 Jun 2010 20:10:40 -0000
@@ -62,18 +62,29 @@ class nodequeue_handler_relationship_nod
+    $join->construct();
+    $alias = $join->definition['table'] . '_' . $join->definition['left_table'];
+    $this->alias = $this->query->add_relationship($alias, $join, 'nodequeue_nodes', $this->relationship);
+
+    // Add our nodequeue_queues table too
+    $join = new views_join();
+    $join->definition = array(
+      'table' => 'nodequeue_queue',
+      'field' => 'qid',
+      'left_table' => 'nodequeue_nodes_node',
+      'left_field' => 'qid',
+      'type' => 'INNER',  ¶
+    );

Can you explain why this join is now necessary?

Powered by Dreditor.

Oh, I just realized I reviewed the first patch, not the second. Dangit.

Ah, ok, thanks for the update! I'll do a more thorough test of this patch today. :)

following the progress.

Subscribe

subscribe

+1

Tested the machine-names2.patch and found a few issues when using the 6.x-2.x-dev branch.

First, it looks like enough time has gone by that the hook_update_N is now out of date. The number needs to be increased by one.

Second, also ran into an issue when editing a nodequeue. Definitely not the most elegant solution, but its getting late and I found this to get around the issue. The problem is when a field is required, the form value must be supplied. That is great except when you disable the field (for example: edit the form) the form value does not get returned. So leaving it required and disabling it creates a an error that can not be resolved. The form wants a name, but you can not enter it. The code below is what I used to replace the "name" field in "nodequeue_edit_queue_form" function.

<?php
  // The name is required for setting up a new node queue.
  // The name is disabled on edit, but this causes issues because
  // no values are passes when a form element is disabled.
  // To keep the form display from changing greatly, the display
  // form name is changed and the real value is hidden.
  if (!isset($queue->name)) {
    $form['name'] = array(
      '#type' => 'textfield',
      '#title' => t('Name'),
      '#size' => 50,
      '#maxlength' => 32,
      '#description' => t("This is the unique name of the queue. It must contain only alphanumeric characters and underscores. It is used to identify the queue internally. This name cannot change. The machine name of the subqueue will be automatically generated using the queue's machines name and the subqueue title (if provided). If no title is provided for the subqueue, the subqueue's machine name will be numeric. (e.g. machinename_subqueue1)"),
      '#required' => TRUE,
    );
  }
  else {
    $form['name_display'] = array(
      '#type' => 'textfield',
      '#title' => t('Name'),
      '#size' => 50,
      '#maxlength' => 32,
      '#description' => t("This is the unique name of the queue. It must contain only alphanumeric characters and underscores. It is used to identify the queue internally. This name cannot change. The machine name of the subqueue will be automatically generated using the queue's machines name and the subqueue title (if provided). If no title is provided for the subqueue, the subqueue's machine name will be numeric. (e.g. machinename_subqueue1)"),
      '#default_value' => $queue->name,
      '#disabled' => TRUE,
    );
    $form['name'] = array(
      '#type' => 'hidden',
      '#value' => $queue->name,
    );
  }
?>

Third, to keep the "nodequeue_save" function from adding NULL "name" keys into the table I change the following line of code:

<?php
    db_query("UPDATE {nodequeue_queue} set size = %d, title = '%s', subqueue_title = '%s', link = '%s', link_remove = '%s', owner = '%s', show_in_links = %d, show_in_tab = %d, show_in_ui = %d, i18n = %d, reverse = %d, reference = '%s' WHERE qid = %d", $queue->size, $queue->title, $queue->subqueue_title, $queue->link, $queue->link_remove, $queue->owner, $queue->show_in_links, $queue->show_in_tab, $queue->show_in_ui, $queue->i18n, $queue->reverse, $queue->reference, $queue->qid);
?>

to

<?php
    // Allow nodequeue to save and not update the name unless supplied
    if (isset($queue->name) && !empty($queue->name)) {
      db_query("UPDATE {nodequeue_queue} set size = %d, title = '%s', subqueue_title = '%s', link = '%s', link_remove = '%s', owner = '%s', show_in_links = %d, show_in_tab = %d, show_in_ui = %d, i18n = %d, reverse = %d, reference = '%s' WHERE qid = %d", $queue->size, $queue->title, $queue->subqueue_title, $queue->link, $queue->link_remove, $queue->owner, $queue->show_in_links, $queue->show_in_tab, $queue->show_in_ui, $queue->i18n, $queue->reverse, $queue->reference, $queue->qid);
    }
    else {
      db_query("UPDATE {nodequeue_queue} set size = %d, title = '%s', name = '%s', subqueue_title = '%s', link = '%s', link_remove = '%s', owner = '%s', show_in_links = %d, show_in_tab = %d, show_in_ui = %d, i18n = %d, reverse = %d, reference = '%s' WHERE qid = %d", $queue->size, $queue->title, $queue->name, $queue->subqueue_title, $queue->link, $queue->link_remove, $queue->owner, $queue->show_in_links, $queue->show_in_tab, $queue->show_in_ui, $queue->i18n, $queue->reverse, $queue->reference, $queue->qid);
    }
?>

Got to update my editor so I can create a patch for this. Though I would post my findings so others can see and get feedback on them.

subscribe

I just marked #373174: Export and import capability for nodequeue postponed on this issue. Even without the full ctools exportables and features integration (which would be great), machine names would be a big help just for exported views involving nodequeues. I don't have time right now to review the patch in detail, but based on #10 and #19, it seems this patch has some known problems and should be re-rolled. Hence, needs work.

One quick reply to #19: I've run into this problem over and over with #disabled TRUE not showing up in form_state['values']. In fact, I could have sworn chx and I discussed this in a form API issues somewhere, but I'm ashamed to admit I can't find that issue anywhere (neither google nor d.o issue search is revealing it to me). I also can't find some definitive examples in any of my own modules of The Right Way(tm) to solve this. I believe the basic approach of separate form elements -- one as a #type 'value' and the other as a display-only element is the right one, but I could be wrong. ;) However, the specific example code in #19 could be re-written to re-use most of the form element array, instead of duplicating most of it.

Anyway, I sure hope to see this move forward, and will try to help if I can.

Thanks mundanity for updating the version. I was looking at this yesterday and was curious if this was to be done in 6.x or 7.x.

Updated the previous patch with changes to the form based on comments #19 and #21. Also added a check in the form validation for a unique machine name. As for the other comments, I took comment #12's response to #10 to be that no changes were required.

Found one more thing. The install script schema was missing the unique attribute for the name field.

+1, subscribe, and all that jazz...

The patch in #24 appears to be working fine for me.

subscribe

+++ nodequeue.module	Thu Oct 21 22:01:04 PDT 2010
@@ -1962,7 +2052,11 @@
+  if ($title) {
+    $subqueue->name = sprintf('%s_%s', $queue->name, preg_replace('/[^[:alnum:]_]/', '_', $subqueue->title));
+  }

This is problematic for non latin languages.
E.g taxo smartqueues, with term names in cyrillic chars or whatever, will produce subqueue names like queue_name_________,
queue_name_______,
queue_name___________,
with many clashes...

It seems like the tid should be used instead.
I'm not sure that non portable *subqueue* machine names is an issue. Taxo terms are not exportables, subqueue names cannot be transposed. What's important is that the nodequeue definition has a machine name. No ?

Powered by Dreditor.

Contrary to what its phpdoc implies, nodequeue_load_queue_by_name() doesn't return a queue object, but an array of one queue, keyed by its qid, which is the format returned by nodequeue_load_queues().

Since the function name says "nq_load_queue_*" (singular) rather than "nq_load_queues_*", it should have a return format similar to nq_load_queue() rather that nq_load_queues()

+++ includes/views/nodequeue_handler_relationship_nodequeue.inc	Thu Oct 21 19:35:57 PDT 2010
@@ -62,18 +62,32 @@
+    // Add our nodequeue_queues table too
+    $join = new views_join();
+    $join->definition = array(
+      'table' => 'nodequeue_queue',
+      'field' => 'qid',
+      'left_table' => 'nodequeue_nodes_node',
+      'left_field' => 'qid',
+    );
+
+    if (!empty($this->options['required'])) {
+      $join->definition['type'] = 'INNER';
+    }

The additional join is needed, since the relationship adds a condition on queue_name, which is not stored in {nodequeue_nodes} but in {nodequeue_queue}.

However, the join itself doesn't need to be 'INNER' if the relationship is required.
The inner join is between {node} and {nodequeue_nodes}, and that's good enough.

Marking that join as INNER makes it a "different" join than the one added by a "nodeque name" argument handler, and the resulting query joins on an unneeded additional copy of nodequeue_queue.

Powered by Dreditor.

OK, here's a new patch with changes to the nodequeue_add_subqueue function. It addresses yched's concern in #27 and also makes it possible to pass in a machine name for the subqueue. This allows modules that programmatically create nodequeues and subqueues to have more control over how the subqueues are named.

The only other change I made was to the form and how we deal with the machine name field on add/edit: if we're editing an existing queue, and thus have a machine name for it, we just make sure we pass this as the #value and then we can use the same required field even though it's disabled.

Thks katbailey :-)

Remarks in #28 and #29 still apply.

Further thinking about #29, the additional sql join any time the 'nodequeue' relationship is used in a view is a bit problematic - adds a non-minor performance hit on what is probably 80-90% use cases for nodequeues.
Before :

SELECT * FROM node 
[LEFT|INNER] JOIN nodequeue_nodes ON node.nid = nodequeue_nodes.nid
WHERE nodequeue_nodes.qid = %d

After:

SELECT * FROM node 
[LEFT|INNER] JOIN nodequeue_nodes ON nodequeue_nodes.nid = node.nid
LEFT JOIN nodequeue_queues ON nodequeue_queues.qid = nodequeue_nodes.qid
WHERE nodequeue_queues.name = '%s'

This could be avoided if the nodequeue_nodes table contained the 'name' column instead of just the qid.
Shouldn't be too difficult to add since the update function already generates a stub 'name' for existing qids.

Ideally, the qid column would be gone completely - however, that would probably mean a complete API rewrite, best suited for a new Nodequeue 3.x branch. If we're aiming at a short term 'exportable queues' result (which we should IMO), we're probably best with redundant qid / name columns...

Yay.
It will be easy enough for me to take this for a spin seeing as I have the previous version up and running in a few places and your changes only affect the views stuff.
Will report back once I've tried it out.

Nice patch!

+    $view->name = "nodequeue_$queue->name";

The machine name is alnum only but just to be sure, check plain wouldn't hurt? (+ few other places)

Running a soon-to-go-live site on a previous version of the patch, I didn't find the time yet to try the latest iterations :-/

One remark however - the following code generates a subqueue machine name :

+        $name = sprintf('%s_%s', $queue->name, preg_replace('/[^[:alnum:]_]/', '_', $title));

This will badly break with taxo smartqueues since the {nodequeue_subqueue}.name column is currently varchar(32) and has a unique key. Taxo smartqueues can generate quite long titles, resulting in truncated names, which in turn are rejected by the unique index.

As I pointed in #27, a subqueue is identified by a queue and a reference. What's the use for a machine name for subqueues ?

Ugh, I thought I had covered all bases with my subqueue-naming code, but had completely left out all consideration of the varchar(32) restriction. Would something like this work?

   if (!$name) {
    if ($title) {
      $stripped = preg_replace('/[^[:alnum:]]/', '', $title);
      if (!empty($stripped)) {
        // This means we do have some alpha-numeric characters in the title that 
        // can be used to create the name.
        $clean_title = preg_replace('/[^[:alnum:]_]/', '_', $title);
        // Get rid of multiple underscore characters
        $clean_title = preg_replace('/_+/', '_', $clean_title);
        // Set aside at least 12 characters for the title portion of the name.
        $name = sprintf('%s_%s', truncate_utf8($queue->name, SUBQUEUE_NAME_MAX_LENGTH - 13), truncate_utf8($clean_title, 12));
      }
    }
    if (!$name) {
      // If we still don't have a name we'll just use the queue name. We take care
      // of uniqueness when inserting it.
      $name = $queue->name;
    }
  }
  
  // At this point we will have a valid name for our subqueue.
  // If the name isn't unique our insert will fail and return false, in which case
  // we'll add "_{count}" and keep trying until we get a unique name.
  $count = 0;
  while (!@db_query("INSERT INTO {nodequeue_subqueue} (qid, reference, title, name) VALUES (%d, '%s', '%s', '%s')", $queue->qid, $insert_reference, $title, $name)) {
    $len = (SUBQUEUE_NAME_MAX_LENGTH - 1) - (floor(log(++$count, 10)));
    $name = sprintf('%s_%d', truncate_utf8($name, $len), $count);
  }

It's a bit gross, but we definitely need machine names on subqueues.

My use case is deployment of subqueue contents from a staging environment to a QA environment and then on to production. Our sites use modules that automatically generate a subqueue for every enabled language for particular queues. They specify their own way of naming the subqueues (now that I have them using the version of the patch that allows you to pass in the desired name) and when a content producer deploys e.g. the "German featured videos", the deploy process uses the subqueue machine name as the basis on which to synchronise between environments.

My point is :

If your subqueue 'reference' (in your case, a langcode, I guess) is predictable from staging to QA to production (either it's a constant value or you have mappings), then you can unambiguously address your subqueue by its reference and the name of its parent queue. No need for a subqueue name, whether the code to generate it is trivial or, as in #40, complex and hard to predict ;-).

If your subqueue 'reference' is not predictable (numeric ids), then you can't match subqueues anyway. The 'reference' is what nodequeue internally uses to check which node is eligible in which subqueue.
Taxo tids are currently not migratable out of the box. You can't expect taxo smartqueues, based on tids, to be migratable either. Generating a machine name out of taxo term names is bound to be utterly complex because of non-latin chars, arbitrary length,etc , and won't work anyway, because the 'reference' is what matters.

We're currently using a version of the patch close to #24 with taxo subqueues (with fixed tids between site instances), and make heavy use of queue names, with no use for subqueue names whatsoever.

At best, subqueue machine name can be "[queue_name]_[subqueue_reference]". I still fail to see the actual use.

I would have gone that route for subqueue deployment if there was a unique key on the qid and reference fields, but as is you can have multiple subqueues of the same queue with the same reference, so in a sense you are relying on other people's code not doing silly things (e.g. creating multiple subqueues of the same queue with the same reference) for this to be guaranteed to work flawlessly.

I admit I also don't like the idea of going up the chain in my source environment to find a queue name, send that to my target environment, then go down the chain again with various queries to find the right subqueue to put the nodes into; it seems reasonable to expect that I can just reference the immediate container (i.e. the subqueue) and send that along.

I haven't used smartqueues at all so am unfamiliar with the problems there.

It would be interesting to hear about other people's use cases for this.

Oh and by floor() above I of course meant ceil() :-P

I'm not sure subqueues can or should have names. Subqueues can be dynamically created based upon other data, and their criteria is usually static. Naming them doesn't add much.

merlinofchaos +1...
I don't see any use for subqueue machine names.

re @katbailey "I admit I also don't like the idea of going up the chain in my source environment to find a queue name, send that to my target environment, then go down the chain again with various queries to find the right subqueue to put the nodes into"

no need to ping your source environment, this is all done locally :

$queue = nodequeue_load_queue_by_name($name);
$subqueue = nodequeue_load_subqueues_by_reference(array($queue->qid => array($subqueue_reference));

- For a regular nodequeue, $subqueue_reference == $queue->qid.
- For a smartqueue, '$subqueue_reference' depends on the smartqueue type, it's basically term ids for taxonomy smartqueues. If the 'references' are different between your site instances, you can't expect nodequeue to solve that on its own - i.e you can't expect nodequeue to solve the 'machine name' issue for taxo terms.

The sample code above relies on the API in the current patch. It could be made a little easier, like :

$subqueue = nodequeue_load_subqueue_by_name($queue_name, $subqueue_reference);

but that's only a question of API shape, not a question of adding a doomed-to-fail subqueue name.

@yched, I didn't mean I'd have to ping the remote environment, I meant that when deploying the contents of a particular subqueue, I'd have to grab the name of the queue, send that along with the reference, then on the remote end, i.e. where I'm actually saving the subqueue order, derive the correct subqueue using this information. As opposed to just passing a subqueue name and being done. And as I said, there is absolutely no restriction on how the subqueue reference field is used (you only mentioned the two most common uses), so we could end up with 2 subqueues with the same qid and reference.

Having said that, I would be happy to test a version of the patch with your recommended solution ;-)
I think it's high time we came to some consensus on this and got something in.

The point is, the subqueues are dynamic and highly dependent upon the reference. If the reference is somehow named or otherwise unique, then deployment is trivial. tids aren't unique but it's trivial to create some kind of translation table that is.

You don't make subqueues specifically named; naming them doesn't help you if the references (i.e, tids) are not cross-site safe.

"Perhaps the original intention of subqueues is to be wholly dynamic, but we're definitely using them programatically in places, where having a well defined machine name is critical"

Possibly, yet : how do you dynamically generate reliable, transportable and unique machine names out of serial ids ?

"As a generic idea, any sort of content container (IMO) should have a machine name"

Possibly too, but at this point we all know too well that this is not the case, and IMO nodequeue cannot and should not solve that issue in place of taxonomy (for taxo smartqueue) or any random 'entity' with serial ideas. References cannot magically be made more transportable than the thing they reference.

I guess the issue boils down to :
Does nodequeue support having different subqueues of the same queue with the same 'reference' ?
Off-hand I'd say no, but I could very well be wrong.

If yes, then [queue name + subqueue reference] cannot be considered unique, and subqueues do need a name to disambiguate. For taxo smartqueues, or for other types of dynamic subqueues based on dynamic serial ids, I fail to see how the name can be different than [queue_name]_[reference].
Meaning IMO that nodequeue_add_subqueue() shouldn't try to be smart and generate names automatically if none is provided. Smartqueue needs to explicitly build the subqueue name out of vocab ids.

If not, then [queue name + subqueue reference] is unique and unambiguous.
If you use subqueues on stuff with machine names of their own, then reference = machine name, and transportability is a non issue. You just address your subqueue with two strings instead of one, I still don't get how that could be a problem ?
If you use subqueues on stuff with serial ids, then the issue of transportability of subqueue references is strictly equal to the issue of the transportability of those ids. Not for nodequeue to solve.

subscribe

> how do you dynamically generate reliable, transportable and unique machine names out of serial ids ?

One solution is to use the uuid & uuid_features modules.

We could potentially export subqueue references with UUID. This approach works well for menu links: #968826: Export Menu links with UUID

> Does nodequeue support having different subqueues of the same queue with the same 'reference' ?
The API doesn't explicitly prevent this, though I can't think of a use case where a Smartqueue module would need to do this. I think it's safe to assume this won't happen, or a least consider it a super-duper edge case.

In general, addressing a subqueue by queue and reference seems sufficient to me.

Attached is a re-roll of #36 with the subqueue name-related code removed.

I would prefer to keep the existing qid-based Views relationship, even if we label it "deprecated" so that we don't break 11,386 sites once the Views cache is cleared ;).

The other benefit is that we can leave in place the old relationship which as pointed out in http://drupal.org/node/817558#comment-3709294 requires one less join ;).

This patch:

- removes some missed text about subqueue machine names from the earlier patches
- adds a new Views relationship for queue machine name while preserving the existing relationship so that we don't break existing views.
- maintains the qid relationship in default views since it is likely to be more performant.

In my testing I was able to swap out the Nodequeue relationships without issue and without having to replace the fields as long as I saved the View in between.

Since this is largely a re-roll of the above, I plan to to commit this so that we can focus on #373174: Export and import capability for nodequeue .

Any feedback before I do so?

Thanks to everyone for the awesome work put into this issue!

Awesome, thanks @ezra-g !

I can't test the patch right now, I'll try to give it a run on monday.
A couple nits meanwhile :

+++ nodequeue.install	30 Dec 2010 21:14:58 -0000
@@ -11,6 +11,11 @@ function nodequeue_schema() {
+      'name' => array(
+        'description' => t('The machine name for the queue'),
+        'type' => 'varchar',
+        'length' => 32,
+      ),

Is there a reason we can't push the length a bit ? Why not 128 or 255 ?

+++ nodequeue.module	30 Dec 2010 21:14:58 -0000
@@ -1483,6 +1525,38 @@ function nodequeue_load_queues_by_type($
+function nodequeue_load_queue_by_name($name) {
+  $map = nodequeue_get_qid_map();
+  return isset($map[$name]) ? nodequeue_load_queues(array($map[$name]) ) : array();
+}

Can we massage the return value so that it returns a single queue (like the func name implies) instead of an array with 1 or 0 queue ?
Also, the PHPdoc misses a @return statement to clarify all this ;-)

+++ includes/views/nodequeue_handler_relationship_nodequeue.inc	30 Dec 2010 21:14:58 -0000
@@ -63,17 +62,22 @@ class nodequeue_handler_relationship_nod
\ No newline at end of file

I think this is not supposed to happen

Powered by Dreditor.

Applied and tested patch #55, works just fine as far as I can tell :-).

Updated the patch for my remarks in #58 (nodequeue_queue.name column size = 128, nodequeue_load_queue_by_name() return format). Interdiff with #55 attached

One remark, though : changing the existing nodequeue_handler_relationship_nodequeue handler to reference queue names instead of qids will break existing views :-/

I thought I read an intent in this issue to preserve the old handler, append "[deprecated]" to somewhere descriptive, and add a new handler for the new stuff. Next major version of Nodequeue to break stuff.

Sorry folks, I uploaded the wrong patch in #55 that did not include the new Views include files :\.

Here's what I meant to upload - I'll look into integrating this into yched's work.

Thanks for the re-roll yched!

The attached patch should integrate yched's work in #57, including the interdiff, with the views integration from #60. I have undone the changes to the default view names to avoid compatibility issues with existing sites (ie placing a view in a Panel via the view name for a non-overridden default Nodequeue view). If folks need an exportable view, they know enough to create a new view with a new name and relationship.

Because we're not changing the default relationship (we're adding a new one instead) this shouldn't break existing views.

I haven't been able to extensively test this today but this should be consistent with what's above.

Needs review, in part to make sure I didn't clobber important views integration when integrating these patches ;).

Applied and tested #61 on our test site. Works beautifully !
RTBC, as far as I'm concerned.

This is committed: http://drupal.org/cvs?commit=481002 !

Thanks, everyone for the work and testing here! Let's leave this in dev for several days to let folks report any unexpected side effects before making a new release (which is overdue in any case).

@ezra-g is there any reason the new views relationship wouldn't work with views 3.x?

The relationship appears just fine and enables fine, however when I restrict it to a single queue, it doesn't work. It still pulls from every nodequeue instead.

I downloaded the latest dev including this patch. In the file: nodequeue_handler_relationship_nodequeue_queue_name.inc under the query() method starting at line 66:

      $map  = nodequeue_get_qid_map();
      foreach($map as $name => $qid) {
        if (isset($this->options['names'][$name]) ) {
          $qids[] = $qid;
        }
      }

I don't know if this is the intended output or maybe views 3 is output something strange but when I inspect the $this->options['names'] variable i am returned:

[names] => Array
        (
            [queue1] => queue1
            [queue2] => queue2
            [queue3] => queue3
            [queue4] => 0
            [queue5] => 0
        )

I have fixed it by changing the if statement to :

if (isset($this->options['names'][$name]) && $this->options['names'][$name] !== 0) {
  $qids[] = $qid;
}

#65 makes sense. This probably requires an if(!empty()) instead of an if(isset()).

I ported #61 to D7 and needs review.

Sorry, I missed a few data base calls, so it's not working. I'll post a patch in a few hours. Time to lunch, now.

Let's see this time

I'm not a patch expert but I "think" that its failing because your applying a D7 patch to a D6 issue.

#70: machine_name.patch queued for re-testing.

Maybe I'm doing a wrong patch.

[edited out, I was on crack]

@yched: ;-)

re-test #75

Any one knows why this test is failing? It says the patch itself doesn't apply but I can apply it. It's the first time I made a patch for a full directory, maybe it's not right?

--- /Users/robert/Sites/cesca/sites/all/modules/contrib/nodequeue/includes/views/nodequeue.views.inc	2010-12-24 21:55:09.000000000 +0100
+++ ./includes/views/nodequeue.views.inc	2011-01-25 13:59:55.000000000 +0100

The "/Users/robert/Sites/cesca/sites/all/modules/contrib/nodequeue/" part is wrong.
Patches should be generated at the root of the project (i.e, for this hunk, both --- and +++ lines should start with includes/views/...

thanks @yched. let's see now.

I'm starting to use 6.x-2.x-dev but wanted to give a reroll.I've not tested this and don't understand if those two files must be there as of now or the 2nd one in intended to replace the former.

files[] = includes/views/nodequeue_handler_relationship_nodequeue.inc
files[] = includes/views/nodequeue_handler_relationship_nodequeue_queue_name.inc

Attached a patch over HEAD (7.x-2.x-dev). I added the new file and made the diff this way:

nodequeue-HEAD$ cvsdo add nodequeue_handler_relationship_nodequeue_queue_name.inc
nodequeue-HEAD$ cvs diff -up -N > nodequeue-817558.patch

Very much want!

Dreditor shows one whitespace addition, which i don't care about, but:

i think the machine_name form element should be used here?

The machine name element works where there is also a 'human name' or 'label'. Do we have a case for nodequeue labels ?

I've opened a new ticket (with a patch) at #1112504: Views relationship handler for queue name is broken to track the bug that mrconnerton noted in comment #65 for the Drupal 6 branch.

It looks like this bug is also present in the current patch for Drupal 7 at comment #84.

Reroll of #84 with the fix mentioned in #87.

@jonhattan, is there a reason for which we're not using the machine_name element for 7.x?

Would it be easier to make this change as a follow-up after #88 gets in?

@amateescu as stated by @yched in #86, the machine_name is useful for when the item (a nodequeue) has also a human name.

And it does. Isn't 'Title' the first field from the nodequeue add/edit form? :) Or because it's not required we don't consider it as the nodequeue's human name?

I'm just asking, don't think that I have something against the patch from #88 :)

After a chat on IRC with merlinofchaos and ezra-g, the following were recommended for 7.x:

1) use #machine_name
2) change the automatic naming for existing queues (provide more meaningful machine names using the queue title)
3) do not change the views relationship handler, just have the handler automatically transition from qid to name

Here is a patch that implements 1) and 3), along with a couple of other improvements over the one in #88. I will try to finish 2) during the weekend, but if someone else is brave enough to tackle it until then, please do :)

Subscribe. Good work on this, everyone.

An updated patch:

• The patch in #92 wouldn't apply cleanly for me failed because it couldn't find the hook_requirements() block.
• I renamed nodequeue_update_7000() to nodequeue_update_7200(), which would be the first hook_update_N for the 7.x-2.x branch.
• I changed code for amateescu's item #2 to use a shortened version of the queue title with spaces replaced with the underline character; I've a feeling, though, that a more complete solution involving a Drupal string function is going to be required.

Updated patch that wraps the new name value in LOWER() and TRIM()'s the string too, just to be sure. Too bad MySQL doesn't have any regexp-based string replacement functions, like PostgreSQL :p

This patch sponsored by Bluespark Labs.

One final patch for the night, this one loops over all existing nodequeues and updates their names using some PHP code with some regex to reduce strings to more easily digestible chunks - it's slower bug more compatible.

@DamienMcKenna, thanks for starting the work on this.

Continuing from #96, the attached patch fixes these problems in the update function:

• the where clause is not needed because we already checked in the beginning of the update function if the name column exists, and, since it didn't exist, it couldn't possibly have a value because we just created it :)
• use fetchAll() before iterating through the results array
• check if the nodequeue has a title
• check for uniqueness of the generated machine name

Nevermind my second point from #97, it seems I was on crack. Re-rolling without that part.

I think I caught a tiny bug, or at least an inconsistency - shouldn't there be a 'name' attribute on the nodequeue_queue class?

Reading at the patch in #99, this looks like a great start. I feel like this is RTBC and can receive further testing and refinement in the upcoming dev snapshot and alpha releases.

Thanks for the awesome work to everyone involved :D!

Re-rolled against this morning's master codebase, had to move some of the changes to includes/nodequeue.admin.inc.

Fixed the duplicate update hooks.

@DamienMcKenna, thanks for this re-roll. I'm sorry I didn't commit this earlier, but trust me, the memory usage improvements from #462290: Move admin functions to separate .admin.inc are certainly worth it :)

So, I tested this again to make sure that it doesn't break existing views, made a few cosmetic changes and commited the attached patch to 7.x. Thanks everyone!

http://drupalcode.org/project/nodequeue.git/commit/dd1c716

shouldn't there be a 'name' attribute on the nodequeue_queue class?

This change introduced a bug in the patch from #99. The machine names were always disabled because we were checking this property with isset().

Commited attached patch to fix this.

http://drupalcode.org/project/nodequeue.git/commit/1015292

I found that the automatic javascript for setting the machine name was not working. Interestingly changing $form['name'] to $form['machine_name'] fixes the problem. I wouldn't be surprised if this is a bug somehow related to #machine_name source defaulting to 'name'. Attached is a patch with the change I made to get this working. It is a bit a kludge... perhaps 'name' should be 'machine_name' across the board for clarity and consistency?

I'm told that the commit in #104 addresses the issue in #105. #disabled was kicking in.

Yup, #104 was the fix.

side note @ezra-g : do we could get an official D6 release with the machine name changes ?

@yched: see #1156740: Nodequeue roadmap. Is that date ok for you or would you like to see it sooner?

@amateescu : thks, replied over there

FYI I've created a follow up issue to use $name within the default views so that blocks delta are consistent: #1160024: Default views should use $name rather than $qid.

I'm also wondering about nodequeue_load() vs. nodequeue_load_queue_by_name(). All menu items use %nodequeue, so we're assuming there is a known $qid. Wouldn't it be better to default to $name for predictability? Perhaps a wrapper function nodequeue_load_queue_derive() could be used to determine if the input is machine names or ids and then call the appropriate load function.