GSA for drupal 6 - first release

Hmmm, the file doesn't seem to be attached.

I have updated the themed output and added a Google Search block which searches purely on the google appliance. Themed output is similar to drupal search output so the look and feel is the same.

I have added a permissions hook so this module is now independent of the search module

I'm excited to get my google mini hooked up with Drupal 6.2. Do you have an ETA on this module? Thanks for all your hard work!!!

I have run richardcanoe's module through Coder for D6 and made all the adaptations that Coder indicated to google_appliance.module.

I haven't yet been able to test on a Google Search Appliance, but could really do with this for an imminent project that I'd look to run on D6 rather than D5. Happy to participate in making this work for D6. May need to make some extensions to it for my particular uses (E.g. the GSA is needing a feed being posted to it with search data - when I last looked in the 5 module this wasn't incorporated, but things may have changed in the last couple of months).

That's for all the work guys! I don't have any large D6 projects at the moment, and certainly none with the GSA, so it will be awhile before I deal with this. If someone is interested in taking it forward once an alpha version is ready, please step forward so I can make you a maintainer of that branch!

Best,
Jacob

Another interim d6 version. Full version rather than patch (since there is as yet no d6 branch to patch against)

- Added basic paging to search results
- Changed url in accordance with #308735: Wrong URL structure
- Some CamelCase naming removed, more still to do especially in GoogleMini.php
- Caching fixed
- Set filter=0 to return all results for now

I've added a new module and a bunch of new guff to it to add the capacity to POST a sitemap.xml file to a GSA on cron or on update. It's basically a reworking of the XML Sitemap module with the submit actually posting the data file off to the GSA (as per http://code.google.com/apis/searchappliance/documentation/50/feedsguide....) rather than just telling the search engine that there's a sitemap file to come and read.

Posted here for peer review. Doesn't incorporate richardcanoe's latest changes.

Hey guys, this is really good stuff.

I'm not very involved now, but if we get some people reviewing and saying this is good, I'll definitely take a look. Also, if anyone is committed to this right now and wants to be a maintainer, I'd be happy to entertain requests.

Best,
Jacob

One question - can anyone suggest what to do? I'm struggling to get fully to grips with the finery of D6 theming from the module layer.

In order to construct the sitemap.xml file to submit to the GSA, I need to constuct full pages for each node, but preferably without the $variables['scripts'] and $variables['style'] data included.

So, I really want to do one of two things

1. unset those $variables only when constructing the XML data (i.e. on cron and/or on node submit, depending on what the user has chosen)
   OR
2. have a page.tpl.php file in the google_appliance_node module directory which doesn't print those variables, but make sure that is only used when constructing the XML data (i.e. on cron and/or on node submit, depending on what the user has chosen).

But I'm just going round in circles.

I thought I could get there with an implementation of HOOK_preprocess_page in the google_appliance_node module, and so getting the system only to use the template when the conditions are right. But the $variables available in this function on e.g. a cron run don't seem to be enough to set the right conditions for unsetting the right variables.

I hope that's a good enough description.

Please help!

Hi everyone

I'm going to be integrating Google Mini and Drupal, so I'll be doing a fair amount of work on this module. I would be happy to volunteer to maintain the Drupal 6 branch! I'm not too fussed though, as long as there's an active maintainer to commit patches and fixes that's all I need.

Just looking through the code posted in #5 by richardcanoe. I can't get it to work. I see in the implementation of hook_search that google_appliance is returning a themed HTML set of results. If I print these results right before the function returns, I can see that it's got the right data. But why return the HTML results when hook_search needs a keyed array returned?

http://api.drupal.org/api/function/hook_search

I would like to extend the module a bit. Probably just scatter a few hooks and preprocess calls around. For example, we use the Google Appliance to search within documents. Our site is configured so that a "document" node type has exactly one attached document. If the Google Appliance returns a file in it's results, I want to hook into the process so that clicking the result takes you to the document node rather than the document file.

Anyway looking forward to hearing back.

Cheers
Mark

Attached patch is #5 copied into checkout of google_appliance DRUPAL-5 branch and cvs diffed. Since HEAD is out of date I'm not sure if the maintainer wants a patch against HEAD or DRUPAL-5, but just a zip file can be harder to deal with.

I'm not in favor of the menu item google_appliance_feeder and its callback. While it only selects published nodes it's a security hole by not adhering to node_access. I'm not even clear what this callback is for, it was removed from the DRUPAL-5 branch in #251309: Page not found on path google_appliance_feeder and should not be put back in IMHO. The GSA should be fed by other means, like #6. I've left the menu item in this patch however for further discussion.

I'm not sure if there are any active maintainers for this module anymore. Is it abandoned?

I have more or less abandonded this module in favor of The Apache Solr Module . If you would like to maintain it, please let me know.

Best,
Jacob

Hi Jacob, yes please add me as a maintainer! I am happy to work on the Drupal 6 branch, but I don't have the time to support version 5. Is that ok?

@Mark Theunissen I have a client that is having their site upgraded to 6 this week so much like you I need this module working in 6. I'm happy and available to help this week if you'd like to discuss feel free to contact me through my user page or find me on #drupal. Google Appliance search doesn't return results for me with the patch from #10 so I was considering re-porting from the DRUPAL-5 branch.

@coltrane: Great, sounds good, but I think your timeframe is tighter than mine. I won't be working on it this week.

I do know that the patch in #10, which is based on #5, doesn't work, but it's actually a simple problem... from my above comment:

"I see in the implementation of hook_search() that google_appliance is returning a themed HTML set of results. If I print these results right before the function returns, I can see that it's got the right data. But why return the HTML results when hook_search() needs a keyed array returned?"

Basically the guidelines for hook_search weren't followed and it's returning the wrong thing. Maybe this means that there are other hidden bugs too? I dunno.

#5, #10 doesn't contain branch commits like http://cvs.drupal.org/viewvc.py/drupal/contributions/modules/google_appl... so I'm going to reroll. @Mark Theunissen I see the problem you point out about the return value of hook_search(), it should be returning an array of results.

@Mark Theunissen, when you have commit access do you want to branch for 6 off HEAD? I can roll a patch against HEAD to sync DRUPAL-5 changes if you like.

Should this module be adding meta tags on node pages? I'm not familiar if its required by the Google Appliance but it seems out-of-scope for this module and could be better handled by the nodewords module.

Rerolled patch off DRUPAL-5 for Drupal 6.

1) Left menu item as google_appliance rather than #308735: Wrong URL structure. I'm in favor of changing the URL structure but for legacy users I think we need a redirect.
2) Left permissions in place rather than creating new ones per #5. I'm in favor of new permissions but think it should be in separate patch.
3) Using system_settings_form() in google_appliance_admin_settings() means we don't need to do our own variable_set()s. Commented out google_appliance_admin_settings_submit() for further testing.
4) Removed metatag functions _google_appliance_add_meta_tags() and theme_add_google_appliance_meta_tags(). I hadn't previously read Google's documentation but I can see how meta tags are needed. Can we leverage nodewords rather than write our own meta tags implementation or is the two functions I removed enough and should be put back in?
5) Many coding style changes, such as changing false to FALSE.
6) Kept google appliance feeder callback out. Again, I'm not too familiar with what Google needs but can the Appliance be fed another way?
7) I haven't touched the simpletest code.

