Change to optional Doctrine/Annotations package introduces a breaking change

[Levivb]

Q	A
Bug report?	yes
Feature request?	no
BC Break report?	yes
RFC?	no

Steps required to reproduce the problem

In c7e6eb1 the doctrine/annotations package is removed from the require section. This change was released in tag 3.30.0 (3.29.1...3.30.0)

In our case a composer update updates jms/serializer and removes doctrine/annotations from the installed packages and as a result, the de/serializer broke down completely. We're using annotations quite a lot for this serializer package.

For example:

    /**
     * @var string
     * @JMS\Type("string")
     * @JMS\SerializedName("id")
     */
    protected $id;

But without the doctrine/annotations, the annotations are suddenly ignored.
I presume this is because of a migration to PHP Attributes? - although I can't find any proof of that in any changes here (CHANGELOG.md. UPGRADING.md have long not been touched)

Since this change was added in a minor release, it was quite unexpectedly breaking our builds.

Our serializer is initialized as following (Laravel framework):

    use JMS\Serializer\Expression\ExpressionEvaluator;
    use JMS\Serializer\Handler\HandlerRegistryInterface;
    use JMS\Serializer\SerializerBuilder;
    use JMS\Serializer\SerializerInterface;

    private function buildSerializerWithEvaluators(): SerializerInterface
    {
        /** @var ConfigRepository $configRepository */
        $configRepository = $this->app->make(ConfigRepository::class);

        $handlerRegistry = $this->app->make(HandlerRegistryInterface::class);
        $serializerBuilder = SerializerBuilder::create($handlerRegistry)
            ->setDebug((bool)$configRepository->get('app.debug'))
            ->addDefaultHandlers()
            ->addDefaultListeners()
            ->setExpressionEvaluator(new ExpressionEvaluator(new ExpressionLanguage()));

        return $serializerBuilder->build();
    }

Expected Result

Deserialization works as in tag 3.29.1

Actual Result

JMS \ Serializer \ Exception \ RuntimeException
Message: You must define a type for Modules\Media\Models\File::$id.

[fredericgboutin-yapla]

Change to optional Doctrine/Annotations package introduces a breaking change

Yes it does, here is the original thread were they discuss the matter - #1471

It broke in production on a live environment and we had to figure all this 💩 out quick - see https://jmsyst.com/libs/serializer/master/configuration#configuring-metadata-locations

So we ended up with explicitly adding the original requirement - aka "doctrine/annotations": "^1.13 || ^2.0", - because we are on PHP 7.4 and we don't provide metadata in XML or YAML files.

It should have been a major OR at least not as that disruptive somehow. That was a bold move.

[Levivb]

Yea, that's what I did in order to fix it on our side.

For future readers of this issue which are still using the Doctrine Annotation for the serializer:

I'de suggest requiring doctrine/annotations explicitly yourself in your composer.json. That way, the JMS Serializer will automatically pick up the AnnotationReader as previously was the case.

[fredericgboutin-yapla]

For future readers of this issue which are still using the Doctrine Annotation for the serializer:

Yeah, the problem is, as a user of jms/serializer you don't necessarily realize you are using Doctrine Annotation. And this is quite normal TBH 🤷 When I pull a dependency over composer I cannot review and be aware of every single dependency of dependency I'm using.

This kind of situation happens and is usually handled in a way that you have "optional" dependencies that are declared by the library. And yes, at that moment you can go and read the doc about it to realize "oh, if I pull X then the component will use it". But every time you have those optional dependencies... well... they are optional! So the library works without them. And this is exactly what broke here.

Basically, Doctrine Annotation is NOT an optional dependency on PHP 7.4 without explicit metadata.

[mbabker]

Basically, Doctrine Annotation is NOT an optional dependency on PHP 7.4 without explicit metadata.

Doc block-based annotations ARE a form of explicit metadata. Except that metadata is declared on your class objects and not in a separate mapping file, and parsing that metadata relies on the doctrine/annotations package. The need to install that package may very well be hidden from your applications in one way or another, but that dependency exists. Until this last release, that package was always arbitrarily installed, and that's no longer the case given the intent to deprecate the Annotations library in favor of PHP 8's native attributes.

Yes, that change broke applications that relied on transient dependencies to configure their environments properly, but short of making a B/C promise about the composer.json file (and the discussions that would require for anyone in the PHP ecosystem making that promise, such as does adding a new package to the require section create a B/C break, or what about raising a minimum package requirement through raised version constraints on the require section or the introduction of a conflict), there's no sane way of getting around the potential of a change in that manifest breaking someone's application or preventing an upgrade.

[scyzoryck]

Docs updated. ;)

[fredericgboutin-yapla]

This is BS. You should rollback all this and release a 3.30.1 version to patch the regression. And then you should find a proper way of removing Doctrine Annotations.

