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

Nice Jonathan - looking forward to your patches!

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.

Good idea - I will go through them as well

I've updated #232400: Update MultiPing to Drupal 6 - I think we should continue our discussion there if it regards the fix of the Drupal 6 port in general and in #537962: The future maintainership of Multiping if it regards new maintainers for this module.

Does that sound like a good idea to you?

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

Apparently there's a problem with drupal_install_schema() not clearing the cache in drupal_get_schema(). I added a new line to the code, based on your patch, that clears the cache between our call to drupal_install_schema() and our call to drupal_write_record().

Attaching a new patch.

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

I didn't try the patch either and I was too fast to find a fix - i looked at #200931: Schema not available in hook_install/hook_enable and thought it would work in hook_install() but I now see that it only works in hook_enable().

I think the best would be to not use drupal_write_record() then and instead insert the code through our own query as was done before.

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