Code improvement patch

Hi,
I will try to review your changes. I have also made a whole load of improvements for my site, but not given any patches because the module board seemed very quiet. I expect we have done alot of the same work, so that is my fault for not getting patches up here.

It would be good to get a fully stable tested official D6 release. I'm sure we can do it.

Jonathan

There were 16 open issues, but I have just closed some of the really old (over 1 year) ones which did not have anything other than the opening post, and which would appear to be out of date now anyway.

When we can get this board down to a level with only current issues and patches, then it looks much more like a current active module. We then know what we are dealing with and new visitors will be able to contribute to the latest developments.

Fine

Good work, voxpelli. You have made lots of improvements for standards and readability.

I checked the pacthed files against coder and found a couple of 'else if' which I have changed to 'elseif' (line 33 and 200 in .module). Then I checked with /scripts/code-style.pl because I have found that this detects some things that coder does not yet do (see #484566: Operator spacing), and found two more things to fix, which were adding spaces around == in line 133 and adding the required braces around the IF statement on line 194.

For the .install file I added the $Id$ line needed for cvs, an @file description and header blocks for each of the functions. Attached is patch which includes all your work plus these few new changes.

I tried a fresh install and the multiping table was re-created ok but the initial row for pingomatic did not get inserted. I tested the code separately after install and both the creation of the $pingomatic object and the drupal_write_record worked fine. So I debugged during another installation run and it appears that the multiping schema is not installed at that point, so $schema = drupal_get_schema($table) returns nothing, hence drupal_write_record returns without doing anything. Is there a way to force the results of drupal_install_schema() after calling it in hook_install such that the schema can be used later in hook_install? That may be a question for the support forum. If there is not an easy way then we may need to go back to doing a plainer slq query instead of drupal_write_record.

Jonathan

Looks good. Before seeing your post I spent an hour on the train today following through the logic of drupal_get_schema, and I also tried an additional call to drupal_get_schema('multiping', TRUE) in an attempt to rebuild the static variable $schema, but this still did not work. It calls module_implements('schema') but this does not return multiping at this point. I then tried an additional call to module_implements('install',true,true) which is supposed to refresh the list. Still no multiping is returned. Is this becaue the module is not enabled yet, ie status is not 1? That's as far as I got, so not flushed out the ultimate cause yet.

Maybe I was doing something wrong and your change with drupal_get_schema(NULL, TRUE) will work. I've not got time to test this at the moment because I am off on holiday tomorrow morning, but I will get on to it the following week, if no one else has tested and passed it.

Jonathan

Thanks for the link to that post, it was useful. After making my comment in #10 I was wondering about hook_enable. I think that doing a forced schema reload in multiping_enable is probably the best idea, as this allows us to use drupal_write_record, and maintains the database abstraction layer which is important if Multiping is going to become a fully compliant module.

Before inserting the default record I have checked that the table is empty, ie if the admin disabled the module but did not uninstall it before re-enabling then we dont want to add the record again.

I have made these changes and created a new patch based on everything we've done so far from the existing dev release.

Jonathan

Hey guys - glad to see people started working on a d6 update. Any recent progress? Happy to QA test if you have anything worth testing?

Hello John,
Thanks for your interest. Unfortunately it has gone a bit cold because there's been no activity on #537962: The future maintainership of Multiping for a while. I am not in a position to take on making the actual commits, but I do have an interest in cleaning and updating the module, then providing enhancements, via patches.

Have you tested out the patch in #12 above?

Jonathan

Hi Voxpelli,
Thats great news, getting maintainership.

It would be good to have at least one other person test our patch in #12 before you apply it.

Jonathan

The patch in #12 is working fine to me, pingomatic is pinged, which didn't work before the patch. But I still have problems to ping Twingly, I've tried both weblogUpdates.ping and weblogUpdates.extendedPing, with and without RSS. The watchdog says Failed to notify Twingly.

Thanks for testing. Regarding Twingly, I think you should start a new issue, as this thread is purely for the code-improvement patch, which brings the existing code up to standards. We did not introduce any functionality changes here.

Jonathan

Fantastic! this is great. We now have an active module again!
I'm busy on another drupal site at the moment, which does not have any use for multiping, but soon I will have time to get back and work on the site which does use it. I'll test it, but I'm sure everything will be exactly as we expected.

Thanks for laying out the roadmap. I'll definitely be contributing and helping you.

Jonathan.