search/google_appliance works correctly, but leaving CNW for further testing.

What's the point of the menu item with path 'google_appliance'. It's callback is google_appliance_search but just returns Array. Same result in 5.x release as well.

The schema missed the new serialized column on cache_google. Patch updated to include it. I see the cache table being populated but I don't see it being pulled from, has it ever?

Done. You have CVS access. Good luck!

Thanks everyone!

@coltrane:

1) I agree adding metatags is out of scope.
2) I agree that appliance_feeder is a disaster, bypasses proper access control! Let's get rid of it.
3) Would prefer to make a DRUPAL-6 branch than using HEAD... just what I'm used to I guess.
4) Good to use system_settings_form() properly
5) No idea what the point of 'google_appliance' menu item is. But we do know that it's wrong, because it's calling a Drupal hook directly. ;) Get rid of it!
6) I don't know about the cache. I trust you to do the right thing for now until I take a closer look.

Lots of changes! Can I commit #20?

@Mark Theunissen I'll roll against HEAD, expect it soon. Congrats on CVS access!

Cleaned up #20 patch and rolled against HEAD

Edit: this patch includes changes to DRUPAL-5 that didn't make it the HEAD branch, so it catches up and ports to 6

Done the following:

1) Created a DRUPAL-6--2 branch. Hopefully I (we) will be making enough improvements / changes to warrant a version bump to 2!
2) Patch applied and committed. Haven't tested. Will get around to that shortly!
3) Added a CHANGELOG.txt
4) Added a -dev release to the project.

