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

[Serializer] Custom Normalizer broken after upgrading to 6.1 #1252

Closed
alexchuin opened this issue May 27, 2022 · 60 comments
Closed

[Serializer] Custom Normalizer broken after upgrading to 6.1 #1252

alexchuin opened this issue May 27, 2022 · 60 comments
Labels
Bug Bug Fix Status: Needs Review Needs to be reviewed

Comments

@alexchuin
Copy link

Symfony version(s) affected

6.1

Description

My custom Normalizer is broken since i've upgraded to Symfony 6.1.
This problem seems to be related to the new Serializer Profiler.

Maybe I forgot something?

Thanks,
Alex

How to reproduce

Just use this example :

https://symfony.com/doc/6.1/serializer/custom_normalizer.html

Possible Solution

No response

Additional Context

FileNormalizer::__construct(): Argument symfony/symfony#2 ($normalizer) must be of type Symfony\\Component\\Serializer\\Normalizer\\ObjectNormalizer, Symfony\\Component\\Serializer\\Debug\\TraceableNormalizer given

@alexchuin alexchuin added the Bug Bug Fix label May 27, 2022
@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label May 27, 2022
@alexchuin alexchuin changed the title Custom Normalizer broken after updating to 6.1 Custom Normalizer broken after upgrading to 6.1 May 27, 2022
@alexchuin alexchuin changed the title Custom Normalizer broken after upgrading to 6.1 [Serializer] Custom Normalizer broken after upgrading to 6.1 May 27, 2022
@xabbuh
Copy link
Member

xabbuh commented May 27, 2022

Does the issue go away when you change the type-hint of your constructor argument from ObjectNormalizer to NormalizerInterface and does your normalizer still work as intended?

@alexchuin
Copy link
Author

alexchuin commented May 27, 2022

Nope, already tried and got a Circular reference detected :

Circular reference detected for service "debug.App\Api\Normalizer\FileNorma !! lizer", path: "debug.App\Api\Normalizer\FileNormalizer -> debug.serializer !! -> debug.serializer.inner -> debug.App\Api\Normalizer\FileNormalizer".

Because ContextAwareNormalizerInterface is deprecated since 6.1 :
@deprecated since symfony/serializer 6.1, use NormalizerInterface instead

Thanks!

@xabbuh
Copy link
Member

xabbuh commented May 27, 2022

I changed my suggestion for the update of the documentation a bit. Can you try out the new version in symfony/symfony-docs#16826 please?

@derrabus
Copy link
Member

I think that the example from the docs is correct and should continue to work after an upgrade. We should treat this as a bug.

The serializer topic aside: If I autowire a constructor parameter with a Foo type, I find it astonishing that I receive a service that is not a Foo.

@alexchuin
Copy link
Author

Yes, thanks all but I just downgraded to 6.0 until it will be fixed :)

@darthf1
Copy link

darthf1 commented May 27, 2022

Similar (?) issue. On 6.0, the following service declaration works:
services.yaml

  Infrastructure\Shared\ApiPlatform\Serializer\JsonLdNormalizer:
    decorates: 'api_platform.jsonld.normalizer.item'
use ApiPlatform\Core\Api\IriConverterInterface;
use ApiPlatform\Core\JsonLd\Serializer\ItemNormalizer;
use Symfony\Component\Serializer\Normalizer\CacheableSupportsMethodInterface;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

final class JsonLdNormalizer implements NormalizerInterface, DenormalizerInterface, SerializerAwareInterface, CacheableSupportsMethodInterface
{
    public function __construct(
        private readonly IriConverterInterface $iriConverter,
        private readonly ItemNormalizer $decorated,
    ) {
    }

    ......
}

On 6.1 I suddenly get the error:

Cannot autowire service "Infrastructure\Shared\ApiPlatform\Serializer\JsonLdNormalizer": argument "$decorated" of method "__construct()" references class "ApiPlatform\Core\JsonLd\Serializer\ItemNormalizer" but no such   
  service exists. Try changing the type-hint to one of its parents: interface "Symfony\Component\Serializer\Normalizer\DenormalizerInterface", or interface "Symfony\Component\Serializer\Normalizer\NormalizerInterface". 

