A clean CRUD API is a nice thing to provide for other modules to integrate with privatemsg (in my case, to implement migration of private messages). Attached patch renames _privatemsg_send() to privatemsg_message_save(), and adds privatemsg_message_delete().

Note that moving the send/save function so the CRUD functions are grouped together makes the patch look bigger than it really is.

Files: 
CommentFileSizeAuthor
#11 1184984-privatemsg_crud-11.patch4.71 KBcthos
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]
#10 1184984-privatemsg_crud-10.patch3.99 KBjrbeeman
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]
privatemsg_crud.patch8.35 KBmikeryan
PASSED: [[SimpleTest]]: [MySQL] 3,101 pass(es).
[ View ]

Comments

Status:Active» Needs review

Version:7.x-1.x-dev» 7.x-2.x-dev

The public API functions for sending messages are privatemsg_new_thread() and privatemsg_reply().

_privatemsg_send() is not supposed to be a public API function, it's an internal helper function which is used by these two functions. The other functions ensure that all necessary keys of the $message object exist, validate the message and so forth. I know that validation in API functions is somehow problematic, but it's an important part for private messages. Users can disable it and shouldn't receive any messages, or their inbox can be full if the privatemsg_limits is installed, or they could be blocking the author and so on. Also, updating existing messages is not supported, which might not be clear, as all $entity_save() allow to update existing entities.

Similar for the delete function, we already have API functions to mark messages as deleted, privatemsg_message_change_delete() and privatemsg_thread_change_delete(). I know, that just marks them as deleted, but that's what delete means in our context. So, if that function is added, it should be called privatemsg_message_flush().

And 7.x-1.x is stable, any API changes need to happen in 7.x-2.x.

Status:Needs review» Needs work

I missed that _privatemsg_send() doesn't handle update, I need to address that to fully support migration for privatemsg (Migrate supports running imports in update mode, for example when field mappings have changed and we don't want to rollback and re-import from scratch to reflect them).

The existing API supports the privatemsg UI, but does not cleanly support dealing with messages as data objects, which is what a true CRUD API would do. I am all for validation in the API - no point in saving objects lacking required fields or self-consistency. The problem here is that _privatemsg_validate_message() assumes it's being called from a privatemsg form (calling form_set_error()), so it (and the send API functions that call it) won't work from migrate, we do need a true privatemsg_message_save() in its place.

I was hoping to have an updated patch, but the deeper I dig the more complex it looks...

http://api.worldempire.ch/api/privatemsg/privatemsg.module/function/_pri... does support validation for API calls, that's what the $form parameter is for.

I see what you say, just wondering why migrate needs to support updating private messages when we don't allow that in the first place ;)

Maybe we *can* add privatemsg_message_save() and keep reply/new_thread as wrappers for that for an easy to use API (that's why we have different functions in the first place). That will probably require quite a bit of work to clean that up properly...

not sure..

Ahh, I just saw the form_set_error() calls and averted my eyes, $form=FALSE is good.

re: adding save and wrapping reply/new_thread - yes, that's exactly the approach I'm trying. And, yes, it does look like a bit of work...

Thanks.

It would be very helpful from a migration perspective to have save() and delete() functions just like our other entities [comment_save, user_save, node_save, file_save, ...]. It could be that the normal use cases don't even call them. Consider that user_delete is (almost) never called by core. Most UI flows go throug user_cancel which is quite different. But user_delete() still exists for a reason. I think thats the analogy here.

BTW, we need to update private messages because their contents or metadata might have changed on the source side. We frequently do propogation of changes like that. Just because our UI does not let you change a message does not mean that developers won't need to change the message store.

Subscribe... I'm trying to get threading to import with Migrate.module, but may not have time to get that done today. #984050: Privatemsg code for 2.x branch related.

Seconding the request for a good CRUD API for creating and saving private messages. I'd like to be able to easily create new private messages from another module.

StatusFileSize
new3.99 KB
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]

Updating patch to latest state of 7.x-2.x. Simplified the patch a bit as well by avoiding the rearrangement of privatemsg_get_link(), privatemsg_message_load(), and privatemsg_message_load_multiple().

privatemsg_message_save() still lacks the ability to update an existing message. I'll try to work on adding that, but help would be appreciated.

Status:Needs work» Needs review
StatusFileSize
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]

The CRUD API provides a privatemsg_message_delete method, which unfortunately does not play well with the message module. The reason is *_message_delete is an entity hook that will get called, passing an entity to privatemsg_message_delete, which it is not expecting. Causing db explosions.

I've implemented privatemsg_module_implements_alter() to tell it to not call the _delete or _load hooks to prevent db errors.

Status:Needs review» Reviewed & tested by the community

We're using this patch on a site that utilizes Privatemsg and Message modules. Verifying that #11 addresses the conflict while also providing a public CRUD API.