Thanks again! :)

Is it possible to download the first dev release instead of applying the patches manually on the 5.x version?
Because I don't see direct links...

EDIT: done changing the download url for 5.x version with 6.x-2.x-dev
;)

Damn... I didn't notice there had been activity here since #8, or I would have posted earlier!!

I've done some work on this module recently. I started with richardcanoe's version from #5. The only file I've touched is google_appliance.module, apart from the debug variable name in DrupalGoogleMini.php.

I commented and re-formatted the existing module code (a few instances of incorrect indentation, etc) to start with.

I gather the original module worked with search.module, and it had been changed to exclusively work without it, which seemed a bit of a shame, so the main thing I did was to make the module happy in either instance. It cleanly enables the use of theme('search_results') whether or not the search module is enabled (so the custom pager is no longer needed, and users can do everything through standard theme over-rides). I think I've cleaned up a few other things along the way, but nothing too special.

I'm attaching what I think was a fairly functional version of the above. My dev version is currently broken while I implement some other changes (I need to be able to specify multiple collections, rather than just the one set in the admin page), and it might be a wee while before I get around to finishing that, but hopefully this is of some interest in the meantime.

edit: Working through the merge of the #5 version and mine, I have done the following:

* Formatting and comments
* Theme work and code fixes so the module works with or without search.module
* Configure the global pager variables, so that the standard pager can be used.
* Configurable results-per-page
* Changed the path. I'm likely to change this again when I make more changes. I figure sites will add whichever URL Alias they want for the base search URL, though.
* Fixed some issues with the admin form submission.
* hook_init() to trigger the meta tag additions.
* A change to the taxonomy meta tag naming. Unfortunately I put a colon in, but now see that google uses a name:value syntax, so this was a bad change.
* Made the custom search form/block more like the search.module
* Renamed google_debug to google_appliance_debug

p.s. I've included google_appliance.module.original in the archive. This was the starting point from #5, if you wanted to compare the code.

edit: I'll post a new version in the next day or so.

There is now a DRUPAL-6--2 branch so you may be better off rolling patches for anything new you've done from that.

Hi jweowu, looks like you're doing some good work! As coltrane says, let's pool our efforts together to make this module great. See if you can patch your changes into the 6--2 branch, let's make issues for the new features and discuss!

I've merged my development version of google_appliance.module with 6.x-2.x-dev // $Id: google_appliance.module,v 1.1.4.1 2009/02/26 13:15:43 marktheunissen Exp $

I'll hopefully be able to do some testing on it tomorrow, and will post a patch here if it looks good.

I've been pulled onto something else for a few days, and I won't be able to do anything with this until next week. Sorry; I'll get back to it as soon as I'm able.

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

Here's a new version, based on #27 above, but also merged with 6.x-2.x-dev, and courtesy of Signify Ltd (my employer), and our client.

Patches against 6.x-2.x-dev are provided in patches.tgz

Since the improvements detailed in #27, I have done work on the following items (either implementing, or improving, although a few changes were also replicated in 6.x-2.x-dev, I noticed :)

* Facilitates other GSA search options (e.g. meta-tag constraints) -- see example.txt (attached)
* Multiple tabs for specified Front End and Collection combinations
* Arbitrary Front End and Collection combinations
* Works with or without the core search.module
* Standard Drupal pager
* Standard or custom search-result(s) theme templates
* Available fields for search result themeing
* Attributes and Meta Tags for search result themeing
* MIME type display
* KeyMatches (recommended links)
* Synonyms (alternate search terms)
* Caching of results to increase response time and decrease load on GSA

