It seems like a good idea to move porterstemmer_stem() and all its helper functions into a porterstemmer.inc file, since this is a fair amount of code that doesn't get used on most page requests.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

That idea sounds fine to me. This could go even farther and make two include files, where one has only the stem function (all that is needed if you have the "stem" PECL extension installed), and the other has all the other stuff. Patch to do this and load files when necessary would be welcome!

jhodgdon’s picture

I will say though that I haven't seen other modules doing this type of thing except for admin screens.

decafdennis’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Active » Needs review
Issue tags: +needs backport to 6.x
FileSize
0 bytes
jhodgdon’s picture

Status: Needs review » Needs work

Thanks... but could you perhaps try uploading the patch again? I'm showing a 0 byte patch, and when I click on it I cannot see anything in the file.

decafdennis’s picture

Status: Needs work » Needs review
FileSize
41.46 KB

I'm sorry. I don't know what went wrong, but here's the patch again!

jhodgdon’s picture

Thanks! This looks reasonable... We'll see about getting it committed.

deanflory’s picture

This still hasn't been committed right since there isn't a newer release?

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Sorry, this fell off of my radar. Assigning it to myself so I can get it committed soon.

Pravin Ajaaz’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Looks good to me.

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

This patch actually has a couple of problems:

a) It removes the @file doc block from the .module file without replacing it.

b) The include file only needs to be loaded if the PECL stemming library is not present, so it should only be loaded in that case if you actually want to reduce the footprint to its minimum.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
mark_fullmer’s picture

Assigned: Unassigned » mark_fullmer
mark_fullmer’s picture

Status: Needs work » Needs review
Issue tags: -needs backport to 6.x
FileSize
44.34 KB

The attached patch loads the local stemming code only if the PECL library is not present, and brings the .module code and .inc file up to the latest Drupal CodeSniffer ruleset standards. All simpletests pass, as well: https://www.evernote.com/l/AlW19cSigq9A9IgVCMvW88mb4TG0nmm6V98

jhodgdon’s picture

Status: Needs review » Fixed

This looks great, thanks! I've committed it to the project, with a minor change to the @file comments at the top of both porterstemmer.module and the new standard-stemmer.inc file. (Basically, I put the comment about the implementation of the stemming algorithm in the include file, because that is what is now in the include file, and the main module file now doesn't have the implementation in it.)

Status: Fixed » Closed (fixed)

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