fix(mapper): serialize nested objects

[radoslav-grencik]

Fixes #2258

[xHeaven]

This seems to be a bit problematic right now. You're introducing an infinite recursion (nested objects reference each other) and also a performance regression because you are resolving all configured mappers again for every nested object.

The goal idea is good, but I'm afraid this should be a bit deeper refactor than 2 lines of change. I'll try to think of something, let's also wait for @brendt here.

Edit: added a couple failing tests for clarity.

[radoslav-grencik]

This seems to be a bit problematic right now. You're introducing an infinite recursion (nested objects reference each other) and also a performance regression because you are resolving all configured mappers again for every nested object.

The goal idea is good, but I'm afraid this should be a bit deeper refactor than 2 lines of change. I'll try to think of something, let's also wait for @brendt here.

Edit: added a couple failing tests for clarity.

Thanks, that makes sense. I reworked the fix so nested object serialization no longer calls the public mapper pipeline recursively.

The updated approach keeps recursion inside ObjectToArrayMapper and tracks visited objects with spl_object_id(). This avoids infinite recursion for cyclic graphs; cyclic objects are left for JSON encoding to fail normally instead of hanging.

It also avoids the performance regression from repeatedly resolving configured mappers for every nested object. The new test for unused mapper resolution now passes.

I also updated ArrayOfObjectsSerializer to preserve mapping context when it is used directly.

[brendt]

I'm can't remember if this was ever intended behavior, but I agree that it should be changed. However, I do wonder if this could be a potential breaking change, because we're suddenly changing how the mapper works… I for example remember manually mapping nested objects in some of my project exactly because of this bug. I don't believe my code would break, but I wonder if others might…

On top of that, I don't see an automated upgrade path for this change; all of that leads me to think we should target 4.x with this one

[radoslav-grencik]

I'm can't remember if this was ever intended behavior, but I agree that it should be changed. However, I do wonder if this could be a potential breaking change, because we're suddenly changing how the mapper works… I for example remember manually mapping nested objects in some of my project exactly because of this bug. I don't believe my code would break, but I wonder if others might…

On top of that, I don't see an automated upgrade path for this change; all of that leads me to think we should target 4.x with this one

That makes sense, targeting 4.x sounds reasonable.

Is there already a plan or timeline for 4.x? I don’t see a 4.x branch yet, but I’d be happy to help.

[brendt]

Is there already a plan or timeline for 4.x? I don’t see a 4.x branch yet, but I’d be happy to help.

There is a branch now :)

We have a milestone for 4.0, but we haven't started with it. I'll mostly be a matter of "tagging when we're done". (https://github.com/tempestphp/tempest-framework/milestone/24)

Realistically, I think we may need to wait until October, because I have some things planned for September where I suspect more people will try out the new worker mode support. If anything weird comes out of that which requires breaking changes, I'd like to include those in 4.0