google_appliance.module itself is significantly different to the 6.x-2.x-dev version (the patch is bigger than the module), so I apologise for not providing features in a more piece-meal fashion. I think the modifications are basically all good, so hopefully you agree.

I'll try to follow up any queries about the changes in my spare time, but as my paid work on this module is essentially now complete, I can't promise to be too responsive.

cheers,
-Phil

I really do like the sound of these new features. The problem is that there are now 3 versions of this module. One from coltrane, one from jweowu and one from Signify.

We agreed back in #25 to commit coltrane's version and work from there. It doesn't help anyone that there are now two complete rewrites without the changes being discussed and patches being provided one-by-one.

Signify, even if your module is the superior version, there's no way I can accept it without disrupting everyone else who may be already using the module. Unfortunately, you've gone about this the unDrupal and anti-collaboration way.

The only way I could hope to accept this, is if I took the time to go through the patches and try establish where and why all your changes were made, and how they disrupt others. I don't have the time to do that, especially when it should be up to you as the patch provider.

So could you please post patches with specific features? Unless of course your client is happy that their module will now fall out of sync with the rest of the community.

I don't want to sound ungrateful, because really I do appreciate your work. The problem is in the delivery. You have to understand my position as a maintainer, it's not helpful at all to me if I have to spend twice as long dissecting all the changes, as you did actually making them! ;)

Please consider #33 a complete replacement for the work done in #27 :)

> The problem is in the delivery.

I genuinely appreciate that, and I do apologise. I was keen to proceed earlier, but unfortunately this was out of my hands. I did set about merging and submitting these patches as soon as I had approval to do so, but until then I simply needed to build something that suited our client's requirements.

> Unless of course your client is happy that their module will now fall out of sync with the rest of the community.

I'm sad to say that there is some definite irony in that prospect. (sigh).

If I can follow this up further and provide smaller patches, I will do so. This isn't my top priority right now, though, so I can't promise anything (once again, my apologies).

jweowu> thanks, that makes things easier!

Signify> No problem, don't apologise!

I have two big GSA deployments coming up in the next few months.... I'll have a detailed look then. If anybody else tests the new version of the module, please report your findings! Thanks.

where are we on this?

Have the various changes proposed here been rolled into 6.x-2.x-dev or are there updates forthcoming? What is left to be done before we can have an official release? i have worked with the google appliance, not with the mini. if its a matter of testing out some features i can assist.

-J

it looks like features have been ripped out, like i dont even see a menu hook for the google search results page or a block for the google search - is 6 dev even functional?

6-dev is working, it uses hook_search.

If you would like to help, please take a look at comment #33 where another version of this module has been posted. It is a massive rewrite. In order to get these changes into the branch, they need to be provided one-by-one as patches so that the community of people who use Google Appliance can be involved in the process and make sure that changes are sensible and don't break their implementations.

Thanks! :)

Is anyone else having issues getting search results back? I can see that my queries are making it to my Google Mini based on the reports I am running, but I am not seeing the results actually coming back to me. I am thinking its some sort of formatting error because I do know its trying to do it, but nothing is being displayed.

Brandon, please make a new issue for your support request. Thanks!

So take the #33 updates and break those up as patches for head? Am I correct there?

Pretty much, although there's a DRUPAL-6--2 branch (rather than HEAD).

does the dev version support meta tag filtering?

so if this uses hook_search() then it would require the search module to be enabled, correct?

Hi everyone,

I'll be working the next couple of weeks on Google Mini. It would be great to have the best version feature-wise as well as community support-wise available here. My view is we'll need to get jweowu’s and Signify’s good feature additions into the current dev version.

Here is what I did. I rolled a single huge patch against 6.x-2.x-dev. I used Signify’s version from #33 as a starting point. I did a little bit of cleanup and updated the google_appliance.install from 6.x-2.x-dev rather then using Signify’s. This should allow for a smooth upgrade.

Here are the major changes the patch introduces.
- Cache table name changes from cache_google to cache_google_appliance
- Pager gets replaced by a custom pager
- Debug changes. No more logging to error_log, only to syslog using watchdog.
- theme_google_appliance_search_result_array() now takes an extra parameter, $total_results
- removed functions: google_appliance_blocksettings_get(), google_appliance_blocksettings_set(), google_appliance_block_nogoogle()

