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

Strict types breaks serialization #11

Open
troelskn opened this issue Feb 21, 2020 · 3 comments
Open

Strict types breaks serialization #11

troelskn opened this issue Feb 21, 2020 · 3 comments

Comments

@troelskn
Copy link

Since strict typing was added, I get this error in my application:

TypeError: Argument 1 passed to Metroplex\Edifact\Serializer::escape() must be of the type string, null given, called in /var/app/gas/vendor/metroplex-systems/edifact/src/Serializer.php on line 61

/var/app/gas/vendor/metroplex-systems/edifact/src/Serializer.php:79
/var/app/gas/vendor/metroplex-systems/edifact/src/Serializer.php:61
/var/app/gas/vendor/metroplex-systems/edifact/src/Message.php:151
...

The problem is that I have added non-strings (null, integers) as values to segments as I build them up. While I could change the code on my end to coerce into string, I don't think it would be unreasonable for the library to do this transparently, as it used to. I suggest changing Serializer::escape as follows:

-     private function escape(string $string): string
-    {
-        $characters = [
+    private function escape($string): string
+    {
+        $string = (string) $string;
+        $characters = [

Would you like me to open a PR for this?

troelskn added a commit to troelskn/edifact that referenced this issue Feb 21, 2020
@duncan3dc
Copy link
Member

Hi @troelskn sorry for the delay here! Transparently converting here seems reasonable, however I'd like to bundle this with a clear error for when the value cannot be escaped, if you're like to throw up a PR for that then I would be happy to take a look, if not I'll tackle this myself when I get a moment

@duncan3dc
Copy link
Member

Hi @troelskn just checking back in if this is still an issue?

This library has never used strict types, so presumably the integers and other numbers are not a problem, the only issue you're seeing is with null? If so I'd be happy to change this method to accept null and silently convert to "" does that work for you?

@troelskn
Copy link
Author

It's been a while since I used the library, but I think the change still makes sense. It's probably fine as you suggest to only handle null this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants