Exported nodequeues have bad references

I'll review this when I get a chance, but if someone else could as well (@toleillo?) that would be great.

netw3rker, what's the status on Features integration directly in Nodequeue if you know? Is the maintainer reluctant to do so? As I've said elsewhere (#921872: Rewrite to use UUID, merge with UUID Features?), I think all this module's current functionality could be served by other modules in the long run. It was always a hack, basically.

For our purposes this patch gets rid of the qid successfully which has been causing us problems for months so RTBC for me.

Here's a reroll of the patch with -p1 from the features_extra folder, which is standard

Been using #4's patch for awhile now, and I am curious if this can be reviewed and pushed out in a new dev?

The patch in #4 works for me, RTBC. Thanks.

Patch #4 gets my vote :)

The patch was riddled with coding standards violations, here's a cleaner version :)

Patch #8 works as a charm!

It would be better if Patch #4 gets reviewed and merged in dev branch.

#8: 1338000-8-features_extra-nodequeue_references.patch queued for re-testing.

This just rerolls #8.

Rerolled but untested.

@@ -44,7 +41,19 @@ function fe_nodequeue_features_export($data, &$export, $module_name = '') {
       $export['dependencies'][$module] = $module;
     }
     // Otherwise, export the nodequeue
-    elseif (nodequeue_load_queue_by_name($name)) {
+     elseif ($queue = nodequeue_load_queue_by_name($name)) {
+       // Add dependencies for the roles that are associated with these queues.
+       if ($queue->roles) {
+         $manipulate_all_queues = array_keys(user_roles(FALSE, 'manipulate all queues'));
+         $queue->roles = array_diff($queue->roles, $manipulate_all_queues);
+         $roles = user_roles();
+         foreach ($queue->roles as $index => $rid) {
+           $export['features']['user_role'][$roles[$rid]] = $roles[$rid];
+           // Convert the role from a rid to a 'machine name' for saving. This
+           // will be converted back to a rid at save time.
+           $queue->roles[$index] = $roles[$rid];
+         }
+       }
       $export['features']['fe_nodequeue'][$name] = $name;

• Reading the code it is not clear why the users with 'manipulate all queues' are filtered out here. Best add a comment referring to #213074: Usability: Permissions are confusing for the underlying reasons.
• The indentation is off by one.

+           // Convert the role from a rid to a 'machine name' for saving. This
+           // will be converted back to a rid at save time.

I suppose 'converted back to a rid at save time' should be 'converted back to a rid when reverted'?

@@ -106,10 +126,12 @@ function fe_nodequeue_features_revert($module) {
       continue;
     }
 
-    if ($nodequeue_queue = nodequeue_load_queue_by_name($object['name'])) {
-      // Update existing nodequeue in the situation that a nodequeue with the
-      // same name already exists.
-      $object['qid'] = $nodequeue_queue->qid;
+    $map = nodequeue_get_qid_map();
+    if ($qid = $map[$object['name']]) {
+      $object['qid']=$qid;
+    }
+    else {
+      unset($object['qid']);

• I'm afraid it is not a good idea to unset the object here when the qid does not exist. This function is not only called when reverting a feature but also when rebuilding or enabling a feature. It would prevent new nodequeues from being created. I have not tested this though so I could be mistaken.
• Coding standards: the assignment operator should be surrounded with a space.

@@ -138,6 +160,15 @@ function fe_nodequeue_features_enable_feature($module) {
  * @return array
  */
 function _fe_nodequeue_save_queue($settings = array()) {
+  // Convert roles from names to rids.
+  $roles = array_flip(user_roles());
+  foreach ((array) $settings['roles'] as $index => $role){
+    // In case we are dealing with an old export with rids, don't do anything.
+    if ((int) $role) {
+      continue;
+    }
+    $settings['roles'][$index] = $roles[$role];

Casting to an (int) is not the right way to check if a variable contains an integer, it would return a false positive if a role machine name would start with a number. Use is_int() instead.

Updated the patch. I addressed everything from #16, except this part

@@ -106,10 +126,12 @@ function fe_nodequeue_features_revert($module) {
       continue;
     }

-    if ($nodequeue_queue = nodequeue_load_queue_by_name($object['name'])) {
-      // Update existing nodequeue in the situation that a nodequeue with the
-      // same name already exists.
-      $object['qid'] = $nodequeue_queue->qid;
+    $map = nodequeue_get_qid_map();
+    if ($qid = $map[$object['name']]) {
+      $object['qid']=$qid;
+    }
+    else {
+      unset($object['qid']);

My comment above was wrong, this object is not being unset. It is not clear to my 8AM brain why this code is necessary, a small comment would be nice :)

Leaving it to "needs work" for the moment.

#4: features_extra_nodequeue_reroll-1338000-3.patch queued for re-testing.

#17: 1338000-17-features_extra-nodequeue_references.patch queued for re-testing.

I have reviewed this again, and the part I was doubting in #17 seems very obvious to me now lol. This really needs some tests, but there are no tests for the nodequeue support yet. I'm going to create branch 1338000, commit this patch and start working on tests.

#17 works for me, thx!

Rerolled the patch for latest features_extra.

It seems nodequeue_load_queue_by_name is no longer availabel to nodequeue 7.x-3.x
Use nodequeue_load instead.

@moonray, there's an issue for that :) #1845752: Make fe_nodequeue work with nodequeue-7.x-3.x-dev

Should have linked to that myself. Just thought I'd mention it in relation to this issue.

I have been writing tests and have hit a bug in Nodequeue. When a nodequeue has changed it is impossible to check later in the same page request if the feature is overridden. This is due to Nodequeue's use of static variables, as described in this issue: #1666556: Use drupal_static instead of static variable..

As the development of Nodequeue is moving rather slow lately I am going to implement variants of these functions in FE. This will allow us to move forward on this issue without having to wait on Nodequeue being patched.

This has been fixed and merged into 7.x-1.x.

Commits:

88d8d12 Test reverting of nodequeues.
85f5d1a Don't flush caches after reverting features. This should be done during the revert.
66f0119 Added documentation.
e68f192 Provide a version of nodequeue_load_queue_by_name() that passes $bypass_cache.
346a2e3 Coding standards.
e498fe1 Use the new version of nodequeue_get_qid_map() and clear its static cache when needed.
f223014 Provide a version of nodequeue_get_qid_map() that allows to clear its static cache.
0cc899e Whitespace.

Removing tags