Skip to content

(5.4) add compatibility layer for doctrine/annotations v1 and v2 #48912

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

Conversation

alexislefebvre
Copy link
Contributor

@alexislefebvre alexislefebvre commented Jan 8, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #48792
License MIT
Doc PR no

The previous code always registered annotations.dummy_registry and unregistered it with doctrine/annotations v1.

This has the unexpected effect to break the call to addGlobalIgnoredName with doctrine/annotations v2 : #48792 (comment)

I propose a novel approach with a workaround that:

These changes can be tested with this reproducer: https://github.com/alexislefebvre/symfony_bug_app_48792

Downgrading and upgrading become possible:

composer require doctrine/annotations:~1.13.0 --with-all-dependencies
composer require doctrine/annotations:^1.13.0 --with-all-dependencies
composer require doctrine/annotations:^2.0 --with-all-dependencies

It looks like services declaration was meant to be as simple as possible, and logic went into FrameworkExtension, I think it made things more complicated in this case, with the service annotations.dummy_registry being declared and unregistered just after. So I took a simpler approach.

@derrabus
Copy link
Member

derrabus commented Jan 8, 2023

Wouldn't it be enough to change this line…

service('annotations.dummy_registry')->ignoreOnInvalid(), // dummy arg to register class_exists as annotation loader only when required

to use ->nullOnInvalid() instead? Looks like I broke that with #48718.

@alexislefebvre
Copy link
Contributor Author

alexislefebvre commented Jan 8, 2023

Wow, it works! 😮

I was confused by ignoreOnInvalid, it thought it ignored passing the annotations.dummy_registry service but actually it looks like it disabled the call to addGlobalIgnoredName.

@alexislefebvre alexislefebvre deleted the 5.4-add-compatibility-layer-for-doctrine/annotations branch January 8, 2023 13:06
nicolas-grekas added a commit that referenced this pull request Jan 9, 2023
…exislefebvre)

This PR was merged into the 5.4 branch.

Discussion
----------

[FrameworkBundle] restore call to addGlobalIgnoredName

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #48792 and replace #48912
| License       | MIT
| Doc PR        | no

The service `annotations.dummy_registry` is unregistered with `doctrine/annotations` v2.

This had the unexpected effect to break the call to `addGlobalIgnoredName` with `doctrine/annotations` v2 : #48792 (comment)

By using `nullOnInvalid` instead of `ignoreOnInvalid`, the call to `addGlobalIgnoredName` is restored.

These changes can be tested with this reproducer: https://github.com/alexislefebvre/symfony_bug_app_48792

Downgrading and upgrading `doctrine/annotations` become possible:

```bash
composer require doctrine/annotations:^1.13.0 --with-all-dependencies
composer require doctrine/annotations:^2.0 --with-all-dependencies
```

Thanks to `@derrabus`: #48912 (comment)

Commits
-------

e058874 [FrameworkBundle] restore call to addGlobalIgnoredName
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants