Fosrestbundle: [BUG] Composer dependency is set improperly.

[yyaremenko]

According to current documentation, the bundle needs a serializer and can either use JMSSerializerBundle or the Symfony Serializer

But if you have a look at the bundle code, it uses
Symfony\Component\Serializer\Normalizer\NormalizerInterface
now and then - thus meaning this bundle REQUIRES the Symfony Serializer

Steps to reproduce

1. Create a new project with a Symfony utility. Along with other bundles, the FrameworkBundle is installed. This bundle has no Symfony Serializer installed by default.
2. Try to install the FOSRestBundle.
3. Get an error saying a serializer is required.
4. Install JMSSerializerBundle. This bundle does not require Symfony Serializer. It only reqires FrameworkBundle, which, in turn, has no Symfony Serializer installed by default

Expectation:
FOSRestBundle is now fully functional

Reality:
FOSRestBundle is broken, because it heavily relies on Symfony\Component\Serializer

[xabbuh]

FOSRestBundle is broken, because it heavily relies on Symfony\Component\Serializer

Can you clarify that part a bit? What errors do you get or what does not work as expected?

[yyaremenko]

I have a form error normalizer

use FOS\RestBundle\Serializer\Normalizer\FormErrorNormalizer as FosRestFormErrorNormalize;

class FormErrorNormalizer extends FosRestFormErrorNormalize

if you have a look at FOS\RestBundle\Serializer\Normalizer\FormErrorNormalizer you'll see

use Symfony\Component\Form\FormInterface;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;

class FormErrorNormalizer implements NormalizerInterface

see, the FOSRestBundle FormErrorNormalizer depends on Symfony\Component\Serializer\Normalizer\NormalizerInterface

so when I run PHPUnit tests which feed a form with deliberately incorrect data, I get:

Symfony\Component\DependencyInjection\Exception\RuntimeException: Class "Symfony\Component\Serializer\Normalizer\NormalizerInterface" not found while loading "App\Serializer\Normalizer\FormErrorNormalizer".

[xabbuh]

Well, this class is precisely meant to be used with the Symfony Serializer component. If you use JMS Serializer instead, you will probably want to use the FormErrorHandler class instead.

[yyaremenko]

okay, then there's an architectural problem here

1) there should be either one single wrapper over final (non-extendable) SymfonySerializer-dependent class and final (non-extendable) JMSSerializerBundle-dependent class, so that the wrapper would give a single 'point of access' to needed functionality to a developer; in the other hand, serializer-dependent classes, being final, will prevent a developer from a temptation of extending them directly,

or, if this is not possible,

2) during the installation the bundle should only install those classes which work with the corresponding (installed) serializer and don't install classes which depend on another (not installed) serializer

[GuilhemN]

there should be either one single wrapper over final (non-extendable) SymfonySerializer-dependent class and final (non-extendable) JMSSerializerBundle-dependent class, so that the wrapper would give a single 'point of access' to needed functionality to a developer

The two serializers work differently, I don't see the benefits of providing an abstraction over their normalizers/handlers, this would be complex (if even possible) for no gain.
If you use the JMS Serializer, just use handlers, if you use the Symfony Serializer, use normalizers. That's how things work in your project and adding a wrapper would even worsen DX in my opinion.

during the installation the bundle should only install those classes which work with the corresponding (installed) serializer and don't install classes which depend on another (not installed) serializer

Composer doesn't allow this. And anyway many libraries provide features that only work with suggested libraries (here it's even just a library dependent implementation).

Honestly I don't see the problem here, this class is not used by the bundle when the Symfony Serializer is not installed, you should do the same in your project.

[xabbuh]

Honestly I don't see the problem here, this class is not used by the bundle when the Symfony Serializer is not installed, you should do the same in your project.

I agree here. Additionally, these classes are tagged as internal so you need to be extra careful as we may change and/or remove them at any time without further notice/deprecation.

[yyaremenko]

but why install code which you will never ever use? does this really sound logically sane to you?

these classes are tagged as internal so you need to be extra careful as we may change and/or remove them at any time without further notice/deprecation

okay, so I'm doing the things in the wrong way then? what is the correct way to alter form errors output in FOSRestBundle? because, according to documentation, I return the invalid form as