The features from #33 are all there but I haven’t tested them. The patch doesn’t seem to break anything obvious though it would be great if someone who actually uses the current dev version in a project can find some time to test it.

Cheers,
Lars

jaykali: The version from #33 works with or without the search module.

It facilitates meta tag filtering (I used it myself for my application), and indeed any arbitrary QueryParts, but you have to write your own code to make it happen. Usage will (naturally) depend upon how you generated your meta tags, but in short you want to build an $options array to pass to google_appliance_search() in order to generate the QueryParts you need.

(larskleiner's version in #49 will have retained all of this functionality, by the sounds of it.)

The 'partialfields' and 'requiredfields' QueryParts enable you to match on meta tags. I recommend reading the GSA documentation regarding these options, as there are some pitfalls (e.g. limitations with AND/OR combinations, and the maximum limits on number of meta tags, and the amount of text per tag, that can be indexed -- 64 tags and 160 characters per tag, for the Mini I used).

See example.txt attachment in #33 for some example code. That was a submission handler for a form which provided taxonomy terms in a select list. The terms appear in the pages as meta tags, and I convert the values into an $options['requiredfields'] entry when searching.

I ended up rewriting my own code to use 'partialfields' so that I could put many values into a single meta tag, as I ran into the indexing limits. I used a double-hyphen marker to delimit term boundaries, as the GSA treats hyphens as word characters (but practically nothing else other than the alpha-numeric chars).

Ack.

Sorry Lars, but the patch you've posted is based on #27, not #33!

I think you must have downloaded both, and inadvertently used the wrong one.

How about this one?

I consolidated Phil's (Signify's) changes from #33 into the 6.x-2.x-dev version. There is a host of new features introduced in #27 and #33 that however don't seem to break anything obvious. Again, it would be great if someone can have a more thorough go at testing.

Changes from 6.x-2.x-dev include:
- variable google_debug renamed to google_appliance_debug
- table cache_google renamed to cache_google_appliance
- field google_appliance_client renamed to google_appliance_default_client, field google_appliance_collection renamed to google_appliance_default_collection
- google_appliance_search() now takes additional optional params $options, $collection and $client
- functions google_appliance_blocksettings_get() and google_appliance_blocksettings_set() removed

Hi Lars,

I'm pretty confused by your patch, in all honesty. At least as far as google_appliance.module is concerned (I've not looked at the other changes).

I can only see one fix compared to #33: removing a surplus break from google_appliance_block().

You've then made a large number of formatting changes, most of which makes the code less readable to me. I find that horizontal alignment of multi-line array items improves readability tremendously, so the fact that you have gone to the effort of undoing all of that confuses me no end.

