Currently, the filter recognizes "Phi" as an abbreviation for Philippians, but not "Phil." Bible Gateway, on the other hand, only recognizes "Phil," not "Phi." This patch changes the filter's regex to accept "Phil" only.

Note that this will, of course, break any existing references on your site that use "Phi," but given that Bible Gateway just gives a "not found" when you click those links anyway, I figure they're already broken, so no loss. :) Not sure how this will work if you're pointing the filter at someone other than Bible Gateway, though...

CommentFileSizeAuthor
#7 Philippians_Abbreviation-274953-7-D6.patch1.32 KBjamesoakley
#6 Philippians_Abbreviation-274953-6.patch1.32 KBjamesoakley
scripturefilter.inc_.patch753 bytesmuriqui

Comments

Steve Simms’s picture

Steve Simms commented

Hi batdan,

Given that this would break anyone's Philippians reference who isn't using Bible Gateway, I'd rather not implement it as is. Could you change it to automatically convert "Phi" to "Phil" when it's making the link instead? That way, it would fix everyone's links, rather than risking breaking any.

muriqui’s picture

muriqui commented
Assigned: Unassigned » muriqui
Status: Needs review » Needs work

I was afraid you were going to ask me that. :) Let me see what I can do...

mdlueck’s picture

mdlueck commented

Does the module work correctly when not using abbreviations for the books of the Bible? I just learned of this module, so at this point (NOW!) is the time to make sure data is entered correctly into Drupal. Thanks!

muriqui’s picture

muriqui commented

@mdlueck: It should, yes.

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Version: 5.x-1.1 » 7.x-1.x-dev
jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Assigned: muriqui » Unassigned
Status: Needs work » Needs review
StatusFileSize
new Philippians_Abbreviation-274953-6.patch1.32 KB

I think this fixes it, and also provides an easy way to fix any other abbreviations that need to be altered before passing to Bible Gateway.

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
StatusFileSize
new Philippians_Abbreviation-274953-7-D6.patch1.32 KB

and this version should patch against 6.x-1.x, to allow those still using Drupal 6 to assist with reviewing this solution.

muriqui’s picture

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

Thanks, James... Sorry I completely dropped the ball on this one.
Just tested the patch on D7 and it seems to work fine. Philippians, Phil, and Phi are all recognized and properly linked for both Bible Gateway and ESV. Seems like a pretty straightforward fix to me, so marking RTBC.

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Reviewed & tested by the community » Needs review

Thanks for testing that so soon. I've committed the 7.x-1.x patch.

I'll wait until either I or someone else has had time to double-check the 6.x-1.x patch works correctly before I commit that one.

mdlueck’s picture

mdlueck commented

I will test the D6 patch. I had actually advised against using abbreviations as editing the one node you and I have been working on the XHTML validation with, I found abbreviations were not getting picked up several places. I will apply this D6 patch on top of the XHTML validation patch.

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented

Thanks in advance.

I think the code changes for this and the XHTML validation patch are far enough removed from one another that you should be able to apply them in either order.

If you had time to look at that node to see which abbreviations were being missed, you could open another issue about that. That would give us the opportunity to tighten up on the abbreviations. It's a bit like comment #1 in this thread - given the module does abbreviations, we'd need a very good reason to remove the functionality, even if it doesn't pick everything up.

mdlueck’s picture

mdlueck commented

Hunk #2 failed to apply via patch. You appear to have had whitespace differences in the leading

$verse = strtok($verse,',& ');

LOC. So I applied hunk #2 manually. Seems to work / mod does not crash or anything bad.

I tested adding Matt > Matthew to the list and it still would not detect the Matt. 12:33 reference, so I will open a new bug siting this bug as reference.

I have the Matt > Matthew on a test copy of the site, only your patch provided here is on prod.

Thanks!

muriqui’s picture

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

Just repeated my tests from #8 with the D6 patch... I can confirm that the patch applies properly against 6.x-1.x-dev, and that it fixes the Philippians abbreviation issue the same as it did for D7. I can't speak to mdlueck's problem in #12 when applying this on top of another patch, but as far as this issue by itself is concerned, I think it's good to go... RTBC again?

jamesoakley’s picture

jamesoakley
Primary language English
Location North Yorkshire, UK
commented
Status: Reviewed & tested by the community » Fixed

Thanks both. That's committed to 6.x-1.x as well.

Status: Fixed » Closed (fixed)

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