CVS edit link for skomorokh

I'm writing some GPL'd Drupal modules and I'd like to share!

My first one is a Drupal integration of the Pazpar2 metasearching software. It's primarily for library catalogues (Z39.50 and SRU/SRW) but is starting to become more generic as we add SOLR support.

http://www.indexdata.com/pazpar2

There seems to be no module for using Drupal as the UI for a generic library or other metasearching system. This module also supplants the existing defunct module providing a Z39.50/SRU client via the PHP bindings for our YAZ toolkit since Pazpar2 can be set up to search even just one target and then essentially acts as a web service for YAZ:

http://drupal.org/project/z3950

CommentFileSizeAuthor
#1 mkdru-6.x-1.x-dev.tar_.gz28.77 KBskomorokh

Comments

skomorokh’s picture

skomorokh commented
Component: Miscellaneous » new project application
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new mkdru-6.x-1.x-dev.tar_.gz28.77 KB

Tarball attached. I've included jQuery BBQ for your convenience as it's not in D6. I realise that when I go to check it in I'll need to leave it off and instead put instructions on where to put that file.

Also required is a running install of Pazpar2. It's a web service and is in Debian, Ubuntu, possibly other distros. It usually needs to be proxied through apache to a path on the same domain to allow XHR to see it. There are some default targets and even an apache config file to include so it should be pretty straightforward.

http://www.indexdata.com/pazpar2

This module is similar to our proprietary UI product Masterkey2 which runs on a stack of software, some open source, some not. While the module I'm submitting is compatible with the proprietary components it can be used to good effect with only GPLv2 software, namely Pazpar2. Some more details on how pieces fit together:

http://www.indexdata.com/masterkey

A demo of the standalone one that served as inspiration for the module (but is entirely different code against the same webservice/js library from Pazpar2):

http://mk2.indexdata.com/

And finally, yes, the module actually runs. It's rather plain because I've tried to make it as theme-overridable as possible to allow very tight integration:

http://mkdru.indexdata.com/node/3

skomorokh’s picture