If I change my service declaration:

  Infrastructure\Shared\ApiPlatform\Serializer\JsonLdNormalizer:
    decorates: 'api_platform.jsonld.normalizer.item'
    arguments:
      $decorated: '@.inner'

The error message changes:

Infrastructure\Shared\ApiPlatform\Serializer\JsonLdNormalizer::__construct(): Argument symfony/symfony#2 ($decorated) must be of type ApiPlatform\Core\JsonLd\Serializer\ItemNormalizer,  
     Symfony\Component\Serializer\Debug\TraceableNormalizer given

@camilleterol
Copy link

I am having the same issue. Let me know if you need any help reproducing this one.

It happens when a Custom Normalizer is autowired with the ObjectNormalizer service.

@stof
Copy link
Member

stof commented May 27, 2022

Well, I'm not sure the normalizer services were actually meant to be autowirable based on their class name. Supporting that means that we don't support decorating them (and so that we cannot apply the traceable decorators on them).

To me, the autowirable names in core should only about interfaces for that reason.

@derrabus
Copy link
Member

I think, the use case described in the linked piece of documentation is a valid one: We want a normalizer that behaves like ObjectNormalizer mostly, but wee need to change a thing or two in the resulting array.

This is why our custom normalizer requests exactly that implementation of a normalizer. And our custom normalizer may receive an undecorated ObjectNormalizer, that's fine. I doubt that we need that level of traceability.

@danielburger1337
Copy link

A possible workaround, that would work with the new TraceableNormalizer, is to use the new Autowire attribute:

class CustomNormalizer implements NormalizerInterface, CacheableSupportsMethodInterface
{
    public function __construct(
        #[Autowire(service: ObjectNormalizer::class)]
        private NormalizerInterface $normalizer
    ) {
    }
}

But I agree with @derrabus that it is somewhat crazy, that symfony injects an object with an incompatiable type.

I'm not to familiar with how symfonys dependency injection works internally, but is the "symfony/proxy-manager-bridge" package not able to solve this problem? A "lazy" service will be replaced by a proxy service, so wouldn't it be possible to create a proxy object that has the TraceableNormalizer functionality? Currently the service definition gets replaced by a decorator.

The library the bridge is based on, Ocarmius/ProxyManager, has the Access Interceptor Value Holder Proxy concept. This should be able to solve the problem.
Bascially I'm suggesting that the service is not replaced by "TraceableNormalizer" (this applies for all traceable decorators, like TraceableAuthenticator), but by a proxy object.

@aniolekx
Copy link

this "possible workaround" does not work for me :/ (Circular reference detected)

@shouze
Copy link

shouze commented May 30, 2022

@danielburger1337 @aniolekx I confirm that workaround with Autowire works perfectly here, thx!

@shouze
Copy link

shouze commented May 30, 2022

EDIT: still have the issue in local dev with profiler enabled in fact...

@aniolekx
Copy link

EDIT: still have the issue in local dev with profiler enabled in fact...

Do you import that Autowire class, I had it not imported and this was leading to "Circular reference detected" error. After import seems tat it works in dev env

@shouze
Copy link

shouze commented May 30, 2022

Yes I import the class, but I have another issue after. If you decorate a Normalizer that strictly implement the Normalize interface it's OK. But for Example we decorate the pagination Normalizer of Api Platform... that have some abstract methods like getPaginationData... and then in local dev I have a crash saying that TraceableNormalizer don't have any getPaginationData method 😭

@danielburger1337
Copy link

But for Example we decorate the pagination Normalizer of Api Platform... that have some abstract methods like getPaginationData... and then in local dev I have a crash saying that TraceableNormalizer don't have any getPaginationData method 😭

That is a good point. This could be easily fixed by adding php's magic __call method to the TraceableNormalizer implementation. As far as I can see, exactly this was already implemented for TreaceableAuthenticator, TraceableEventDispatcher, LoggingTranslator etc..

I can submit a PR incase this function wasn't omitted on purpose?

