Add PHP 8.1 Compatibility

[danielmorell]

Description of the change

This PR primarily is designed to resolve some compatibility issues with PHP 8.1.

The major change in this PR is the removal of the PHP built in interface Serializable from Rollbar payload objects. I have replaced Serializable with a new interface I created called SerializerInterface.

Why this change?

The intent for the built in PHP Serializable interface has been updated with PHP 8.1. It now also should declare objects to be compatible with the built in serialize() and unserialize() functions. Note I am referring to the built in functions not the class methods. For instance you could do the following....

class Foo implements Serializable {
    // implementation stuff
}

$foo = new Foo();

$foo_string = serialize($foo);

$new_foo_instance = unserialize($foo_string);

With the release of PHP 8.1 classes which implement Serializable without also implementing __serialize() and __unserialize() (which were added in PHP 7.4) will generate a deprecation warning. This has been noted by #555.

Our intent in implementing the Serializable interface has been to make objects transformable into strings that can be sent to the Rollbar service. This is why we never implemented the unserialize() method. We only encoded the data so it could be transferred. We never needed to decode it. Because of this we don't really need to implement the Serializable interface since doing so should represent a guarantee we probably have no intention of making.

My solution was to create a new interface SerializerInterface that has a single method serialize(). This should not introduce any breaking changes since the __serialize() method not serialize() is called by the built in serialize() function.

The second change in this PR is to add some additional type declarations to the Response class. This resolves some type warnings that were raised during linting.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Maintenance
• New release

Related issues

• Deprecation error in PHP 8.1 #555

Checklists

Development

• Lint rules pass locally Well not really. But the ones that passed before still pass. There are a lot of CRLF files. I have updated all the ones I touched to LF.
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers assigned
• Issue from task tracker has a link to this pull request
• Changes have been reviewed by at least one other engineer

[cyrusradfar]

I don't love the idea of having a custom interface to Serializable and an OR check on type.

I don't think we get enough and it introduces confusion. I appreciate that throwing exceptions for unimplemented functions isn't ideal either but it's clear to a developer very quickly that this function won't work.

How would you feel about implementing an Abstract class that implements Serializable. The abstract class can leave the serialize function abstract so it must be implemented by anything that extends it and implements a stub unserialize as they are today?

That would mean that TraceChain, et al. would extend this new abstract class vs a new interface.

Thoughts?

[danielmorell]

I definitely see your point. Let me walk through this option.

To do an abstract class we would need to throw implementation errors on both __serialize() and __unserialize(). The reason is... the return and argument types do not match serialize() and unserialize(). This is because as strange as it sounds, they serve two different purposes.

An abstract implementation would look something like this...

class AbstractSerializable implements \Serializable
{
    abstract public function serialize();

    public function unserialize(string $serialized)
    {
        throw new \Exception('Not implemented.');
    }

    public function __serialize(string $serialized): array
    {
        throw new \Exception('Not implemented.');
    }

    public function __unserialize(array $data): void
    {
        throw new \Exception('Not implemented.');
    }
}

Usage of this class would look something like this...

namespace Rollbar\Payload;

use Rollbar\SerializerInterface;

class TraceChain implements SerializerInterface
{
    // Other methods

    public function serialize()
    {
        $mapValue = function ($value) {
            if ($value instanceof \Serializable) {
                return $value->serialize();
            }
            return $value;
        };
        return array_map($mapValue, $this->traces);
    }
}

This all seems fine. However, our return type on TraceChain::serialize() is array and this does not align with the Serializable::serialize() interface return types.

interface Serializable {
    public serialize(): ?string;
    public unserialize(string $data): void;
}

Fortunately, the return types on Serializable implementations are not strictly enforced. So, we can get away with returning an array like we are in TraceChain::serialize().

I agree that a custom interface that really just replaces a built-in interface and then running checks against class implementations of two interfaces, is definitely not ideal. Both the new interface and the abstract class solution have tradeoffs that I'm not thrilled about.

I am open to either solution. What would you prefer to see?

[cyrusradfar]

for the thread, @danielmorell and I chatted off-thread and decided on some next actions.

We've decided to follow the direction in Daniel's original PR with some additions.

1. Update the PR to include deprecation warnings for usage of Serializable
2. Refactor DataBuilder::getCustomForPayload() to transform primitives into an array
3. The deprecations warnings warrant a minor version bump to v3.1.0

From our conversation some context from Daniel for deprecation of Serializable is relevant

[...] with PHP moving more and more toward providing better type support, and the way the community is embracing it, using Serializable without correctly implementing it seems like code smell especially looking long term.

Open to feedback from the community at-large about how we should approach.

[danielmorell]

Status Update: I have made the change @cyrusradfar and I talked about, but need to make some updates to our unit tests. I will try to have that done soon.

[danielmorell]

This method was added so that we could simplify/consolidate the logic around transforming the custom parameter into an array.

I tossed around the idea of making this method recursive. So, it could handle a nested object or multidimensional array. However, recursion could cause a lot of issues if we ran into circular references. I also don't know if we even support multidimensional data on the Rollbar server processing side. After I had thought about it a bit. I decided recursion is not a good idea here.

This is partly why I also deprecated the use of an object that does not implement the new Rollbar\SerializerInterface interface. We probably should not just be trying to serialize someone else's object without them having a very clear understanding of what will be included. This should be done by them explicitly including it in the serialize() method.

[danielmorell]

@cyrusradfar this is ready for you to take a look at it again. I made a few design decisions that we have not explicitly talked about that are worth noting.

1. The type logic in Rollbar\DataBuilder::getCustomForPayload() was moved to a new method Rollbar\DataBuilder::resolveCustomContent().
2. Passing a object to custom that does not implement Rollbar\SerializerInterface has also been deprecated. See my comments on the new method above.
3. Falsy values are transformed into an empty array.
4. Non-falsy values are turned into an array ["message" => $value].

I added test coverage for the new logic. We only tested array previously. The majority of the test changes will be found in tests/ConfigTest.php.

[cyrusradfar]

Thank you @danielmorell for all the changes and improvements in this PR. I appreciate that we're moving to be more type specific across the SDK and managing expectations with our community that we're following the ever-changing norms and standards of the PHP eco-system.