Port module to 6.x
drunken monkey - December 19, 2007 - 14:24
| Project: | Privatemsg |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed |
Description
Hi!
I've begun porting this module (atm just privatemsg, w/o mail_edit, etc.) to Drupal 6.x, using the 2.x-dev release.
Is there anything special I should keep in mind?
Greets,
dm
#1
Looking at the issues queue, there seem to be lots of issues still open with 2.x-dev. Does it really make sense to try to port it before fixing these issues? When Zen took over Privatemsg from chx, it did not yet have beta quality, and it doesn't seem to have improved dramatically.
BTW, I thought pm_subscriptions was an effort to remove the dependency on the Subscriptions module, but there are still calls to Subscriptions, and some of these calls are invalid. As it is now, Privatemsg is incompatible with Subscriptions 2.0 (currently in beta).
mail_edit is now at http://drupal.org/project/mail_edit — it's not part of Privatemsg anymore!
#2
hm, why not port 2.x and fix that bugs ;) so we get the improvements of the 2.x version.
#3
Chicago Technology Cooperative is also working on a 6.x port based on DRUPAL-5--2.
#4
First version is now done, please review.
Everything I tested seemed to work fine, except for the preview function when writing messages, which I just couldn't get to function properly (or at all). I'd appreciate it, if someone more familiar with the new Form API than I could take a look at it.
Patch is for files privatemsg.module, privatemsg.info and privatemsg.install, and adds privatemsg.pages.inc.
#5
drunken monkey,
Thank you very much for the patch. I really need this module.
#6
The patch no longer applies cleanly. (7 out of 85 hunks failed - I would post the full message but I have no idea how to do so in cygwin...)
#7
Yeah, I had that problem too. I've attached privatemsg.module.rej & privatemsg.info.rej (I had to change the "rej" extension to "diff" to upload).
I'll go through it piece by piece when I have a chance (maybe tomorrow).
#8
Attached is finally a version with the preview bug fixed. Also fixed is a bug in the autocomplete function.
The diff is from the same version as the last one. Unfortunately, I don't know anymore, which one that was. Please check the $Id$ tags in the headers of your file against the ones in the patch file, for determining if you are using the correct one.
#9
Patch updated to match latest in drupal 5.x-2.x-dev. There is an error though - viewing ANY private msg will show the same one - the first one in the database.
#10
Just tested and the same failure happens to the previous patch, so it is not something extra that I have messed up on in updating the patch.
#11
Further investigative work:
Replacing the
%privatemsgplaceholder with%in the menu function will show the correct message, but the following error along with it on messages where the user is the recipient (but not other cases):It does not appear on other pages.
PS@ drunken monkey: Any reason for the special access rules? In my quick tests, they ar the same as having the array in the menu, instead of the menu calling a special function which calls the array with additional bits... But I have no real idea how this module works so I could be way off base.
#12
That error message comes from:
$form['actions']['folder'] = array('#type' => 'select',
'#options' => $extra_folders,
'#attributes' => array('class' => 'pm-spacer'),
);
in the function privatemsg_view_form (starting line 1902).
Attached is an updated patch to get us to this stage.
#13
There was a missing call to privatemsg_load_folders() to load the users folders so an empty variable was being passed to the form builder when it was expecting an array. I added this around there:
<?php$folders = privatemsg_load_folders($user->uid);
foreach ($folders as $key => $folder) {
if ($folder['fid'] != PRIVATEMSG_FOLDER_SENT && $folder['fid'] != PRIVATEMSG_FOLDER_RECYCLE_BIN && $folder['fid'] != $current_folder) {
$extra_folders[$folder['fid']] = $folder['name'];
}
}
$extra_folders['new'] = t('New folder...');
?>
I added the wildcard value "%privatemsg back" into the path of $items['privatemsg/view/%privatemsg/read'] in privatemsg_menu() since you get the message load from privatemsg_load() and it's sort of used in that path. I left the other wildcards as "%" since it was upsetting the access callback for the other pages.
<?php$items['privatemsg/view/%privatemsg/read'] = array(
'title' => 'Read message',
'access callback' => 'privatemsg_special_access2',
'type' => MENU_DEFAULT_LOCAL_TASK,
'weight' => -10,
);
?>
Attached is a patch against 5.x-2.x-dev.
#14
WIth the current patch you cannot send Private messages. Rather, you can, but no one will receive them.
No idea if this was the same with earlier patches or not as I no longer have them lying around.
#15
Hmmm... Seems to be working in my install... I probably should have mentioned that the patch was only for privatemsg.module. I'm guessing you applied the patch, and it didn't fix the privatemsg.install file, and so drupal_write_record() wasn't able to actually write a record since the schema was missing. Sorry about that. I'm a little new to creating patches and not terribly well versed in the protocol... corrected patch (against 5.x-2.x-dev) attached.
#16
It still don't work for me. Could be something wrong with my set up though - I am also having problems running cron - not even manually typing cron.php runs it...
Everything else works fine. (http://www.therevival.co.uk is running Drupal 6 on a live site - I like living on the edge.)
#17
A bit of debugging:
The first new message sent was saved as message with an id of 0. Second message gave the following error:
user warning: Duplicate entry '0' for key 1 query: INSERT INTO privatemsg (author, recipient, subject, message, timestamp, newmsg, hostname, folder, author_del, recipient_del, format, thread, type, variables) VALUES (1, 3, 'test', 'test test test', 1203639864, 1, '82.3.162.241', 0, 0, 0, 1, 0, 'private-message', 'a:0:{}') in /var/www/vhosts/therevival.co.uk/httpdocs/includes/common.inc on line 3314.#18
After clicking edit next to a content type on "admin/content/types" I also get the following error message:
warning: Missing argument 3 for privatemsg_form_node_type_form_alter() in /var/www/vhosts/therevival.co.uk/httpdocs/sites/all/modules/privatemsg/privatemsg.module on line 390.#19
Hmmm... Still not sure what's going on with the sending issue, but I think I got the "admin/content/types" issue fixed. I spent some time trying to get various other things working that weren't on my install, which was getting the "move messages to a new folder" to work. This ended up with me re-writing the message list pages and message view pages using drupal_get_form in the menu callback so as to get the $form_state['storage'] values to be passed to a form. This is also meant re-writing the submit callbacks for a bunch of forms, and somehow resulted in the javascript for the folder selection pulldown to get really flakey... AND it breaks the configuration options of "Display action form on view screens:"...
Attached is a patch of my changes against 5.x-2.x-dev so far...
#20
tracked down my problem - the column for id was not set to auto_increment in the Extra field. I changed that through phpMyAdmin and it now works.
EDIT - not used your current patch, but with the past one there is a type on the delete function on the Inbox page - it does not delete and sends you to the priatemsg page which does not exist. (missing "v")
#21
Attached is another patch (against 5.x-2.x-dev). I think it resolves the delete issue...
Other changes are:
Something I haven't looked too closely at is privatemsg_autocomplete. For some reason it was throwing me a bunch of errors and I temporarily set it to use users_autocomplete (which doesn't check for "allow_privatemsg" when pulling data.)
Also, when viewing a message, there's an option for having message action forms on both top and bottom of the message. I had an issue where, when trying to move a message to a folder, the wrong folder would be used for the move since there are two selects for the same thing which can be set to different values...
#22
Attached patch is against 5.x-2.x-dev.
I think single message view is pretty much working (with some javascript oddness.), but the threaded view is not functioning at all. nbz, I finally found what was causing messages not to be saved. In _privatemsg_send(), there is a call to db_last_insert():
<?php$message_id = db_last_insert_id('privatemsg', 'id');
?>
This was to replace a couple of calls to the deprecated sequences tables:
<?php$message_id = db_next_id('{privatemsg}_id');
if (!$thread) {
$thread = db_next_id('{privatemsg}_thread');
}
?>
However, it seems that with a MySql db it returns 0, and I guess this isn't a problem if the mysql table is set to autoincrement, BUT it breaks the thread view since I can't seem to find a way to assign a thread id without the old sequences table. I was thinking that I could just use the $message_id if a thread id had not been assigned yet. But if I can't get the message id prior to inserting the message, then that's not really going to work either. Doing a variable_set for something like "last_privatemsg_thread" seems a little absurd as does creating another table just for thread id sequences...
Anyway, as for this patch, I got a little overzealous and started splitting the code up into include files, so the code shifted quite a bit. (Everything is still in the privatemsg.module file, though)
#23
Are you sure? the call to write the PM does not seem to use message id.
To save a new record using drupal_write_record, you do not (or should not - I am not expert) need to specify an id. It will automatically select a distinct id.
The only reason to use that call is if there are hooks after insert which need the id. In that case, instead of getting the id first, you should get it after message insert (this is that comment_save has started to do in the comment.module).
Not sure if you can do that though... the line for message insert (
return ($result == SAVED_NEW) ? db_last_insert_id('privatemsg', 'id') : FALSE;) is a bit beyond my absolutely basic php understanding.#24
My understanding of db_last_insert_id() (and I am by no means an expert either) is that this how drupal maintains compatibility for postgresql for serial tables. In order to get the next incremented id for a table you call on db_last_insert_id('table', 'column') like how you used to call on db_next_id() to retrieve the next id in a sequence prior to actually inserting a row. I'm assuming that with mysql's autoincrement it just ignores the passed passed value and assigns the next id automatically, whereas, I think postgres requires the id explicitly set.
In any case, I haven't actually looked to see if I can get the id after the insert has taken place, but I'd like to avoid a situation where in order to assign a thread id, you would have to save the message and then immediately do an update on the row. It doesn't seem like the best solution, but then again, maybe it is.
BTW, this code:
return ($result == SAVED_NEW) ? db_last_insert_id('privatemsg', 'id') : FALSE;, is pretty much the same as:<?phpif ($result == SAVED_NEW) {
return db_last_insert_id('privatemsg', 'id');
}
else {
return FALSE;
}
?>
#25
If that is the case, the message_id is not used in the insert operation? (which I assume is the $result = drupal_write_record... does that line actually do the writing? or is it this call that does it?) You can then get it after the write, with the db_last_insert_id, without needing to add the +1.
This is only for the hook to other modules (module_invoke). as for the threading, it seems that the $message_id is not needed for that - My reading of the current code is that the code gets the last message_id, adds one, saves the message and thread information WITHOUT using that $message_id, then calls module_invoke with that $message_id.
Anyway, can the schema set the id field to autoincrement? That would solve the bug on at least Mysql, and to me it sounds logical.
#26
Yeah, my understanding of db_last_insert_id was off. It returns fine, as long as you made an insert before you call it, and you're right, the drupal_write record doesn't actually use it. I was actually looking at the old query that's commented out, because I'm dumb like that sometimes...
A little test run showed that this was working fine for threads:
<?php$message = array(
'author' => $sender->uid,
'recipient' => $recipient->uid,
'subject' => $subject,
'message' => $body,
'timestamp' => time(),
'newmsg' => 1,
'hostname' => getenv('REMOTE_ADDR'),
'format' => $format,
'thread' => $thread,
'type' => $type,
'variables' => serialize($variables),
);
$result = drupal_write_record('privatemsg', $message);
$message_id = db_last_insert_id('privatemsg', 'id');
if (!$thread) {
db_query("UPDATE {privatemsg} SET thread = %d WHERE id = %d", $message_id, $message_id);
}
?>
It gets the $message id of the last insert and updates the record if a thread hasn't been defined, which is the case of new messages vs. replies which carries the message id of the first message in that thread.
#27
I think I've spent all the time I can afford to spend on this before I really have to start hacking it for my own needs. This patch (against 5.x-2.x-dev) splits the privatemsg.module up into a privatemsg.pages.inc file.
#28
Just upgraded to the latest patch. Very good stuff.
Only one problem, on the view message page (privatemsg/view/*), the actions buttons always show above the message. I have tried above, below, both and neither.
#29
A few of other issues - people cannot view sent messages in their account. ("Access denied" - even if the user is in a role with "administer Privatemsg" checked)
And when user 1 looks at other accounts, they are empty.
3. There is no reply button on messages.
#30
The access sent messages problem that was mentioned in the above comment is centered around this code:
<?php$items['privatemsg/list/%'] = array(
'title' => privatemsg_get_inbox_title(),
'page callback' => 'drupal_get_form',
'page arguments' => array('privatemsg_list', 2),
'access callback' => 'privatemsg_access_callback',
'access arguments' => array('create new folder', 2),
'file' => 'privatemsg.pages.inc',
'type' => MENU_DEFAULT_LOCAL_TASK,
'weight' => -10,
);
?>
In other words only those who can create new folders can see their sent massages. changing that to "access private messages" does NOT fix it though... no idea why not.
No idea how to fix the other two problems. (no reply button, action buttons on the top only)
#31
Also, line 831 of privatemsg.pages.inc:
<?phpif ($user->uid == $message->recipient && !$threaded_view) {
?>
should be:
<?phpif ($user->uid == $message->recipient && empty($message->thread)) {
?>
#32
subscribe
#33
Fine! I hope, the port to D6 will be finalized in the next days.
I would like to get this module for my phpbb to drupal migration :-)
Thank you developers!
#34
Looking forward to the D6 release
#35
This needs to be changed:
<?php$items['privatemsg/list/%'] = array(
'title' => privatemsg_get_inbox_title(),
'page callback' => 'drupal_get_form',
'page arguments' => array('privatemsg_list', 2),
'access callback' => 'privatemsg_access_callback',
'access arguments' => array('create new folder', 2),
'file' => 'privatemsg.pages.inc',
'type' => MENU_DEFAULT_LOCAL_TASK,
'weight' => -10,
);
?>
To this:
<?php$items['privatemsg/list/%'] = array(
'title' => privatemsg_get_inbox_title(),
'page callback' => 'drupal_get_form',
'page arguments' => array('privatemsg_list', 2),
'access callback' => 'privatemsg_access_callback',
'access arguments' => array('access', 2),
'file' => 'privatemsg.pages.inc',
'type' => MENU_DEFAULT_LOCAL_TASK,
'weight' => -10,
);
?>
Sorry about the convoluted access callback, I was trying make sense of what the earlier patch had ("privatemsg_special_access1, privatemsg_special_access2, etc...).
That there is only an action bar at the top was due to the forms getting whacky on submission because of identically named form elements set to different values. It's been a while since I worked on this, but I think that I removed them from the bottom since I figured that one working set of action buttons was better than two wonky ones...
Are you using the threaded view by any chance, nbz?
If you are, then I think the "reply" is intentionally left out, as per this code that you noticed:
<?phpif ($user->uid == $message->recipient && !$threaded_view) {
?>
"!$threaded_view" isn't checking for a message thread, it's checking to see if the site is using the threaded view for messages. I think that this was by design since in threaded view, when you view a message, you're automatically sent to a reply form when you view a message...
Of course, I never actually used the 5.x version of this module and was porting it to see if it would be useful for a client's 6.x install, so I'm not sure how it's really supposed to act. Sorry.
#36
Yes, I am using threaded view.
The reason I need the reply button is because threaded_view does not seem to automatically show a reply forum on the message view page, but only on conversations which are threads.
(I guess the bug may be with the reply box, but I could not figure out where it is being set and how to modify that.)
I never used the 5.x-2.x-dev version either! :P (Since there was no stable release from the 2.x code, I doubt everything worked as advertised. Just well enough for it to be an improvement from the simpler 1.8 release.)
#37
Re: Sent Folder
I forgot: There is a bug with Sent folders. Currently the module defines the Sent box as "1", which causes problems when the first new folder is created since its id is also "1". You might have delete whatever folder with the id of "1" from your privatemsg_folder table to get things working...
It's been a while since I've looked at the code for threaded messages, so I don't remember it too well, and it might be a little while before I get another chance to...
#38
Postponed until d5 version is as bug less as possible.
#39
thanks for the wonderful works! I'm waiting this module and og/og_forum to roll out my site.
#40
...
#41
up
#42
litwol: Are you sure this should be postponed? In the guestbook module, we committed an almost-finished D6 module to HEAD. It spawned a lot of extra activity, which led to good patches and bugfixes both for the D5 and D6 branch. The D6 version was released shortly afterwards because of this. Release early, release often!
#43
There is a cryptic message on the project page about this:
step n: Port to drupal 6 (this step will include complete architectural change of privatemsg module. work is already in progress)
I just want to know what the plan is.
A new pm_lite module has popped up which is similar to the vision Litwol had (and according to the forums here, was talked out of), so is that module the future, or will everything continue over here?
(I am using the patches above for Privatemsg on a live site. It has mostly been ok as a stopgap, but I think it is causing some cron issues atm.)
#44
i've been working on privatemsg d6 version for a while now. testing the waters with different architecture and different methods to see what suits this purpose best in the long run. i'll be posting alpha code soon (alpha because it works but doesnt have ALL the feature set that i want for first official release)
it will start off with minimal feature set with strong focus on usability, security, performance optimization, and theming and developer flexibility.
I have spoken to the developer of pm_lite module urging him to combine efforts instead of splitting and confusing the user base of private message module. unfortunately that person did not turn out to be a team player so now we end up with two modules. however i do intend to continue actively developing privatemsg module and continue to raise the bar on security of privatemsg content (i have something special in store for this which i'll implement after the basic privatemsg feature set made official release).
When i put alpha code up i will make a write up along with it explaining the architecture and how it will continue to develop so that eager developers here can contribute patches :-).
#45
Thanks for the heads up. Nice to know there is a fully developed plan.
(I can do simple patches too. change a few words here and there...:P)
#46
The patches havent been working for me anymore, does anyone have the most recent Drupal 6 patch? I would really appreciate it.
#47
I've commited the initial code for the drupal 6 version rewrite.
The module is in pretty usable shape right now but it is of course severily lacks features.
I will continue to add more features and improve the code and i'd love to get some reviews if some one could install the d6 version.
thanks.
#48
Just did a quick test on an almost default install of drupal 6.2 (other modules installed: tribune, markitup, smileys) with the garland theme.
Most of what I noticed you are probably aware of, but just in case:
1. clicking on "private messages" or inbox take you to the same place - full listing of sent/recieved messages, not an inbox.
2. Most of the info on that page is good for development, but should probably be removed before a proper release: id, thread_id, message_id.
3. recipient and timestamp need to be reformatted. recipient as a name. timestamp as a time/date.
4. recipient seems to be whatever user is viewing that message. This should always be the people the message is sent to.
5. There is no reply button on messages.
6. messages are to "you". IMO not a good personalisation as I personally have the username "you" in quite a few places... I say stick to the actual username.
7. I see that the default look is "facebook style". Not really used facebook too much, but the page does not look pretty without any user pictures/avatars. It needs to degrade gracefully. for different sized/non-existant avatars.
8. trying to access other private messages does not give an access denied, but rather an almost blank message to you with a message of n/a, author of anonymous. (the "access all messages" permission does not seem to work yet)
Overall, I am very impressed. With a reply button, this could already be way better than the previous "patch" available further up in this issue. Thanks for all the hard work so far.
#49
And some images to go with the above comment.
#50
The site only seems to allow me to attach one image per message.
#51
On a structural note, is there a reason for the current way of having the author in the pm_message table, but having the recipients in the pm-index table? (that way there would not eb the "incorrect" recipient of the actual author.)
Secondly, sending a message to two users gives both (three including the "author recipient") messages the same thread id. I can see how this could work, but it may get a little complicated. (can a user view the whole thread? or just posts sent by/addresses to them? will only replying to a subset of users in a thread start a new one, or continue in the same one?)
I forgot to mention in the previous critique posts the lack of input format information. I assume the default input format is used?
Additionally, the URL namespacing includes */private/* (messages/private/view/*) is there any particular reason why this is preferred over just messages/view/*?
#52
hey nbz, thank you for your feedback :)
to answer some of your questions: most of the output on the current version is debugging information, you can see in code just how much more i had there before i commented it all out :-p. It will change as i continue developing because i have big plans for privatemsg theming architecture which will allow theming all aspects of a message, unfortunately that is not in place yet.
I am thinking to move away from folders concept and onto tagging (just like gmail does). they are practicaly almost the same but i find tagging more flexible, that is why inbox will show both sent and received but maybe it will make more sense to change the name of that folder. but also i just didnt put in time to write correct query build for the inbox :-p.
anyway there's tons of work left to be done and i will definitely address every point you outlined.
#53
1. In the pm_send function, the old
INSERT INTO ...method is used twice. Should this be replaced with drupal_write_record()?2. There is no api to send a message. There has never been one, but having one would be advantageous to other modules. (I think) I have just made phpbb2privatemsg compatible with the new schema changes. This would be easier if in the future I could just call a send function to take care of everything. (but then again that may not work as importing data is one of the few places where even the timestamp needs to be imported.)
3. Some functions start with "privatemsg_", others with "pm_". Maybe this should be made consistent.
My (untested) commit to phpbb2privatemsg HEAD can be seen here:
http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/phpbb2drupa...
#54
nbz i feel as if i'm wasting your efforts if you build functionality upon unreleased d6 version.
I am still experimenting best designs and architecture for this on the application and database level, so current schemas and otherwise may and most likely will change.
as for consistent API for sending messages, i will definitely provide that with the first release.
unfortunately using drupal_write_record will not be possible because it works only for tables that have defined schema. i have some designs in mind that will have more or less fluid schema. i'll look at cck how they solve that problem (if they do... dont know yet)
#55
No probs - I am not planning to those changes in a stable update any time soon.
#56
subscribing
#57
It seems there was a commit recently that updated the schema to use "mid" as the field name for the message ID, but none of the code was updated to reflect this change. This causes 6.x-1.x-dev to break out of the box so here's a patch to fix this issue...
Patch applies to 6.x-1.x from June 23, the latest dev release code. Only privatemsg.module was modified.
#58
Hi all, i'm interesting too in portin on drupal 6.x
I've dowloaded dev module (6.x-1.x-dev - http://drupal.org/node/268824) but after the installation when i'm trying to send a messagge
user warning: Unknown column 'author' in 'field list' query: INSERT INTO pm_index (mid, thread_id, recipient, author, timestamp) VALUES (1, 1, 3, 1, 1215422280) in /home/mrta/projects/drupal-6.2/sites/all/modules/privatemsg/privatemsg.module on line 543.
Then i've add this:
'`author`' => array('`author`'),
to pm_index in install file.
After reinstalling, it's ok.
#59
http://drupal.org/node/278586 for the database table issue.
(IMO this issue should be marked as fixed - it is turning into an all encompassing master thingy.)
#60
i'm still unable to make this work on my fresh install of drupal. Will there be any more updates on this module or a time completion schedule?
#61
Can I install it on my production site to test?
#62
Current state is for developers only. Not too useful to end users.
I do have a site on which I using a hacked up version of the patch we have in comment 27 - but if you do use that, it will need further modifying (cron breaks things, and there are a few other issues with it that you will have to decide to hack depending on your needs.) and you will have to cross your fingers over an upgrade path.
(Unassigning this from Drunken Monkey as this is no longer about his patch).
#63
Drupal 6 is old and it would be very appreciated with a version for it very soon
#64
subscribe
#65
subscribe.
#66
subscribe
#67
I agree with nbz in #59. As there's nothing here that is relevant to HEAD (which litwol has indicated is the current D6 version) as of this date, I'm going to mark this as fixed.
If you have issues against HEAD -- bug reports, feature requests -- please create a new issue.
temp
Automatically closed -- issue fixed for two weeks with no activity.
temp
Automatically closed -- issue fixed for two weeks with no activity.
#68
Automatically closed -- issue fixed for two weeks with no activity.