Hi,

I am setting up the deployment module for a project and extending it so that I can delete content and I noticed a small bug.
In node_deploy.module there is a check to see if the node that is going to be deployed exists (line 104).
If not, it returns: xmlrpc_error($xmlrpcusererr + 1, t('Node not found'));
In deploy.module the return value of node_deploy() is checked. If it is FALSE an error is written to the log. But the value returned when the node does not exist is not FALSE. So it always reports the deployment as success.

Steps to reproduce:

  2. add it to a deployment plan
  3. delete the node
  4. execute deployment plan: the task will return success, while it has failed

the fix that I suggest is:
In deploy.module line 769 replace

if ($xmlrpc_result === FALSE) {

by

if (xmlrpc_errno() != NULL) {

I will create a patch for it after some testing.

CommentFileSizeAuthor
#7 874466-deploy-delete-plan-items.patch6.99 KBgdd
#5 874466-deploy-delete-plan-items.patch7.08 KBgdd
#3 deploy-delete-plan-items.patch6.47 KBdixon_

Comments

mewren’s picture

mewren commented

I am also seeing this

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

I think a better fix would be to not attempt to deploy these nodes at all, and remove them from the plan if they no longer exist. This is a much cleaner solution IMO. This will potentially be a problem in other areas as well (taxonomy, users, anything really.) I will try and look into this soon.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Status: Active » Needs review
StatusFileSize
new deploy-delete-plan-items.patch6.47 KB

The only object that can be put in a persistent plan is nodes, content types and views. All the other objects are either deployed as dependencies or deployed on the go through internal deployment plans. So it only makes sense to implement delete hooks for those types, IMO.

Here is a patch that works as expected for me. I'll put it up here for a review before I commit. This patch also removed the implementation of hook_comment() in comment_deploy.module that didn't do anything useful (the delete operation on the comments_uuid table is handled by deploy_uuid.module.

Edit: The reason why the delete implementation for Views is handled through a form submit handler is because there is no specific hook fired when a view is deleted.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Assigned: johannesdr » dixon_
gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
StatusFileSize
new 874466-deploy-delete-plan-items.patch7.08 KB

I've made a couple of mods to this patch, but conceptually it looks good

- I made deploy_plan_item_delete() a little more flexible, it can now take any combination of fields and values.
- I also realized this is useful as a general API function, so I moved it into deploy.module. After this patch is committed I will update all the other existing functions to use it.
- Some typos and such.

New patch attached.

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented

Yeah, you are right. It probably belongs in deploy.module!

I will test this patch during this week. Hopefully tomorrow.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
StatusFileSize
new 874466-deploy-delete-plan-items.patch6.99 KB

Somewhat simplified patch here, didn't even know about db_type_placeholder() until today!

dixon_’s picture

dixon_
he/him
Primary language English
Location France
commented
Status: Needs review » Fixed

Tested the patch from #7 successfully. Committed with some additional code comments. http://drupal.org/cvs?commit=434582

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.