Hi!

I managed to port the stable branch of Watchdog Live to D7, for a small university course (Drupal Module Development, University of Szeged) project work.

Changed the .info so that D7 would accept the module.

In the .js file:
I changed Drupal.parseJson() to jQuery.parseJSON(). At the end of the file Drupal.jsEnabled don't has to be checked before running the script.

In the .module file:
One page callback must return JSON data, before it was done by calling exit() after printing the data JSON encoded that is a bit hacky if I'm correct. I tried to use the new delivery callback feature for this, but failed. At least I made it a bit nicer hacky (see patch).
In the same page callback, previously preg_replace() were used to remove unnecessary parts of data. This is not needed, as the data is in associative array form, so the unnecessary parts can be removed by setting given keys to empty string.
Some values were stored with variable_set and in _SESSION['xyz'], changed them to be stored in cache tables per user (I see no point in storing the disabled state only for session).
At the end added some functions for using the cache tables.

And for the cache tables, an .install file were needed with definition of hook_schema().

I attached all these packed together in a .zip file. Patch should be applied in the directory of the module.

My port were tested by me locally, and by Yorirou.

Hope it will be accepted!

Cheers,
Temaruk

CommentFileSizeAuthor
#9 Issue-1032712-by-Temaruk-Port-to-Drupal-7.patch16.48 KBtemaruk
#4 watchdog_live-port_to_drupal7-1032712-4.patch68.88 KBtemaruk
#1 watchdog_live_port_to_D7_patch.zip3.15 KBtemaruk

Comments

temaruk’s picture

temaruk commented
StatusFileSize
new watchdog_live_port_to_D7_patch.zip3.15 KB

The attachment...

naxoc’s picture

naxoc commented
Status: Needs review » Needs work

Thanks for porting the module!

It looks good, I only have a few things about the patch:

The disable chcekbox after a clean install will throw PHP notices:
Notice: Trying to get property of non-object in watchdog_live_get_disabled() (line 146 of /Users/ckj/Sites/heads/head7/sites/all/modules/watchdog_live/watchdog_live.module).

The cache tables are a bit overkill in my opinion. This module will typically be used by a few administrators, and hitting the cache for getting a enabled/disabled variable should not be necessary. Modules that need to create two tables for keeping track of something that simple tend to put people off installing them. I would be more comfortable sticking with the sessions.

The .info file contains the d.o packaging data still, so the patch does not apply on files checked out from CVS. That needs to be removed.
Please reroll it into one patch - that is so much easier to apply and review.

I know that the javascript in the module has not been maintained too much by me, but we should use the jQuery behavior handling in the D7 style. More about it here: http://drupal.org/node/756722

Coding style looks good. There are a couple of small things like elseif not beginning on a new line and true/false not being uppercase for PHP constants. Comments need to be ended with a period (.)

Again, thanks for taking this on :)

temaruk’s picture

temaruk commented

Thank you for taking time to review it! It definitely needs much more work! I will correct the mistakes that you pointed out, and will try to improve the javascript part to follow the D7 style. I will take a look at the AJAX framework too. To be continued! :)

temaruk’s picture

temaruk commented
StatusFileSize
new watchdog_live-port_to_drupal7-1032712-4.patch68.88 KB

Hi!

I got round to finalize the patch (attached).

- Switched back to storing settings in session. (though I kept the working cachetable version too)
- Corrected that when the log overview table was reloaded, the 'Clear logs' fieldset broke.
- Corrected the PHP notice issue that naxoc had on a clean install. (#1032712-2: Port to D7)
- Modified the JS to incorporate behavior handling in D7 style.
- Added AJAX tooltips to the events using the Beauty Tips jQuery plugin. (uses the hoverIntent jQuery plugin - content is fetched only if you hover for several ms on the element with tooltip, this way unnecessary calls could be avoided)
- Added weighting to the settings fieldset so that it appears above the 'Filter/Clear logs' fieldset, and made it collapsible (purely aesthetic "issue").

I think that is all.

Cheers

naxoc’s picture

naxoc commented

I'm sorry I didn't get around to looking at this until after the git migration. I I still cannot apply the patch - please remove the d.o packaging data that the .info file contains.

The using the BeautyTips plugin is a great idea, but we cannot take code that lives elsewhere and commit to this module, so if you make watchdog_live depend on http://drupal.org/project/beautytips we can use it from there.

Thanks! :)

temaruk’s picture

temaruk commented

Thank you for your time! The git migration was much more important issue than this patch.

Weird happenings. I did remove the packaging data before I made the patch as you advised it before. I used a local git repository for version control, and made the patch according to this guide: http://drupal.org/node/707484

I downloaded the patch and succesfully applied it locally. I did it like this:
Changed present working directory to 'watchdog_live' (the module directory), put the patch file there, and 'patch -p0 < name.patch'.

The patch is against the stable version.

"...but we cannot take code that lives elsewhere and commit to this module..."

So, including the BeautyTips jQuery plugin (2 js and 1 css files) in the patch is bad method, right? Wouldn't it be overkill to make it dependant on an other module? Is it possible to make it dependant so that it works without the other module, just with reduced functionality?

Cheers!

temaruk’s picture

temaruk commented

Ah, finally I understand the problem with adding the jQuery plugin this way, my bad! :-(

naxoc’s picture

naxoc commented

I totally agree on the dependency to BeautyTips being overkill. Is is maybe possible to make it a soft dependency like you suggest? That way people can choose if they want the functionality themselves.

The way you roll the patch against stable is fine as long as you remove the packaging stuff from the .info file :)

temaruk’s picture

temaruk commented
Status: Needs work » Needs review
StatusFileSize
new Issue-1032712-by-Temaruk-Port-to-Drupal-7.patch16.48 KB

Here is an updated version!

So, the tooltip feature is optional now. If someone manually downloads and places the needed jQuery plugin files OR installs the BeautyTips module (which has no stable D7 version yet, but I tested it with the dev version and it worked for me) it is automatically enabled.

Hope it is OK!

naxoc’s picture

naxoc commented
Status: Needs review » Fixed

Thank you very much for your time (and patience) temaruk! I commited the port to the 7.x-1.x branch - a dev release will come out soon :)

The soft dependency on Beautytips works out really well, so the module just got a lot more useful!

Status: Fixed » Closed (fixed)

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

temaruk’s picture

temaruk commented

Oh wow, I missed the happenings! I'm glad that I could help! :)