Align SerializationInterface::decode() error contract across implementations

-1, we have yet to find a serializer where you can’t distinguish the difference.

Callers that need to distinguish these cases must consult the
* concrete implementation's documentation

Serializer are encouraged to be injected as a service that can be replaced, this is intentional that it allows changing to fit different needs. It’s not reasonable to expect contrib to code for concrete implementations when it’s very possible to have the API indicate a failure in a unified fashion.

As noted in #3421234: Json and PHP serializers should throw exception on failure this hit me in RabbitMQ where it could have caused data loss, there was an absolute need for us to know when to tell the message to be retried (decode error) vs passing the data to a QueueWorker for processing. We ended up having to duplicate the core serializers as a workaround.

This just appears to be a documentation update mind elaborating what the push back would be?

If I’m reading this correctly The IS says this is about choosing a definition. As I quoted in my first comment this is not a simple documentation update as currently written.

This issue is currently proposing that a module needs to know every serializer ever created for Drupal by name, even those that have not yet been created, determine which one is used, and needs to known how that serializer handles error. Maintainers alternative is to limit themselves to a single class in their constructor.

That is the exact opposite of why interfaces exist, which is to give a representation of how all implementation must uniformly respond. It would create a situation where site owners can not utilize dependency injection to change serializer as maintainers would have to approve each one by class name.

The existing API is sufficent, it indicates when an error occurs in a uniform manner, and it requires serializers to normalize their data into a uniform pattern. The only negative with it has been that one serializer in core has failed to adhere to that documented standard in the past.

Throwing an exception when an unexpected unrecoverable error occurs is normal OOP, it acts as a guard to ensure that a caller knows a fault occurred and processes it correctly. Especially true in context of Queues calling serializers where an uncaught error automatically results in a future retry acting as a guardian against data loss.

TLDR: The proposed change in this issue provides no way for a caller to know was the data actually false/null or an error condition. An existing issue already exists for fixing the one core serializer that has this bug there is no reason to allow this as normal operation.

PS: good timing this is right at the end of my 3 month response window.

I opened #2960938: Serialization component ignores error handling: add json_last_error() to Json component etc. eight years ago about this exact problem: Json::encode() and Json::decode() silently swallowing errors and requiring callers to manually check json_last_error(). That issue was eventually closed as a duplicate of #3421234: Json and PHP serializers should throw exception on failure, which takes the right approach with JSON_THROW_ON_ERROR.

I fully agree with cmlara. The documentation-only approach does not fix the underlying contract violation — it just formalizes it. Callers that depend on SerializationInterface as an abstraction cannot be expected to know which concrete implementation they are dealing with and adapt their error handling accordingly. That defeats the entire purpose of an interface.

If core and contrib need to wrap calls in try/catch blocks as a result, that is a worthwhile and correct change.

@andypost thoughts on the comments?