Bug 9531

Summary: Make SIP2 message terminator configurable via SIPconfig.xml
Product: Koha Reporter: Kyle M Hall <kyle>
Component: SIP2Assignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: adrien.saurat, chris, colin.campbell, f.demians, gmcharlt, katrin.fischer, nengard, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7619
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 9865    
Attachments: Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml
Signed-off patch
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml
Bug 9531 - Make SIP2 message terminator configurable via SIPconfig.xml

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.