[kurakin-oleksandr]

@fredericgboutin-yapla indeed. This is breaking changes and it should be handled accordingly

[mbabker]

I'm now curious. How many folks following this issue have been bitten by the actual B/C break in the 3.30 release, the signature change in the JMS\Serializer\Builder\DriverFactoryInterface::createDriver() method (which I have admitted could have been handled differently if we really wanted to figure something out)? And how many folks are here because their applications did not directly require the doctrine/annotations package in their own composer.json file and were bitten by the removal of a transient dependency their applications relied on?

To my knowledge, nobody has a B/C promise regarding the dependencies in their composer.json files, and I personally find it hard to think of a way to craft one. What constitutes a B/C break in the Composer manifest anyway? A raised PHP version? A raised minimum dependency on a third-party package? Adding new required packages? Removing previously required packages? Adding support for a new major version release, which may cause a downstream application's dependencies to make a major version upgrade without explicit request?

Look, I'm sorry applications broke because upgrading jms/serializer caused doctrine/annotations to be removed. But the only way for this to have happened would be a combination of no other package in your dependency tree requiring it AND your application not requiring this direct dependency. And I personally don't think it's healthy for the ecosystem as a whole to rely on transient dependencies to keep their applications functional, so requiring doctrine/annotations in your applications gives you full control over what version is installed, when to upgrade (for the folks still running on its 1.x branch and not using 2.0), and when to remove it instead of relying on a package like this one to tell Composer your application needs annotation support.

[Levivb]

For me it was the removal of doctrine/annotations. But not that big of a deal. The cause was quickly found and the fix even easier (with a sidenote*).

Removing a dependency is imo no BC break since like you state, there is no promise and going down that route would only result in everything needing a new major release.

There is however one big gotcha.
This package has always used doctrine/annotations to scan docblocks as a way of being instructed on how to de/serialize classes/json. So obviously users of this repository would (indirectly) use doctrine/annotations. By removing doctrine/annotations and switching over to PHP Attributes the full syntax on how to use this repository changed completely.

*(the sidenote) Right now this package still has a soft dependency to doctrine/annotations in which it will use php docblocks (through doctrine/annotations) if they are used. But imagine taking the removal of a package as no breaking change philosophy one step further: remove the doctrine/annotations dependency completely and immediately switch over to PHP Attributes in a minor version...
That would cause mayhem for all the users of jms/serializer since a minor upgrade would require everyone to switch to PHP Attributes immediately (that's what major releases are for).

Now, I could be misunderstanding the upgrade to PHP Attributes requirement here - **second sidenote up head on that.

Another big difference here versus the usage of a lot of other transient dependencies is that the doctrine/annotations dependency itself is hidden in php docblocks (effectively: code comments).

We are using https://github.com/maglnet/ComposerRequireChecker in our pipeline so using classes from a package without requiring it is already detected and reported in our pipeline. Docblocks, however, are not real php code, so that check never took a @JMS\Type reference into account. But that's more on us than it is on any third party package (like this one) we are using.
(Note that PhpStorm will even remove the use statements when running cleanup code when not using the https://plugins.jetbrains.com/plugin/7320-php-annotations plugin)

(**sidenote 2) In regards to improvements, the communication could be a bit better regarding upgrade guide/changelog changes. But that's not really a point specifically for this package, but for the community overal since there are few repositories maintaining an up to date changelog/upgrade guide (even for major releases).

Right now I could switch over from php docblocks to PHP Attributes. But since there's no upgrade guide here... I don't know what to do without quite some investigation and debugging.

Just my two cents :)
***Not mad, just wanted to point out that the usage on how to instruct the jms/serializer changed quite drastically in a minor change (or so it seems)

[mbabker]

*(the sidenote) Right now this package still has a soft dependency to doctrine/annotations in which it will use php docblocks (through doctrine/annotations) if they are used. But imagine taking the removal of a package as no breaking change philosophy one step further: remove the doctrine/annotations dependency completely and immediately switch over to PHP Attributes in a minor version...

That wouldn't have happened. The entire push behind moving doctrine/annotations out of the require section is a combination of PHP itself having a native metadata API (attributes in PHP 8) and Doctrine intending to sunset support for that package and therefore needing to remove the hard dependency on it (which really looks to have only existed because the builder API, which the Symfony bundle does not use at all, had a non-nullable annotations reader argument). Annotations support in this package won't go away in full without a 4.0 release, I don't think anyone is crazy enough to make that move. The only change for now is the annotations library isn't being automatically installed when this package is installed (which is the same as how the XML and YAML drivers are handled, both of those have extra requirements (SimpleXML support or symfony/yaml which aren't hard required); unfortunately that turned out to be more disruptive for some folks than intended.