Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml
Summary: Make SIP2 message terminator configurable via SIPconfig.xml
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 9865
  Show dependency treegraph
 
Reported: 2013-02-01 15:40 UTC by Kyle M Hall
Modified: 2015-06-04 23:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml (8.05 KB, patch)
2013-02-01 15:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Signed-off patch (8.36 KB, patch)
2013-02-09 20:28 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml (8.36 KB, patch)
2013-05-17 12:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml (8.25 KB, patch)
2013-05-21 13:28 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml (8.30 KB, patch)
2013-05-23 06:53 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2013-02-01 15:40:39 UTC
As stated by a number of comments on bug 7619, the message terminator for SIP2 messages should be configurable as neither CR ( the standard terminator ) nor CRLF ( the more compatible terminator ) give full compatibility for all SIP2 devices.
Comment 1 Kyle M Hall 2013-02-01 15:46:29 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-02-08 17:00:54 UTC
I'm upgrading this to a bug, as the existing behavior causes incompatibility with many SIP devices and services ( such as Overdrive ).
Comment 3 Frédéric Demians 2013-02-09 20:28:29 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-02-11 12:56:09 UTC
Frédéric, since the behavior is controlled at the "accounts" level, 1 SIP server instance can handle both styles! Yes, I think I will alert Nicole to add this to the manual.
Comment 5 Frédéric Demians 2013-02-11 13:17:24 UTC
(En réponse au commentaire 4)
> Frédéric, since the behavior is controlled at the "accounts" level, 1 SIP
> server instance can handle both styles! Yes, I think I will alert Nicole to
> add this to the manual.

Yes, even better. Whatever am I thinking of?
Comment 6 Adrien SAURAT 2013-03-20 10:05:07 UTC
Tested OK.
No big deal but I'm wondering: if the default mode is still CRLF when the parameter is not set, shouldn't we set the default line in SIPconfig.xml to CRLF instead of CR?
Comment 7 Kyle M Hall 2013-03-25 12:25:13 UTC
I was under the assumption that the line I altered was an example. I can post a followup to change that if you wish.

(In reply to comment #6)
> Tested OK.
> No big deal but I'm wondering: if the default mode is still CRLF when the
> parameter is not set, shouldn't we set the default line in SIPconfig.xml to
> CRLF instead of CR?
Comment 8 Katrin Fischer 2013-05-14 06:42:14 UTC
Hi Kyle, 
could you perhaps provide a little follow up for the tabulation characters?

 * C4/SIP/Sip/MsgType.pm                                                    FAIL
	pod                         OK
	forbidden patterns          FAIL
		The patch introduces a forbidden pattern: tabulation character (869)
		The patch introduces a forbidden pattern: tabulation character (793)
		The patch introduces a forbidden pattern: tabulation character (781)
	valid                       OK
	critic                      OK

Apart from this all tests and the QA script pass.

SIP is hard to test and I am still not familiar with it. Therefore I would like
to call for a second sign off on this and then it can pass QA.
Comment 9 Kyle M Hall 2013-05-17 12:59:23 UTC Comment hidden (obsolete)
Comment 10 Adrien SAURAT 2013-05-21 13:28:52 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2013-05-23 06:53:08 UTC
Created attachment 18323 [details] [review]
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml

Add a delimeter option to SIPConfig.xml, choices for 'delimeter' are 'CR' or 'CRLF'.
The default continues to be 'CRLF' if 'delimeter' is undefined.

Test Plan:
1) Apply patch
2) Start SIP server
3) Run C4/SIP/t/04patron_status.t
4) Stop SIP server
5) Add terminator="CR" for account login 'term1'
6) Run 04patron_status.t again, you should see no change

Signed-off-by: Frédéric Demians <f.demians@tamil.fr>
Signed-off-by: Adrien Saurat <adrien.saurat@biblibre.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 12 Galen Charlton 2013-09-08 05:44:55 UTC
Pushed to master.  Thanks, Kyle!
Comment 13 Chris Cormack 2013-10-31 19:10:15 UTC
Tomas, Ive checked this is ok on 3.12.x too.

So if you want to push it to that, it should be fine
Comment 14 Tomás Cohen Arazi 2013-11-06 14:33:58 UTC
This patch has been pushed to 3.12.x, will be in 3.12.7.

Thanks Kyle!
Comment 15 Kyle M Hall 2013-12-20 14:06:21 UTC
This patch has been pushed to 3.8.x, and will be in 3.08.20.