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

Serializing java long type to php integer #2

Closed
middlebrain opened this issue Feb 26, 2018 · 10 comments
Closed

Serializing java long type to php integer #2

middlebrain opened this issue Feb 26, 2018 · 10 comments

Comments

@middlebrain
Copy link

Hi.

thanks for providing this serializer. Very helpful and easy to use.

With the current implementation however, there is no builtin adapter to serialize a java long to a php Integer. Casting long to int is not a good idea and writing a custom adapter seems not possible without extending the Writer class.

Another small issue: The documentation says, that registerBuiltinAdapters() registers all built-in adapters. That's not true. For example, the DoubleAdapter and the MapAdapter are missing.

@marcospassos
Copy link
Owner

marcospassos commented Feb 27, 2018

Hi @middlebrain, thanks for reporting these issues.

With the current implementation however, there is no builtin adapter to serialize a java long to a php Integer. Casting long to int is not a good idea and writing a custom adapter seems not possible without extending the Writer class.

As you pointed out, longs cannot be cast to integers indiscriminately as it may overflow the limits of the integer type. The writer supports writing any data type supported on the PHP side, so I can't see how to handle it at this level. That being said, the most straightforward approach for handling this case is serializing longs to strings, the same approach largely adopted when serializing big numbers to JSON in most programming languages. Alternatively, you can write a custom adapter that converts longs to GMP numbers. In fact, the latter approach is pretty interesting, and I would consider accepting a PR if you want to implement it.

Notice that PHP will convert any value beyond the bounds of the integer type to a float, with precision loss. It means that you can just serialize longs as integers if the precision loss is not an issue for your requirements.

Another small issue: The documentation says, that registerBuiltinAdapters() registers all built-in adapters. That's not true. For example, the DoubleAdapter and the MapAdapter are missing.

You're right. I'll review it and release a new version fixing it.

Thank you for reporting it and let me know if you have any other issue.

@middlebrain
Copy link
Author

I believe, that the Java type long can be easily converted to the PHP type integer, at least for 64bit target platforms. Unserializing on 32bit platforms is a problem of course.

So I was thinking of something like this:

Writer.java:

...
     /**
     * Writes an long value to the buffer.
     *
     * @param value The value.
     */
    public void writeLong(Long value)
    {
        setState(state.value());

        buffer.append("i:");
        buffer.append(value);
        buffer.append(';');
    }
...

adapter/LongAdapter.java:

package com.marcospassos.phpserializer.adapter;

import com.marcospassos.phpserializer.Context;
import com.marcospassos.phpserializer.TypeAdapter;
import com.marcospassos.phpserializer.Writer;

/**
 * Adapter for long values.
 */
public class LongAdapter implements TypeAdapter<Long>
{
    @Override
    public void write(Long value, Writer writer, Context context)
    {
        writer.writeLong(value);
    }
}

At least for my use case, that would be enough. Not perfect, but better then casting long to int on the java side.

@marcospassos
Copy link
Owner

Got your point now.

Could you please open a PR and add some tests to cover the feature?

@marcospassos
Copy link
Owner

As PHP makes no distinction between longs and integers, how about using method overloading instead of a new method?

  /**
     * Writes an long value to the buffer.
     *
     * @param value The value.
     */
    public void writInteger(Long value)
    {
        setState(state.value());

        buffer.append("i:");
        buffer.append(value);
        buffer.append(';');
    }

@middlebrain
Copy link
Author

Even better. With my modest Java knowledge I didn't come up with that.

@marcospassos
Copy link
Owner

marcospassos commented Feb 28, 2018

I just released the version 0.6.0 that covers this issue. Could you please check if it covers your use case?

You can check the full release description here:
https://github.com/marcospassos/java-php-serializer/releases/tag/0.6.0

@middlebrain
Copy link
Author

middlebrain commented Feb 28, 2018

Not yet. The DoubleAdapter is not correct registered in SerializerBuilder.java line 198:

registerAdapter(Double.class, new IntegerAdapter());

@marcospassos
Copy link
Owner

marcospassos commented Feb 28, 2018

The bug is fixed. In addition, I've added several tests to avoid this kind of mistake.

Can you please confirm it fixes the issues?

@middlebrain
Copy link
Author

It works out of the box now.

Thank you for your quick solution.

@marcospassos
Copy link
Owner

Closing as the issue has been fixed.

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