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 doctrine ORM 3.0+ compatibility #467

Merged
merged 1 commit into from
Jan 31, 2025
Merged

Conversation

geecu
Copy link
Contributor

@geecu geecu commented Jan 30, 2025

doctrine/orm:3.x made some changes that break lexik/translation-bundle functionality.

EntityRepository::count got a return type: https://github.com/doctrine/orm/blob/c3cc0fdd8c9ffb102170c930ca56188626a34719/src/EntityRepository.php#L137 making the implementation of TransUnitRepository::count throw an error (closes #459)

AbstractHydrator now has a $stmt property instead of doctrine/orm:2.x $_stmt, this is used by SingleColumnArrayHydrator.

I've tested these changes in two projects that I've had at hand, one using doctrine/orm:3.2.2 (the permalinks I've put above are from doctrine/orm:3.0.x, though) and another one using doctrine/orm:2.20.1 - they work in both.

For testing I have ran the import, changed something in the UI, exported - it all worked as expected. Let me know if you want me to test something else.

I've also ran the phpunit tests and they all pass - but I guess travis ci will confirm it.

@bartmcleod bartmcleod merged commit b51202a into lexik:master Jan 31, 2025
1 check passed
@xrogers
Copy link

xrogers commented Feb 10, 2025

@bartmcleod this doesn't solve compatibility issues with latest orm, especially that it is missing the composer version changes.
This should be combined or replaced with #465

@bartmcleod
Copy link
Collaborator

@xrogers It seems to have solved a problem for @geecu Do I understand correctly that your major concern is that the version that his solution is intended for is not in composer.json? I understand that his solution is backwards compatible and that updating composer.json is optional because of that. Don't you agree?

@xrogers
Copy link

xrogers commented Feb 11, 2025

@xrogers It seems to have solved a problem for @geecu Do I understand correctly that your major concern is that the version that his solution is intended for is not in composer.json? I understand that his solution is backwards compatible and that updating composer.json is optional because of that. Don't you agree?

Unfortunately it couldn't solve the problem, as the constraint in the composer.json doesn't allow to install the latest orm. I've tried it in my project (deliberately pointing to the current master) and it wouldn't work while pointing to the fork from the mentioned pr works quite well.

To sum up, maybe it helps a bit with getting there but doesn't actually solve the compatibility problem it is supposed to be solving.

And maybe it makes sense to prepare a major version that breaks backwards compatibility supporting only the latest versions of libraries, because maintaining full compatibility with all of the versions might be problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Symfony 7.1 Compatibilty
3 participants