@chalasr
Copy link
Member

chalasr commented May 30, 2022

PR welcome for the missing __call() method.

@chalasr
Copy link
Member

chalasr commented May 30, 2022

I think, the use case described in the linked piece of documentation is a valid one

While the use case is valid, the way it is implemented seems wrong to me given how the serializer component is designed, as stof explained: NormalizerInterface' concrete implementations are not meant to be referenced by something else than Serializer itself. To me, either it should be extended using inheritance, or we are missing some proper extension point here.

That doesn't mean we shouldn't try to fix this regression though.

@derrabus
Copy link
Member

NormalizerInterface concrete implementations are not meant to be referenced by something else than Serializer itself.

Not sure about that. In the documented case, the developer does not just want any normalizer implementation. They explicitly want the behavior of ObjectNormalizer because it solves 95% of their problem already. The alternatives are either to extend ObjectNormalizer or to work with NormalizerAwareInterface and work with priorities and flags that are pushed to the context.

The former is a technique which we should discourage imho. And the latter feels a bit too complicated for what should be achieved here.

derrabus referenced this issue in symfony/symfony May 30, 2022
…nd TraceableSerializer (danielburger1337)

This PR was squashed before being merged into the 6.1 branch.

Discussion
----------

[Serializer] Added missing __call to TraceableNormalizer and TraceableSerializer

| Q             | A
| ------------- | ---
| Branch?       | 6.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |  #46471
| License       | MIT
| Doc PR        | none

Added missing `__call` method to the new TraceableNormalizer and TraceableSerializer.
This method was forgotten and breaks custom normalizers that have public methods outside of the NormalizerInterface.

This approach is already being used for TreaceableAuthenticator, TraceableEventDispatcher, LoggingTranslator etc...

Commits
-------

74f3c37 [Serializer] Added missing __call to TraceableNormalizer and TraceableSerializer
@ivoba
Copy link

ivoba commented Dec 13, 2022

@lyrixx your link does not link a comment :)

I followed this: #1252 (comment) by turning off autowire and inject the ObjectNormalizer(?) explicitly into the custom denormalizer:

App\Serializer\CustomDenormalizer:
    autowire: false
    arguments:
        $denormalizer: '@serializer.normalizer.object

So i suppose current documentation is wrong?

@lyrixx
Copy link
Member

lyrixx commented Dec 13, 2022