That opening brace in _google_appliance_search() was dropped to the next line on purpose for readability, due to the multi-line condition above. (I'm not especially fussed about that, but just so you know.)

You have erroneously removed the 'google_appliance_search_form' entry from hook_theme(). That was necessary to facilitate an 'over-ride' function in the theme.

In google_appliance_admin_settings() you've unnecessarily changed instances of $google_appliance_name, $default_client, and $default_collection into calls to variable_get(). Those variables were sourced from exactly the same variable_get() calls in _google_appliance_get_settings().

You've reverted a change to the parameters of google_appliance_block_save(). This doesn't actually have any effect, however the version in #33 was the correct version for Drupal 6 forms.

You've switched string concatenation to the Drupal 7 format. I'm actually happy about the D7 change (this is the one D6 formatting standard that I purposefully ignore when writing non-contrib code), however the coder module is going to do a whole heap of complaining about that. I find it's worth adhering to this for D6 contrib code, in order to avoid having to filter those warnings out of the coder report, because you'll get so many of them.

Finally, you've bizarrely deleted the "Implementation of hook_simpletest()." comment from that function.

I had already merged my code against 6.x-2.x-dev for #33, so I think that your changes for the .module file compared to #33 could be reduced down to that surplus 'break' removal?

In conclusion... there might be some confusion here.

I already provided monolithic patches from 6.x-2.x-dev to #33 in the attachments for that comment. There's not much point in providing another version of the same thing (although I hadn't thought to provide all my patches in a single file). I would suggest that if you wish to build upon #33, then you make your changes directly to that version.

Mark is actually keen to get the #33 version broken down into multiple feature-specific patches against 6.x-2.x-dev. I'm definitely not going to have time to do that, however. I don't think it would be very practical, to be honest (although if someone has a lot of time to break it all down, make and test intermediate versions, and submit the patches, I'd be delighted).

The best I would be able to do is to annotate my version with some additional comments to help make it more readable for Mark and co., and hopefully that would explain the new bits sufficiently for it to be considered as a new version.

If there's a chance of it being accepted that way, I'll try to get myself some time allocated to write some more comments. If there's no chance of it being accepted, then it's probably more practical for me to look at forking a new module instead (which would definitely suck, but I understand if that's the case). If there's middle ground I'm not seeing, please do let me know.

I was working on this module at my old job. I no longer have any desire to maintain it and would be happy to transfer ownership.

Signify: The reason I could not accept your large patch is also partly because you originally told me in #33 that your "work with this module was complete" and that you were planning on "leaving" as it were. But now you are still around, reviewing patches, putting in the time. So are you are planning on supporting this?

Lars has actually taken over my old position. Just a few thoughts:

1) Coder module's word is final. As per Drupal's coding standards. ;)

2) There are +- 77 people using this module. Will your big patch above break their installations? Are you prepared to answer their issues in the queue and maintain this module?

My suggestion is that I give CVS access to you and Lars, and you then either make a new branch (version 3) and break backwards compatibility, or you develop the existing version 2 branch and maintain the module properly (the advantages of which are numerous and compelling, of course). Work together on this one.

If you agree, make a new issue requesting transfer of ownership and post it in here so you can be granted CVS access.

Thanks for your feedback Phil. I'll try to clarify.

There are not many people who actually use the current 6.x-2.x-dev version but still it's good practice to provide an upgrade path for them. I'm just starting to work on GSA and I'd find it very hard to make a case for a series of patches to implement a couple of features when they are not even my features to begin with. And you don't have the time either.

My latest patch is very similar to your #33 version which is good. That's where we want end up with to have all the new features available. By putting this patch together I wanted to understand the differences between 6.x-2.x-dev and #33 (for my results see the list of changes in #52) so that 6.x-2.x-dev users get an idea what might break if we proceed with #33. The #52 list is surprisingly short and seems seems quite manageable for the amount of code changes in #33, most of the changes were feature additions.

In reply to #53:
1. I changed some formatting in keeping with the 6.x-2.x-dev style, haven't tested against the coder module though.
2. I had to remove google_appliance_search_form from hook_theme because it caused an error on the Google Appliance search form.
3. The function google_appliance_block_save() went in unchanged from #33.
4. Point taken for the vars in google_appliance_admin_settings, I'll remove them.

Phil, what's your view on providing an upgrade path for 6.x-2.x-dev users for the issues in my list from #52? Once we get an idea about this we can make a call about rolling #33 into 2 or branching to 3.

Attached a quick re-roll minus 4.

Hi guys,

That all sounds quite promising to me.

• Mark:
  

  Signify: The reason I could not accept your large patch is also partly because you originally told me in #33 that your "work with this module was complete" and that you were planning on "leaving" as it were.

  Yes, at the time it did seem like the end of my involvement with the GSA, but some new projects have kept me interested in the future of the module.

  But now you are still around, reviewing patches, putting in the time. So are you are planning on supporting this?

  I can't make any promises, but FWIW I currently believe I will be able to provide some limited support. If GSA work falls completely off my radar, though (and strictly speaking it is not quite back on my radar yet), then I am unlikely to be able to spend much (if any) time on this. I am willing to help out if I do have the time, though.

  There are +- 77 people using this module. Will your big patch above break their installations? Are you prepared to answer their issues in the queue and maintain this module?

  It's certainly possible that my patch will break installations of 6.x-2.x-dev, but I'd be a bit stunned if anyone was using a -dev release and expecting to be able to upgrade without changing anything. Facilitating that wasn't a goal of mine when I did the work -- there has never been a stable 2.x release, so I presumed everything was potentially in flux, and that any users would be well aware of that.

  My suggestion is that I give CVS access to you and Lars, and you then either make a new branch (version 3) and break backwards compatibility, or you develop the existing version 2 branch and maintain the module properly (the advantages of which are numerous and compelling, of course). Work together on this one.

  That sounds promising. I can't spend any notable time on this until I'm actively working with a GSA again, but I am anticipating that will happen in the next few months, and in the meantime I can likely help Lars integrate the work I've done into the -dev branch, or at least answer any questions he has about my code.