// some controller method
return $this->view($form);

and then alter the errors output with a normalizer (handler). is this wrong?

[GuilhemN]

Well, you could do pretty much the same thing we do to extend the FormErrorHandler from the JMS serializer in FOS\RestBundle\Serializer\Normalizer\FormErrorHandler:

• we decorate their service (see https://github.com/FriendsOfSymfony/FOSRestBundle/blob/fc2666edaccac914d1171c7d9f51be85e6f31c48/DependencyInjection/Compiler/JMSFormErrorHandlerPass.php#L35-L37)
• and call it from FOS\RestBundle\Serializer\Normalizer\FormErrorHandler to alter its output.

[xabbuh]

but why install code which you will never ever use? does this really sound logically sane to you?

The only way around that would be to split the bundle into multiple packages which has two major drawbacks: We will have to maintain more packages which means more work and makes it less likely to keep everything stable and the drawback for users is that they need to be more careful when deciding what to install (and you need to discover what packages you have to install).

[xabbuh]

Having said that I don't think there is anything to be fixed on the code side. So I am going to close here. Thank you for understanding.

[yyaremenko]

@GuilhemN okay, thank you so much for help, I'll give that a try

@GuilhemN @xabbuh well, if nothing is possible / nothing makes sense to fix the situation, you could at least make classes final to prevent developers from extending them; also, a quick look over classes which use Symfony\Component\Serializer shows very few of them actually have @internal PHPDoc

[xabbuh]

Can you point out some classes that from your POV could deserve being marked as internal but are not yet?

[yyaremenko]

well, I can't really say my personal opinion here, as I am just a user of the bundle, not its developer, so I don't know the bundle internals and can't really know what should be closed (restricted for the extension);

I just refer to your comment

Additionally, these classes are tagged as internal so you need to be extra careful as we may change and/or remove them at any time without further notice/deprecation.

so there are particular classes you, as a developer, are aware of, which must not be accessible to a developer for extension, because they are only for internal use and thus are a subject to change / removal without any notice;

but as I mentioned, the majority of classes relying on a specific serializer (well, at least those relying on Symfony\Component\Serializer) have no @internal PHPDoc; the very first examples that come to my mind are: FOS\RestBundle\Serializer\Normalizer\FormErrorNormalizer and FOS\RestBundle\Serializer\NormalizerFormErrorHandler - but I believe this is not exhaustive list of classes which should be final

[yyaremenko]

probably, you could additionally point that out in the bundle documentation - smth like 'don't extend the bundle classes, extend corresponding serializer classes instead'

[xabbuh]

Well, but both classes are tagged as internal (see https://github.com/FriendsOfSymfony/FOSRestBundle/blob/2.x/Serializer/Normalizer/FormErrorHandler.php#L26 and https://github.com/FriendsOfSymfony/FOSRestBundle/blob/2.x/Serializer/Normalizer/FormErrorNormalizer.php#L22).

If we were to add these classes today, we would probably just make them final. But as it stands now there is not much value to change that and thus forcing work on users who extend these classes.

[yyaremenko]

that's weird, I should probably update the bundle then, 'cuz on my side these PHPDocs are missing

I surely can't force or persuade you to do something in either way, but I believe you agree that if these classes are a subject to a sudden modification / removal without any warning - the developers will still be forced to do a work once you modify / remove those classes

if you believe nothing should be changed on the code side, at least, you could consider updating the documentation

[xabbuh]

Which version of the bundle do you use?

[yyaremenko]

2.7.3

[xabbuh]

We indeed added these annotations in 2.8.

[yyaremenko]

okay, I'll just update the bundle then, thank you;

please also consider thinking about making these classes final - if not now, then in some nearest future (maybe in some v3.0 or such)

[GuilhemN]

I don't think documenting internal classes is worth it, it would increase the work to maintain this bundle, it would make the docs less readable and only might be usefull to a small part of our users in my opinion.

These classes are marked as @internal, if someone finds this class, I believe they can also find the @internal annotation. (And even if not, the symfony debug component will trigger a deprecation when extending such a class).

About making them final, @internal is more restrictive so no need imo.

[yyaremenko]

okay, I see your point. thank you guys for being responsive.