[Serializer] added the ability to pass \ArrayObject to context.callbacks

[mlavrinenko]

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	-
License	MIT
Doc PR	symfony/symfony-docs#12135

Callbacks system isn't much flexible as sometimes needed, so I decided to allow using ArrayObject here and now it's possible to add more flexible callback reactions since \ArrayObject can be extended and return callbacks dynamically

[OskarStark]

If I am right, this new feature should go in 4.4 branch, same for: symfony/symfony-docs#12135

[fabpot]

Yes, new feature must be for the 4.4 branch. @mlavrinenko Can you change the target branch?

[mlavrinenko]

Oh, it seems I misunderstood something. The documentation says that I must pick "master, if you are adding a new feature", except "when a new major Symfony version comes out" (https://symfony.com/doc/current/contributing/code/pull_requests.html#choose-the-right-branch), but it wasn't clear what means "comes out".

Anyway, I've added fixes to 4.4 branch and changed the PR, hope it's fine now.

I see the coding standard patch suggestion, but it's not relevant to my fixes. Should I apply it?

[dunglas]

This is a BC break. And I'm not sure it is worth it to change anyway because this method is deprecated.

[mlavrinenko]

Tests fall without this change because of the new case "Callbacks as ArrayObject".

Should I rewrite tests of this method to exclude this case from it?
Or is it better to do the type checking in the body of the method (is_array($callbacks) || $callbacks instanceof \ArrayObject) to prevent any illegal usage?

[dunglas]

I'm not very fond of this change. Callbacks are legacy, using custom normalizers (that can decorate existing ones) is the way to go now, especially when more flexibility is needed.

[mlavrinenko]

I'm not very fond of this change. Callbacks are legacy, using custom normalizers (that can decorate existing ones) is the way to go now, especially when more flexibility is needed.

I agree that normalizers have better design than callbacks and should be used in general, but they work in a very different manner than callbacks. With normalizer we work with data, but we don't have context about its parent. Imagine if you want to serialize an object of type A in a different manner when it's a property of type B. For example:

$admin = new User(1, 'Admin');
$anonymousGroup = new Group(1, 'Anonymous Group', $admin);
$anonymousMember = new User(2, 'Anonymous Member', $anonymousGroup, $admin);

echo $serializer->serialize($anonymousMember, 'json');

/* default (ObjectNormalizer)
{
  "id": 2,
  "name": "Anonymous Member",
  "group": {
    "id": 1,
    "name": "Anonymous Group",
    "manager": {
      "id": 1,
      "name": "Admin",
      "group": null,
      "manager": null
    }
  },
  "manager": {
    "id": 1,
    "name": "Admin",
    "group": null,
    "manager": null
  }
}
*/

/* needed
{
  "id": 2,
  "name": "Anonymous Member",
  "group": {
    "id": 1,
    "name": "Anonymous Group",
    "manager": {
      "id": 1
    }
  },
  "manager": {
    "id": 1,
    "name": "Admin",
    "group": null,
    "manager": null
  }
}
*/

class User
{
    /** @var int */
    public $id;

    /** @var string */
    public $name;

    /** @var User */
    public $group;

    /** @var User */
    public $manager;

    public function __construct(int $id, string $name, Group $group = null, User $manager = null)
    {
        $this->id = $id;
        $this->name = $name;
        $this->group = $group;
        $this->manager = $manager;
    }
}

class Group
{
    /** @var int */
    public $id;

    /** @var string */
    public $name;

    /** @var User */
    public $manager;

    public function __construct(int $id, string $name, User $manager = null)
    {
        $this->id = $id;
        $this->name = $name;
        $this->manager = $manager;
    }
}

with callback you can do it with ease:

           'callbacks' => [
               'manager' => function ($data, $parent, $attributeName, $format, $context) use ($serializer) {
                   if ($data instanceof User && $parent instanceof Group) {
                       return ['id' => $data->id];
                   }

                   return $serializer->normalize($data, $format, $context);
               }
           ],

but what should I do if I want to implement such logic for every inner User regardless of property name? Without PR change I can pass such callback for every property name which may hold needed object. But it isn't convenient at all. With the PR change you can extend ArrayObject and do whatever you want in just a single class. Normalizers can't grant such flexibility.

Or should I rework normalizers interface to give them their parent context instead of this change?

[fabpot]

Let's close this one as it won't be merge as is (comment from @dunglas). Regarding extending/enhancing normalizers, let's have the discussion in a new issue.