• Lars:
  

  My latest patch is very similar to your #33 version which is good. That's where we want end up with to have all the new features available. By putting this patch together I wanted to understand the differences between 6.x-2.x-dev and #33 (for my results see the list of changes in #52) so that 6.x-2.x-dev users get an idea what might break if we proceed with #33.

  Right, it makes more sense to me now that I know you're taking over the maintenance!

  In reply to #53:
  1. I changed some formatting in keeping with the 6.x-2.x-dev style, haven't tested against the coder module though.

  Unless you personally find the more compressed style easier to scan, I'd encourage you to stick with the (to me) much more readable white-space/alignment that I'd used. Coder and the Drupal coding standards have nothing whatsoever against that formatting, and personally I don't think the (to me) less-readable spacing in the existing code is something that needs to be adhered to simply because that's how it was originally written. Improved readability is never a bad thing. This stuff comes down to personal preference, though, and it is your call.

  2. I had to remove google_appliance_search_form from hook_theme because it caused an error on the Google Appliance search form.

  My apologies. I didn't get any errors on my site, but potentially that's because I *was* themeing it. Perhaps we need a stub theme function in the module as well? Or is it up to the theme's template.php to define its own hook_theme() entry for forms in these circumstances?

  3. The function google_appliance_block_save() went in unchanged from #33.

  I saw (&$form, $form_state) after applying your patch, but ($form, &$form_state) in #33 ?

  Phil, what's your view on providing an upgrade path for 6.x-2.x-dev users for the issues in my list from #52? Once we get an idea about this we can make a call about rolling #33 into 2 or branching to 3.

  I was going to say that I wasn't especially concerned about it given the 2.x-dev status, but the list you've come up with looks simple enough to be worth doing -- just three persistent variable name changes, and one database table name change. Those should be easy to code as update queries, and I don't believe the other items require any work at all.

  I did change the default search path from search/google_appliance (defined by search.module based on hook_search()) to search/google-appliance (defined by variable_get('google_appliance_default_search_path', 'google-appliance')), so any existing users relying on the old path will need to re-configure that in the admin settings form. Underscores in URLs always seems like bad form to me, but I would expect new users to visit the admin settings form and configure things to their own requirements, so we could revert that path for backward compatibility if you thought it worthwhile.

Phil, thanks for your comments, seems like we are all on the same page :-)

I attached a new patch that we might actually want to commit to dev version 2. After all, this seems easy enough. My list of new changes:

1. I reverted to the string concatenation should be formatted without a space separating the operators (dot .) and a quote string concatenation style so that google_appliance.module now passes the coder module review. I left in the single space style between array elements. Coder states Arrays should be formatted with a space separating each element and assignment operator as do the array elements in coder.module itself. It's also my personal preference though I tend to agree the aligned white space style makes the code easier to read. It won't be a show stopper for me to get this patch committed should you decide to change the style back to your original one.
2. I added a stub function theme_google_appliance_search_form() to avoid the error I'm getting. Definitely a good idea to have a stub in there so that this theme can be implemented any way you want rather then not having it in at all.
3. Changes in google_appliance.install. The standard schema is now called cache_google_appliance. I added an update hook to do two things, rename the variables and rename the cache_google table should it exist. Will the upgrade still work if users set the variable google_appliance_default_search_path to anything other then google-appliance?
4. I re-added the Implementation of hook_simpletest() comment
5. Removed & from google_appliance_block_save() (sorry!)

Mark, if there are no objections, do you want to apply the patch or do you want to promote us to committers first?

