Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segment separators are not correct #22

Closed
arnowelzel opened this issue Mar 10, 2020 · 14 comments · Fixed by #23
Closed

Segment separators are not correct #22

arnowelzel opened this issue Mar 10, 2020 · 14 comments · Fixed by #23

Comments

@arnowelzel
Copy link
Contributor

In certain places the segment separators are defined as following:

$this->hl7Globals['SEGMENT_SEPARATOR'] = '\n';

However - this is not correct:

  1. The sequence '\n' will translate to two characters and not to a newline.
  2. The separator must be "\015" (ASCII 13, CR) and not just a newline.
@senaranya
Copy link
Owner

You can override the segment separator while creating Message object:

$msg = new Message("MSH|^~\\&|1|\rPV1|1|O|^AAAA1^^^BB|", ['SEGMENT_SEPARATOR' => '\r']);

@arnowelzel
Copy link
Contributor Author

Also see here: https://healthstandards.com/blog/2007/09/24/hl7-separator-characters/

The segment separator is not negotiable. It is always a carriage return (ASCII 13 or HEX 0D).

And for my particular use case I do not create a new message. The fragment is as follows:

// $message is a HL7 message recieved from some other system
$request = $hl7->createMessage($message);

// Create a response to be sent back
$response = new ACK($request);

@senaranya
Copy link
Owner

Please use new Message() instead of new HL7() that you might have used. The documentation at https://github.com/senaranya/HL7#creating-new-messages describes various options of creating the Message object from a HL7 string.

I know \n is non-standard, but a lot orgs have configured their RIS to accept it. Making a core change in the default such as this is going to break a lot of systems that are using this library.

@arnowelzel
Copy link
Contributor Author

I understand the problem. But how to fix the behaviour for new ACK()? This will create a message with the wrong separator which is even literally \n (two characters: backslash and n) and not a newline. At least you should replace '\n' by PHP_EOL or "\n".

@senaranya
Copy link
Owner

You can pass a Message object to the constructor of ACK:

$msg = new Message(<HL7 string>, ['SEGMENT_SEPARATOR' => <separator character>]);
$response = new ACK($msg);

@senaranya
Copy link
Owner

Did the above work for you?

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Mar 13, 2020

No, the ACK still contains "\n" (backslash followed by "n" (ASCII 92, ASCII 100)) and not CR (ASCII 13) as segment separator.

Edit: the code fragment I tried:

$request = new Message($message, ['SEGMENT_SEPARATOR' => "\015"]);
$response = new ACK($request);
$responseData = $response->toString();

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Mar 13, 2020

I think the problem is, that the constructor of ACK first creates a message but without any optional parameters for the segment separators and just builds a new message with the default values.

Maybe it would be better to just have one instance of HL7 which is then used for all operations and holds a set of defaults for messages which can be passed in the constructor would be even better, so one does not always have to pass the globals as a parameter.

So this could be used then like this:

$hl7 = new HL7(['SEGMENT_SEPARATOR' => "\015"]);

$request = $hl7->createMessage($message);
$response = $hl7->createACK($request);
$responseData = $response->toString();

@arnowelzel
Copy link
Contributor Author

arnowelzel commented Mar 13, 2020

I just updated my pull request with the proposed changes.

@senaranya
Copy link
Owner

Thanks for the pull request. We shouldn't use the HL7 class though, it is not tested and not used anywhere. I've kept it thinking someday it'd be a front facade for creating HL7 messages, but Message class is working pretty well for everyone so far. So I might remove HL7 class for good.

So please replace new HL7() with new Message() wherever you've used it in the code.

->toString() takes a boolean argument. If you call ->toString(true), it's going to expand '\n' or '\r' to the corresponding characters.

Thank you for the pull request, appreciate your contribution. We'll need to make a few changes though:

  • Revert any changes in HL7.php (as explained above)
  • Add unit tests in AckTest.php covering the updates
  • Create a section under ###ACK in readme.md describing how to create an ACK response.

@arnowelzel
Copy link
Contributor Author

I'm sorry... but ->toString(true) also doesn't work. This will just replace the two characters \n (literally a backslash followed by n and not a newline) to a single character LF (ASCII 10).

Also see here:

            $message .= $pretty
                ? str_replace(['\r', '\n'], ["\r", "\n"], $this->segmentSeparator)
                : $this->segmentSeparator;

Please note: inside single quotes PHP does not expand \n as newline but literally as two separate characters! Only \' can be used to escape a single quote and \\ to escape a backslash character. Also see here:

https://www.php.net/manual/en/language.types.string.php#language.types.string.syntax.single

@arnowelzel
Copy link
Contributor Author

Maybe it would be a good idea, to expand the constructor of ACK so it can accept an optional $hl7Globals parameter to be passed to the message which it creates. Also see my updated pull request.

@senaranya
Copy link
Owner

->toString(true) is used by Connection class before sending the HL7 message, so it expands literal \r and \n to CR and NL characters respectively.

Thanks for the update in pull request. I'll merge it once tests and readme are added. If you'd want me to do it, I can do over the weekend.

@arnowelzel
Copy link
Contributor Author

Thank you. It's not that urgent - just take your time. For now I have a local workaround which will do until there is an official update of your component. Thanks for your work!

@senaranya senaranya linked a pull request Mar 14, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants