GHOP #93: Port Event module to Drupal 6

This is now a GHOP issue: http://code.google.com/p/google-highly-open-participation-drupal/issues/...

The work so far, but there are many strange errors.

Here's a patch containing Placinta's code and my own work... the module seems to work properly.
There's an issue in the event_page() function: on lines 391 and 393, a variable $node is used - however, this variable never has any value. Apart from that, no php notices should appear.
I struggled with the node preview for a while, but in the end I managed to keep the values instead of falling back to the defaults. However, I'm not really sure if my event_preview() function is correct and if I need all the stuff in event_nodeapi(), so I hope that someone will have a look at that.

Looks like a lot of work went into the patch.

quick feedback - these lines in the install file look odd:

+  db_query("ALTER TABLE {event_timezones} MODIFY offset time NOT NULL default '0'");
+  db_query("ALTER TABLE {event_timezones} MODIFY offset_dst time NOT NULL default '0'");

Is this supposed to update pre-existing tables? Why isn't this part of the schema definition?

Also, in hook_nodeapi, maybe you should be looking for this flag during the preview:

$node->build_mode == NODE_BUILD_PREVIEW;

Thanks for looking into this! I attached a new patch which includes the two things you mentioned.
I just spotted a php notice appearing after login if you didn't select a timezone in your user profile. This isn't really related to the Drupal 6 conversion, so I didn't do anything to correct this.

The 2 code lines

+  db_query("ALTER TABLE {event_timezones} MODIFY offset time NOT NULL default '0'");
+  db_query("ALTER TABLE {event_timezones} MODIFY offset_dst time NOT NULL default '0'");

were added by me, for the time being, because the schema doesn't support time fields, only datetime.

looks like at least MySQL and PostgreSQL support the time type - so you could propose a core patch?

However, seems like many other DBs that might be supported in the future (Oracle, MSSQL, DB2) do not support a time type. So, perhaps you should consider using the datetime type and pulling off just the time part in the PHP code?

Either way, it seems a little problematic as it is.

As another possibility, the queries could be re-written using EXTRACT:

http://www.postgresql.org/docs/8.2/interactive/functions-datetime.html#F...
http://dev.mysql.com/doc/refman/5.0/en/date-and-time-functions.html#func...
http://download.oracle.com/docs/cd/B28359_01/olap.111/b28126/dml_functio...

something like:

SELECT EXTRACT(HOUR FROM offset) as offset_hour EXTRACT(MINUTE FROM offset) as offset_minute

The patch appears to be clean from a style point of view. I highly recommend using the coder module for checking style and for helping to port modules to Drupal 6. It's extremely helpful.

There is this bit of code in the patch:

+        echo 'test arg';
+        if ($node == menu_get_object()) {
+          echo 'pass';
+        }

That looks like maybe it's left over debug code that needs to be cleaned out?

I ran into some problems while testing. I created an event for today, with a start and end time. Then I went to the calendar view.

The first problem I noticed was that I could only click "arrows" to go to the next day, week or month, but not the previous. If I did go to the next day, week or month, then I only had an arrow to return to the previous (where my event was listed) day, week or month. I could not continue into the future.

The second problem occurred when I tried to filter the calendar view for even types of "Event". It resulted in this PHP error output:

    * warning: date_parse() expects parameter 1 to be string, array given in /home/chris/www/d6/sites/all/modules/event/event.module on line 2790.
    * warning: date_parse() expects parameter 1 to be string, array given in /home/chris/www/d6/sites/all/modules/event/event.module on line 2790.
    * warning: mysqli_real_escape_string() expects parameter 2 to be string, array given in /home/chris/www/d6/includes/database.mysqli.inc on line 329.
    * warning: htmlspecialchars() expects parameter 1 to be string, object given in /home/chris/www/d6/includes/bootstrap.inc on line 670.

This error did not re-occur once I added a second event.

I did not do extensive testing, but the things I checked all worked, e.g. the RSS feed, the iCal link, comments, etc.

When choosing to uncheck the "all day event" box, the event is still listed as having a starting time. This might be the same way it behaves in Drupal 5, so would not be a problem with the patch. It would be nice to have that make sense, and not list a Start date/time for all days events, but just list "Date: date-value".

Also, although the patch itself is clean regarding Drupal style conventions, the existing module has a bunch of minor spacing issues pointed out by the coder module.

I corrected the issues the coder module mentioned and removed the debug code, of course.
When I had a look at the arrows issue, I noticed that the Drupal 5 version of the module has the same problems, so I didn't fix this.
Concerning the filter problem: I couldn't reproduce the error output with a fresh Drupal installation and one created event node; indeed I get some php notices, but these are because of the inexistent $node variable I mentioned in #4. Can you give a more detailed description of the steps to reproduce the error?

I installed the new patch and gave it some testing.

I tried hard to reproduce the PHP error problem I reported before, but was only able to do so once after extensive clicking on various calendar options, and so cannot describe exactly the path I took. I tried to reproduce it again and was unable to do so. It appears to happen at the same time the Filter setting is preserved across clicks; normally it gets reset to All whenever someone clicks on month, day, week, table, list, etc. I would guess the problem I am seeing is also in the Drupal 5 version, given how obscure and hard to reproduce it is.

2 pwolanin: There is already an issue regarding the time type.
http://drupal.org/node/200953
But it seems that the developers voted on not supporting the 'time' type.

@Placinta - well, the choice of how to proceed seems to be up to killes and the other maintainers.

For the record, I spoke with killes via IRC and he said he was happy with the patch, so I'm marking this GHOP task as closed closed so that flor...vtipp.at gets credit for the task and can move on to take another task. Great job!

I've applied the patch and created a snapshot release at http://drupal.org/node/206919.

Thanks for the help Placinta and fwalch!

I dont know if this thing was meant to be stable or not. But it killed my drupal (6.x) site after running for a while. I got http://drupal.org/node/283656 error after a while for no apparent reason. So use this thing on your own risk. I dont know why Drupal core cant have a decent stable Forum and Event module built in. After all thats what all community portal need and druapl was supposed to be community oriented CMS.

There is no stable version of the event module for Drupal 6 - there's only the development snapshot on the event module project page. You shouldn't use this on a production site, though.