You guys can take over. Make a new issue in this issue queue requesting transfer of maintainership. I will reply saying I agree. Then you apply for CVS access quoting that as reference.

http://drupal.org/cvs-application/requirements

Just post back in here with the issue when you've made it. Cheers chaps.

Looking good in general, Lars :)

Some notes on the update. (I haven't done any testing, mind.)

Changes in google_appliance.install. The standard schema is now called "cache_google_appliance". I added an update hook to do two things, rename the variables and rename the cache_google table should it exist.

The variable update looks very slightly dodgy (you're setting new persistent variables where potentially no old version existed). Ideally we'd only set a new variable if the old one existed. I think the clean way to do that would be:

I can't see another way to establish for certain whether or not a variable exists without looking at the global $conf array, but this approach also stops local changes to the global $conf from confusing matters, which I think is a good thing. It does have the potential to break for users who override the global $conf (using the old variable names), but any such users will need to deal with their override code in any case, so I suspect the most correct thing to do is to update what is in the database without regard to $conf overrides. (If you disagree, just replace $conf = variable_init(); with global $conf;)

OTOH none of that is a concern unless the user installed the old module and never configured it through the admin settings page, which is incredibly unlikely, so it's probably a moot point. (This is more a case of me dumping my thoughts on something I haven't needed to consider before.)

I've also spotted a slight inconsistency in the old code. google_appliance_admin_settings() sets the debug value to '' by default, but all other uses of the variable that I can see use zero as the default option, so I think the admin_settings default should be changed to also be zero, and it would be preferable not to copy an empty string during the upgrade.

That's just a textfield in the settings form, so an empty string is a (visually) reasonable default. Maybe we should change it into a select list with hard-coded options?

Will the upgrade still work if users set the variable "google_appliance_default_search_path" to anything other then "google-appliance"?

Hmm. Existing users will have a menu item for 'search/google_appliance/%menu_tail'. The upgraded module will over-ride that when the menu is rebuilt, but I don't know whether that will happen automatically. We do need it to happen, though, so potentially we'll need an explicit menu_rebuild() in the update?

Removing myself as the assignee.

Phil,

that's fine, I changed google_appliance.install so that it only creates a new variable if the old one already exists. It probably wouldn't matter functionality-wise in 99% of the use cases but I agree it's cleaner this way.

google_appliance_debug now defaults to 0 instead of ''.

menu_rebuild() gets run by update.php as part of drupal_flush_all_caches() after each run in update_finished() so we should be fine.

I added these bits to my #58 patch and rather then rolling yet another patch commited the whole lot to dev version 2. I also updated the project page to reflect the new features as from the updated README.txt.

If there are no objections I think we should now close this issue and start to log new issues against the updated dev version 2. For example adding a select box for debug settings would be a good new feature worth logging an issue against so that you or someone else can come up with a patch we can review and possibly commit.

I'm pleased we got this conflict with different versions of the same module resolved and now have a good base to work from. This latest patch adds a lot of new features which is great though I think we should now try to go back and supply patches in a more piecemeal fashion on a per issue basis.

I'll have some time the next couple of weeks to test the new features. I'd really like to aim for an official 6.x-2.x release next.

Cheers Lars, that all sounds good :)

Actually, does the hook_update_n function name need changing?

See http://api.drupal.org/api/function/hook_update_N/6

To be honest, after reading that I'm still unclear as to which functions we need if we're going to catch users upgrading from 5.x-1.x as well as users who installed the previous 6.x-2.x-dev release.

I think google_appliance_update_6201(&$sandbox) is correct for the latter, at least. Do we then need dummy 6000 and 6200 updates to let 5.x-1.x users even get to that? Or will it just work for all users?

jweowu,

yes it does, looking at the D6 api doc. I created issue #528428: google_appliance_update_1 should be renamed to google_appliance_update_6201 for this and will supply a patch.

It was not my intention to supply an upgrade path from the D5 version. The changes in google_appliance.install are meant to provide for a smooth upgrade between the previous and latest D6 dev 2 versions.

Having said that, D5 users upgrading to the D6 dev 2 version should be fine with the latest google_appliance.install changes as well.