skomorokh commented
Status: Needs review » Closed (won't fix)

It's feature complete for initial release, just up for some performance tweaking. I'm actively developing it and wanted as much feedback as possible while I'm still on this project full time so thought this was the right place.

I found out about the sandbox concept on IRC. And that d.o is going git, yay! I'll move it from our company git/bugzilla to that and hopefully solicit a reviewer or two from IRC before this process later when development slows.

Hopefully I close this before you read it, but if you've read it, sorry about that. I'll put a link to the sandbox for continuity once it exists.

skomorokh’s picture

skomorokh commented
Status: Closed (won't fix) » Needs review

Okay, a bit confusing. Apparently this IS the right place for what I'm after.

skomorokh’s picture

skomorokh commented

Questions/detail:

What I'm embedding is a Javascript client for an XML-based webservice. It polls for updates from a metasearch of several targets so that results can be displayed before they all report in.

Some situations will require searches to different target configs via a parameter to the service, different authentication, or different instances of the service. To accommodate this, I've implemented it as a node type even though there will usually only be 1-3 instances of the type. It seemed like a tidy way to have a place for the per-node configuration and tie into users/roles, paths, blocks, menus, etc.

In hook_block I dynamically generate blocks for each search node. However the facets (lists of sources/authors/subjects to optionally limit the search) share blocks across the nodes in an attempt to keep the block menu from getting too long. Instead I insert some php to limit the visibility to search nodes by default.

I contemplated instead having per-node facet blocks as we may add more facet types and perhaps make that configurable more granularly. It seems like one block for the content type would still work as you could change them to be explicitly displayed on only the nodes/paths where they're desired and theme them individually with template suggestions. But perhaps per-node blocks would be cleaner?

In general, is this architecture reasonable? It's my first Drupal module and I hope it's not abusing the system too badly?

arianek’s picture

arianek commented
Status: Needs review » Postponed

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

  • The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
  • Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.
kevin.reiss’s picture

kevin.reiss commented

Interesting module. I was able to get it working without a hitch over an existing pazpar2 installation. One thing that might be useful is integration with views. That would be helpful in localizing the display with other library services. The one that comes to mind is showing whether or not a search result is actually available for loan or include a link to the full-text of the result. These services are usually dependent on sending a standard number (ISBN, OCLC Number, or local system identifier) to a library management system.

skomorokh’s picture

skomorokh commented

Hi Kevin, thanks for testing it out! I'm shortly going to migrate this to a drupal.org git sandbox as per the above. It's now ported to D7 and is well on its way to gaining generalised facets so you can have a themable facet interface for whatever you tell pazpar2 to track a termlist for. When it's up on git I'll post here and in the library group. Hopefully sometime next week if not sooner.

As far as views---we're looking into it. This implementation is a Drupal-friendly javascript client for pazpar2's webservice and as such the search results never pass through Drupal's PHP or even the webserver hosting it. Views 3 (currently alpha) gives views the ability to have pluggable non-database backends so I'm going to see if, time permitting, I can convince it to consume pazpar2's search results. Remains to be seen how practical it is though, what with session management and frequently changing metasearch data set, etc.

kevin.reiss’s picture

kevin.reiss commented

Hi,

Thanks for your response. It is great that you are planning to put up the module on drupal.org as a sandbox project. This gives drupal libraries a chance to experiment with a powerful tool like pazpar2. Looking forward to the next iteration.

skomorokh’s picture

skomorokh commented

Hi again Kevin, sorry about the delay. It's on git now with arbitrary facets configurable via AJAX:

http://drupal.org/sandbox/skomorokh/1103554

skomorokh’s picture

skomorokh commented
Status: Postponed » Needs review
skomorokh’s picture

skomorokh commented
Status: Needs review » Postponed
skomorokh’s picture

skomorokh commented
Status: Postponed » Needs review
skomorokh’s picture

skomorokh commented
Project: Drupal.org CVS applications » Drupal.org security advisory coverage applications
Component: new project application » module
skomorokh’s picture

skomorokh commented
Title: skomorokh [skomorokh] » Pazpar2 metasearch integration
skomorokh’s picture

skomorokh commented

The module is now linked from our site and I've written up some documentation:
http://www.indexdata.com/software/mkdru
http://www.indexdata.com/software/mkdru/docs

jthorson’s picture

jthorson commented
Priority: Normal » Critical

Updating priority according to the new project application priority guidelines. The application's priority should be set back to normal once a reviewer responds to your application and the application review process has continued.

alexreinhart’s picture

alexreinhart commented
Priority: Critical » Normal
Status: Needs review » Needs work

I'm taking a look through the 7.x branch of this module for a review. Here's some of my initial comments:

  • mkdru_node_access() should check the type of the node passed to it before deciding on the user's permissions, shouldn't it? If I understand correctly, it'll be called even for nodes that have nothing to do with your module.
  • You have a leftover dpm() in mkdru_add_facet_form.
  • In mkdru_settings_form, you concatenate $facet into some form #title fields. If it is possible for $facet to contain HTML entities, this will allow arbitrary HTML to be injected. I'd suggest changing $facet . ' ' . t('facet') to t('@name facet', array('@name' => $facet)) or something similar.
  • Ah, now I see you check entries in mkdru_settings_validate to be alphanumeric. You might use ctype_alnum() rather than regular expressions, since it's more straightforward.
  • Your hook_uninstall does not remove the comment_mkdru variable you create in hook_install.

Otherwise there's not much to complain about. The module looks good, although I have not installed it to test. Once you've resolved these issues, set the status here back to "needs review" and I'll take another look.

skomorokh’s picture

skomorokh commented
Status: Needs work » Needs review

Thanks for looking this over for us! And for looking over modules in general, it really helps Drupal to have eyes on these.

I've added some checks in hook_node_access, it is called for all nodes. hook_access in D6 isn't though so I left that version.

Grepped the whole tree for dpm just to make sure I hadn't missed any others.

I didn't use the ctype_alnum() function since I wanted to also allow undescores and dashes and I think in that case the regex is most clear.

Thought those settings were already there, oops. I've added some more and also make sure to delete them in hook_uninstall.

alexreinhart’s picture

alexreinhart commented
Status: Needs review » Needs work

I see you added this line in hook_node_access:

if (in_array($type, node_permissions_get_configured_types())) {

It's my understanding that node_permissions_get_configured_types() returns all node types, including those created by other modules. You need to check $type against your own module's node type instead.

skomorokh’s picture

skomorokh commented

Your understanding is absolutely correct, I've confirmed it with dpm(). This part of the doc is confusing:

Return value

An array of node types managed by this module.

Added a commit to just compare explicitly.

skomorokh’s picture

skomorokh commented
Status: Needs work » Needs review
alexreinhart’s picture

alexreinhart commented

Yeah, the docs are a bit confusing there. I filed an issue about the misleading documentation.

A question: I'm not particularly familiar with the templating system. However, I see that mkdru-form.tpl.php includes some English text that's not in t(). Is that normal for templates, or should it be directly translatable?

skomorokh’s picture

skomorokh commented

The templates are really just a stub meant to demonstrate theming for the module, I doubt anyone will put this into production without changing it to fit their design. At least, I hope not :)

Couldn't find much on it being normal or not but I think it's a good thing for any themers to see. Spreads awareness of Drupal's having good localisation tools. Templates updated.

If you want to take a look at the module in action without setting up Pazpar2:
http://mkdru7.indexdata.com/

For a partial list of the libraries you can metasearch with this:
http://irspy.indexdata.com/

alexreinhart’s picture

alexreinhart commented
Status: Needs review » Reviewed & tested by the community

Looks good. Setting to reviewed & tested, so now you just need to wait for an administrator to grant you the project promotion permission.

red29’s picture

red29 commented

Wait for what the administator grant you then you should take action.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Reviewed & tested by the community » Fixed

Thank you for your patience. Congrats!

skomorokh’s picture

skomorokh commented

Thanks everyone!

I moved to a short URL and am preparing a 1.0 release, but I notice the old sandbox URL has changed to "Page not found". Might be better for promoted sandboxes to be an HTTP redirect in future so old posts and links with the sandbox URL (and search engines) have a chance to update their pointers / find out why their git repo lost track of origin?

Status: Fixed » Closed (fixed)

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