-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
Replace phpro/zf-doctrine-hydration-module with doctrine/doctrine-laminas-hydrator #34
Conversation
Signed-off-by: andyo <[email protected]>
Signed-off-by: andyo <[email protected]>
Signed-off-by: andyo <[email protected]>
Signed-off-by: andyo <[email protected]>
…inas-hydrator Signed-off-by: andyo <[email protected]>
This PR branches off #33 to attempt to fix CI builds for PHP 8 |
@@ -37,6 +36,8 @@ | |||
*/ | |||
class CollectionListener implements ListenerAggregateInterface | |||
{ | |||
public const CONFIG_NAMESPACE = 'doctrine-hydrator'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably isn't the best place for this, thoughts?
{ | ||
if (! $this->hydrator) { | ||
// FIXME: find a way to test this line from a created API. Shouldn't all created API's have a hydrator? | ||
$this->hydrator = new Hydrator\DoctrineObject($this->getObjectManager(), $this->getEntityClass()); | ||
$this->hydrator = new DoctrineObject($this->getObjectManager()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2nd argument passed into this constructor has always been incorrect. It was supposed to be whether to use byValue hydration or not.
Declining in favor of #29 |
Reopening this PR as #29 has become stale and other PR's to address this issue are using incorrect hydrator libraries. |
+1 on this as it is still preventing much needed PHP 8 upgrade |
Also +1 on this |
Please push this through. Will help get the rest of the project up to PHP 8.1/8.2 |
Is this likely to get merged anytime soon @Ocramius @weierophinney? We are starting a new API Tools project and would be good to have some direction. Thank you! |
@javabudd I see you are still targeting PHP 8.0 (not 8.1). I guess this repo is abandoned now, so we'll roll our own Doctrine integration. |
I was the original author of this repository. And I'm a principal on doctrine-laminas-hydrator. Since I left this project I've seen very little movement on this repository. That is unfortunate. I've moved on to GraphQL now. @davidwindell if you're starting a new project I would encourage you to consider GraphQL. My integration library is here: https://github.com/API-Skeletons/doctrine-orm-graphql |
Thanks for the ping @TomHAnderson. REST makes more sense for our use case but will certainly bear doctrine-orm-graphql in mind for future projects 👍 |
I would be more than happy to assist with this repository, hence the pull request, but it seems nobody is willing to review or merge PR's anymore. |
I've been maintaining my own branches for forward compatibility. I can help with the effort needed to ensure that this repo is compatible with 8.2 and 8.3 |
@Ocramius thank you for these meeting minutes. I am shocked to read the "laminas-mvc seems to be almost dead" comment. I guess I need to catch up with the times and start looking at Mezzio. |
FYI we ended up going with API Platform. |
Description