@ivoba the issue has been moved. I think GitHub has rewritten all comments and has lost this information :( The link was https://github.com/symfony/symfony/issues/46471#issuecomment-1140350673

@kissifrot
Copy link

Well disabling autowiring can be quite cumbersome if there are many other services injected in the custom normalizer, is there a workaround for for workaround? 😛

@mtarld
Copy link
Contributor

mtarld commented Dec 14, 2022

I think you still can let autowiring enabled and override only the denormalizer argument

@lyrixx
Copy link
Member

lyrixx commented Dec 15, 2022

This works well
#1252 (comment)

Or use the NormalizeAwareInterface

@ivoba
Copy link

ivoba commented Dec 15, 2022

@lyrixx i think that NormalizeAwareInterface is not working is the actual problem.
Seems that the autowiring is broken with a circular reference problem.

This is why the most comments here recommed to disable autowire as a solution.

@lyrixx
Copy link
Member

lyrixx commented Dec 15, 2022

For me it's was OK, since it's a setter.

@aiibileme
Copy link

aiibileme commented Jan 4, 2023

When ı added a comment at collect_serializer_data on web_profiler.yaml, it worked.

framework:
    profiler:
        only_exceptions: false
        collect_serializer_data: false

@gansky-alexander
Copy link

Hello, all !

I tried all workarounds and them are not working for me.
Do you have any information about this bug when are you going to fix it ?

What issues i faced:

  • circular reference on service registration
  • endless loading of the controller

@mtarld
Copy link
Contributor

mtarld commented Jan 19, 2023

Actually, with a fresh Symfony instance, I managed to make it work.

You can leverage the NormalizerAwareInterface like the following:

final class FoobarNormalizer implements NormalizerInterface, NormalizerAwareInterface
{
    use NormalizerAwareTrait;
}
services:
    App\Serializer\Normalizer\FoobarNormalizer:
        calls:
            - setNormalizer: ['@serializer.normalizer.object']

Or using a constructor injection like the following:

final class FoobarNormalizer implements NormalizerInterface
{
    public function __construct(
        private NormalizerInterface $normalizer,
    ) {
    }
}
services:
    App\Serializer\Normalizer\FoobarNormalizer:
        arguments:
            $normalizer: '@serializer.normalizer.object'

In either way, you must never use a concrete implementation of any normalizer/encoder in the code, you should inject the interface and configure your container to inject the proper instance. Therefore, IMHO the maker bundle should be updated accordingly.

And if you don't want to specify which normalizer instance to use because you don't care, be aware that the constructor injection won't work because of a circular reference exception (with and without the debug), therefore my advice is to always use the NormalizerAwareInterface.

@gansky-alexander
Copy link

App\Serializer\Normalizer\FoobarNormalizer:
        calls:
            - setNormalizer: ['@serializer.normalizer.object']

Got it, thanks !

I tried this approach. You may see my example below:

Symfony 6.2.5 and all related bundles.

services.yaml

    App\Serializer\Normalizer\OrderNormalizer:
        calls:
            -   setNormalizer: ['@serializer.normalizer.object']

Normalizer itself

<?php

namespace App\Serializer\Normalizer;

use App\Entity\Order;
use Symfony\Component\HttpFoundation\RequestStack;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerAwareTrait;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class OrderNormalizer implements NormalizerInterface, NormalizerAwareInterface
{
    use NormalizerAwareTrait;

    private $requestStack;

    public function __construct(
        RequestStack $requestStack
    ) {
        $this->requestStack = $requestStack;
    }

    /**
     * @param Order $topic
     * @param $format
     * @param array $context
     *
     * @return mixed
     */
    public function normalize($topic, $format = null, array $context = [])
    {
        $data = $this->normalize($topic, $format, $context);

        $data['extra_field'] = 'extra data';

        return $data;
    }

    public function supportsNormalization($data, $format = null)
    {
        return $data instanceof Order;
    }
}

When i export orders i use next in controller

        $query = $orderRepository->getQuery();
        $page = $request->query->getInt('page', self::DEFAULT_PAGE);
        $pageSize = $request->query->getInt('pageSize', self::DEFAULT_PAGE_SIZE);

        try {
            $result = $this->paginator->paginate(
                $query,
                $page,
                $pageSize <= self::MAX_PAGE_SIZE ? $pageSize : self::MAX_PAGE_SIZE,
                ['wrap-queries' => true]
            );
        } catch (LogicException $exception) {
            return $this->json(['message' => $exception->getMessage(), 'code' => Response::HTTP_BAD_REQUEST]);
        }

        return $this->json(
            [
                'result' => $result,
                'pagination' => [
                    'page_size' => $pageSize,
                    'page' => $page,
                    'total' => $result->getTotalItemCount(),
                    'pages_count' => ceil($result->getTotalItemCount() / $pageSize),
                ],
            ],
            Response::HTTP_OK,
            [],
            [
                'groups' => $groups,
                AbstractObjectNormalizer::ENABLE_MAX_DEPTH => true,
            ]
        );

Result:
502 Bad Gateway.
If i remove custom normalizer i gets correct data but obviously without 'extra_field'. How can i manage this ?

Looks like ObjectNormalizer injects all normalizers even custom and when i tries to serialize any object ObjectNormalizer went to infinity loop with that.

@danielburger1337
Copy link

public function normalize($topic, $format = null, array $context = [])
{
    $data = $this->normalize($topic, $format, $context);

    $data['extra_field'] = 'extra data';

    return $data;
}

Infinite reccursion because you call $this->normalize instead of $this->normalizer->normalize

@gansky-alexander
Copy link

gansky-alexander commented Jan 31, 2023

final

Thank you, missed this !
But infinite reccursion is still here, if i make next all works

public function supportsNormalization($data, $format = null)
{
    return false;
}

if i put next

public function supportsNormalization($data, $format = null)
{
    return $data instanceof Order;
}

i see 502 error message

@gansky-alexander
Copy link

I found a solution.

  1. Inject object normalizer by myself
App\Serializer\Normalizer\OrderNormalizer:
    calls:
        -   setBaseNormalizer: ['@serializer.normalizer.object']
            
    public function setBaseNormalizer($normalizer)
    {
        $this->normalizer = $normalizer;
    }

@teol
Copy link

teol commented May 15, 2023

Any update on this problem? Looks like it hasn't been fixed yet

@mdeboer
Copy link

mdeboer commented Jun 2, 2023

It hasn't been fixed in the sense that there is no fix to be made in the code imho. The docs need to be updated to explain about this issue and how to properly wire up your custom normalizer.

Here is an example on how I do it:

<?php

namespace App\Serializer\Normalizer;

use Symfony\Component\DependencyInjection\Attribute\Autowire;
use Symfony\Component\Serializer\Normalizer\DenormalizerInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

readonly class MyCustomNormalizer implements DenormalizerInterface
{
    private NormalizerInterface&DenormalizerInterface $normalizer;

    public function __construct(
        #[Autowire(service: 'serializer.normalizer.object')] NormalizerInterface&DenormalizerInterface $normalizer,
    ) {
        $this->normalizer = $normalizer;
    }
}

Obviously you can replace the serializer.normalizer.object service with any normalizer you like.

Now this example is using Symfony 6.3 with PHP 8.2, but the same principles apply for older versions although you'd need to make some modifications like replace the Autowire attribute etc.

@mdeboer
Copy link

mdeboer commented Jun 23, 2023

I just found out that there is proper documentation for this now:

https://symfony.com/doc/current/serializer/custom_normalizer.html#creating-a-new-normalizer

If this isn't related to the maker bundle anymore, I reckon this ticket can be closed?

@milabron
Copy link

milabron commented Aug 5, 2023

I just found out that there is proper documentation for this now:

https://symfony.com/doc/current/serializer/custom_normalizer.html#creating-a-new-normalizer

If this isn't related to the maker bundle anymore, I reckon this ticket can be closed?

The documentation is not updated. Using the information in the documentation I get deprecated messages and when I try to test the method described in the message I get circular reference, leaving docker unusable.

@mtarld
Copy link
Contributor

mtarld commented Aug 22, 2023

I just found out that there is proper documentation for this now:

https://symfony.com/doc/current/serializer/custom_normalizer.html#creating-a-new-normalizer

If this isn't related to the maker bundle anymore, I reckon this ticket can be closed?

I think this PR still needs to be merged: #1273

@mdeboer
Copy link

mdeboer commented Aug 24, 2023

I just found out that there is proper documentation for this now:
https://symfony.com/doc/current/serializer/custom_normalizer.html#creating-a-new-normalizer
If this isn't related to the maker bundle anymore, I reckon this ticket can be closed?

I think this PR still needs to be merged: #1273

I don't think that is related as that proposes a whole new redesign of the Serializer component.

Offtopic: I do think that sounds interesting though but would need to dig into the details.

OskarStark added a commit to symfony/symfony-docs that referenced this issue Oct 10, 2023
…tNormalizer` (mtarld)

This PR was merged into the 6.3 branch.

Discussion
----------

[Serializer] Use `NormalizerInterface` instead of `ObjectNormalizer`

As mentioned in symfony/maker-bundle#1252 (comment), the documentation is telling to use a concrete implementation of the `NormalizerInteface`.
This is not the best in terms of OOP, and moreover doesn't work since Symfony 6.1 and the introduction of `TraceableNormalizer`.

Commits
-------

3e9af1b [Serializer] Use NormalizerInterface instead of ObjectNormalizer
@chalasr
Copy link
Member

chalasr commented Feb 22, 2024

Now that #1273 has been merged, I think we can close here.

@chalasr chalasr closed this as completed Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bug Fix Status: Needs Review Needs to be reviewed
Projects
None yet
Development

No branches or pull requests