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

Fix compatibility with doctrine/persistence 2.0 #1332

Merged

Conversation

jordisala1991
Copy link
Member

@jordisala1991 jordisala1991 commented Feb 22, 2021

Subject

Class aliases need to be created on a place that it is always executed,
previous code was only executed on an empty cache. Next request were
always broken. Moving to the bundle class ensures the code is always
executed.

I am targeting this branch, because this is a bugfix and BC.

Closes #1331
Closes #1336

Changelog

### Fixed
- Fix compatibility with doctrine/persistence 2.0

Class aliases need to be created on a place that it is always executed,
previous code was only executed on an empty cache. Next request were
always broken. Moving to the bundle class ensures the code is always
executed.
@jordisala1991 jordisala1991 requested review from phansys and a team February 22, 2021 11:33
@jordisala1991
Copy link
Member Author

jordisala1991 commented Feb 22, 2021

Well, it works on a project, but not on the test suite, any ideas?

Maybe adding the alias on the classes that needs them?

@jordisala1991 jordisala1991 marked this pull request as draft February 22, 2021 11:56
@phansys
Copy link
Member

phansys commented Feb 22, 2021

IMO, these aliases are not really required by this package, but by "friendsofsymfony/user-bundle". Maybe we should just move them to tests/bootstrap.php in order to allow our test suite to pass the build.
The final users should declare these aliases in their own bootstrapping, since the issue around them is not actually under our responsibility.

@VincentLanglet
Copy link
Member

IMO, these aliases are not really required by this package, but by "friendsofsymfony/user-bundle". Maybe we should just move them to tests/bootstrap.php in order to allow our test suite to pass the build.
The final users should declare these aliases in their own bootstrapping, since the issue around them is not actually under our responsibility.

It should be in custom_boostrap then.
https://github.com/sonata-project/SonataUserBundle/blob/4.x/tests/bootstrap.php#L27

@jordisala1991
Copy link
Member Author

IMO, these aliases are not really required by this package, but by "friendsofsymfony/user-bundle". Maybe we should just move them to tests/bootstrap.php in order to allow our test suite to pass the build.
The final users should declare these aliases in their own bootstrapping, since the issue around them is not actually under our responsibility.

Well, you are declaring this package as compatible with doctrine/persistence 2.0, but this package has a dependency that is not compatible. IMHO we either fix this with code like this one, or we can't do this. Someone just updates this packages and then code stops working.

Agree on adding it to tests/bootstrap.php if that fixes the issue, but as I explained before I don't think that is the responsability of each user to know what we did here and add some alias so have the bundle working again.

@VincentLanglet
Copy link
Member

We could declare the BC method as a static one and call it every where we need the BC layer

@phansys
Copy link
Member

phansys commented Feb 22, 2021

I think using SonataUserBundle::boot() for projects and tests/custom_bootstrap.php for the test suite is the best approach.

@phansys
Copy link
Member

phansys commented Mar 2, 2021

@jordisala1991, do you have time to finish this PR?

@jordisala1991
Copy link
Member Author

I will try tomorrow, yes

@jordisala1991 jordisala1991 marked this pull request as ready for review March 3, 2021 07:42
@jordisala1991 jordisala1991 requested a review from a team March 3, 2021 07:43
* We MUST remove this method when support for "friendsofsymfony/user-bundle" is dropped
* or adapted to work with "doctrine/common:^3".
*/
private function createDoctrineCommonBackwardCompatibilityAliases(): void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about making this method public static in order to call it in the custom_boostrap ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it would be bc break to remove it, IMO not a good idea.

@phansys phansys merged commit 9136fb0 into sonata-project:4.x Mar 3, 2021
@phansys
Copy link
Member

phansys commented Mar 3, 2021

Thank you @jordisala1991.

@SonataCI
Copy link
Collaborator

SonataCI commented Mar 3, 2021

Ok @phansys,

I requested a new release for SonataUserBundle via Slack in #releases channel 👍

If you want to get notified about new releases, make sure to follow SonataNews on Twitter!

@sonata-project sonata-project deleted a comment from phansys Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants