Remove hook_form_alter in favor of a more flexible privatemsg_new form [#398916]

Imho (and litwol agreed on irc ;) ) our hook_form_alter is pretty ugly, it does heavily change our own form based on arg().

The attached patch tries to change that and changes the following things:

New privatemsg_new interface:

function privatemsg_new(&$form_state, $recipients = array(), $subject = '', $thread_id = NULL) {

This means:
- Multiple default recipients are possible. There is some pretty ugly code to detect what $recipient is, because it could be an array of user objects or a string like "uid1,uid2,uid3". I tried to make this nicer with using a named menu wildcard but that doesn't work because such a thing doesn't work together with an empty array. it has to return either FALSE or a actual value.
- It is possible to set a subject
- By setting a thread_id, the form "converts" to a reply form, with fixed subject and to field. Currently, the recipients field is displayed as a disabled textfield but that has the downside that not all names will be displayed when there the list is too long. I am not sure if I should change that to a description or so. (Note: the disabled field is _not_ used, it's just displayed.
- Changed the menu hook to allow urls like: /messages/new/1,4,5/Subject
- Changed privatemsg_get_link, it is now possible to provide a subject and an array of recipients. This is a possible fix for #382756: Allow to set subject and body with query arguments

This is nothing important, conflicts with several other patches and is only a first (working but ugly) version, but I'm submitting it to get some feedback...

Comment	File	Size	Author
#8	privatemsg.killformalter6.patch	11.7 KB	Berdir
#8
#5	privatemsg.killformatalter.5.patch	12.23 KB	NaheemSays
#5
#4	privatemsg.killformalter4.patch	11.94 KB	Berdir
#4
#3	privatemsg.killformalter3.patch	11.82 KB	Berdir
#3
#1	privatemsg.killformalter2.patch	11.23 KB	Berdir
#1
#1	reply.png	13.23 KB	Berdir
	privatemsg.killformalter.patch	12.38 KB	Berdir

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Berdir

German

Switzerland

CreditAttribution: Berdir commented 16 April 2009 at 22:48

File	Size
reply.png	13.23 KB
privatemsg.killformalter2.patch	11.23 KB

Ok, attached is a re-roll. Instead of a disabled textfield, it's now displayed as simple text, see screenshot. I think that's better for many users because you can't see all of them in a disabled input field.

Comment #2

NaheemSays CreditAttribution: NaheemSays commented 17 April 2009 at 14:15

Status:

Needs review

» Needs work

when going to messages/new/%, the subject is being set to whatever % is, instead of leaving it blank.

Secondly, going to messages/new/%/subject_title shows a reply form instead of the normal new message form. Is that how its meant to be? if so, then good. if not, then, its still ok. Just not what I expected.

Also if we are dealing with this all, the following was a sort of hack that I added:

  if (empty($to) && $usercount >= 1) {
    // Assume the user sent message to own account as if the usercount is one or less, then the user sent a message but not to self.
    $to[] = $user->name;
  }

It has the nice side effect of allowing the person to reply to itself, but its not really as clever as it could be. We can leave it as is, or we can do something about it. up for ideas.

That current check should always be greater than or = 1 when a person has taken part in a conversation (simply by receiving the message). I guess we could even remove the && $usercount >= 1 bit?

(at the very least, should there be a comment to let people know that it is a hack?)

Comment #3

Berdir

German

Switzerland

CreditAttribution: Berdir commented 17 April 2009 at 17:02

Status:

Needs work

» Needs review

File	Size
privatemsg.killformalter3.patch	11.82 KB

I can't reproduce that, are you sure you cleared the menu cache as privatemsg_menu() changed slightly?

Attached is a updated patch which fixes a few issues with multiple recipients. I also replaced your "hack" with a simple check to only add a a recipient once, it will currently display your name own name too.

That stuff is confusing anyway, because if you write UserA once, and then go to the thread, it will display "Reply to UserA", which isn't actually what you are doing.

Maybe something like:

Reply to thread
Recipients: UserA, UserB

Comment #4

Berdir

German

Switzerland

CreditAttribution: Berdir commented 17 April 2009 at 18:54

File	Size
privatemsg.killformalter4.patch	11.94 KB

No idea why that didn't work, can you try again with that patch?

Comment #5

NaheemSays CreditAttribution: NaheemSays commented 17 April 2009 at 19:20

File	Size
privatemsg.killformatalter.5.patch	12.23 KB

I have re added that logic which I thought was a hack - with the new code it actually worked properly.

As for the display, I like your proposed format in comment 3 - not done here as I have no idea how to.

Apart from that the patch is ready.

Comment #6

NaheemSays CreditAttribution: NaheemSays commented 22 April 2009 at 19:59

Status:

Needs review

» Needs work

This patch needs to be rerolled and may also need to take into account #349389-5: Page titles do not change.

Comment #7

litwol CreditAttribution: litwol commented 23 April 2009 at 04:59

Priority:

Normal

» Critical

Fixing this is top priority, it doesn't make much sence to form_alter own forms when we can just modify the form in code where needed.

Comment #8

Berdir

German

Switzerland

CreditAttribution: Berdir commented 23 April 2009 at 07:28

Status:

Needs work

» Needs review

File	Size
privatemsg.killformalter6.patch	11.7 KB

Re-roll, this also fixes the issue with the title.

Comment #9

NaheemSays CreditAttribution: NaheemSays commented 23 April 2009 at 17:34

Status:

Needs review

» Reviewed & tested by the community

This works well with one caveat, which was mentioned before.

Sending a message to yourself with the method messages/new/% works - as does replies to an existing thread.

However if the new message format is messages/new/%,% where one of the many comma separated uids is your own, that is not entered as a recipient, just all the other uids.

Afaik, that is acceptable as the message will still appear in your own messages and it does not impact on any other aspects of conversation building.

Setting to rtbc

(if anyone else tests this patch, remember top clear the cache beforehand - I have been caught out by that twice now.)

Comment #10

igorik CreditAttribution: igorik commented 16 May 2009 at 12:41

I think that it would be great it admin could set different numbers of allowed recipients for various roles, e.g. 10 for auth. user role, 100 for trusted user role, etc.

thanks
Igorik
http://www.somvprahe.sk

Comment #11

Berdir

German

Switzerland

CreditAttribution: Berdir commented 16 May 2009 at 12:44

That patch is not related to such an option, and you can always use hook_form_alter, no need to change privatemsg.module : http://api.drupal.org/api/function/hook_form_alter/6

However, it is a possible setting that could be added to the message quota module: #69856: Message limits