Rewrite factories for mongodb-odm version 2

[rieschl]

• This is still a WIP
• The abstract ServiceFactory and a bunch of code is borrowed from https://github.com/Roave/psr-container-doctrine
• Some tests are marked skipped
• Options classes are deprectated (and will be deleted). They are quite useless in a config driven factory.

My personal goal is that everything that's not required for setting up the ODM (Paginator, Logger) and all Laminas specific stuff is stripped and maybe put in a separate package, so that we have a Laminas agnostic way to set up the ODM, just like psr-container-doctrine is doing. At least it should work with mezzio because I'll need it there, too :)

Current behavior and configuration almost stays the same, except for changes that happened in the mongodb-odm package.

Fixes #205

[coveralls]

Coverage increased (+11.9%) to 83.333% when pulling 97737b5 on rieschl:odm-2 into 1cdbb69 on doctrine:master.

[rieschl]

@TomHAnderson @alcaeus Do you mind giving some feedback? I'd like to start using odm2 in production, but I'm not sure about the future of that module (nor is it my decision). For my projects I just need the service factories, which I already migrated to be compatible with ODM2. IMO all the other Laminas relates stuff shouldn't be here anyways, that includes the doctrine-module.
So my questions:

• Should I strip all laminas stuff?
• Could you help me with the skipped tests?
  • ConnectionFactoryTest:86 and :163
  • PaginationAdapterTest:41 and PaginationTest:52. I've never used the Cursor stuff, I don't know how to rewrite that.

Basically, my goal is having a package like Roave/psr-container-doctrine and, if necessary, provide depper Laminas integration in a separate package. But usage of this package is quite low, so I don't know if it's worth the work.
Having a generic PSR-12 package might increase ODM usage?

If we can decide how to go further, I could improve the code a bit and write more tests to make it mergeable.

Thanks!

[TomHAnderson]

Hi @rieschl ,

I wish we already had ODM 2.0 in this package. Work was done to implement it but never finished. This became the last package I control to be migrated to Laminas so I made the decision to ship with ODM v1.

I appreciate your work! Even if moving to 2.0 isn't backwards compatible I'd welcome a major version change to give Laminas users a choice of as-is-now and 2.0.

All the fluff, like pagination, really needs to stay. I view this repo as a sister to doctrine-orm-module which also has fluffy helper classes. I'm afraid I don't know enough to answer your question about a minimalist repo vs a fluffy one. I think the fluff differentiates this repo as it shows "fuller" support of Laminas.

Let's work out the direction this module should take then we can address issues you need help with like skipped tests.

[rieschl]

All the fluff, like pagination, really needs to stay. I view this repo as a sister to doctrine-orm-module which also has fluffy helper classes.
I'm afraid I don't know enough to answer your question about a minimalist repo vs a fluffy one. I think the fluff differentiates this repo as it shows "fuller" support of Laminas.

Alright then. Most of the things should work already, I'll test it in my environment later on but I think there aren't many BC breaks for standard usage. I tried to keep the configuration keys the same so it should minimal effort to upgrade.

Let's work out the direction this module should take then we can address issues you need help with like skipped tests.

I managed to fix two tests, so only the pagination Cursor stuff is remaining. After that, I'll increase the test coverage.

Maybe, in future iterations we can strip out the service factories to a separate package and use that package in this one, to increase abstraction and broaden the usability. As I said, sooner or later I'll need that for mezzio anyways and my preference goes to a neutral PSR-style code.

Maybe @alcaeus can help me with the cursor? Also, I never wrote a changelog and Doctrine (and Laminas for that matter) keep very good changelogs to ease upgrading. So maybe someone can halp me there or point me to a correct direction.

Thanks!

[TomHAnderson]

Maybe, in future iterations we can strip out the service factories to a separate package and use that package in this one, to increase abstraction and broaden the usability. As I said, sooner or later I'll need that for mezzio anyways and my preference goes to a neutral PSR-style code.

@rieschl I hope I'm not too late in the process here but if you want to write a new repository let's call it doctrine/mongodb-orm-psr-container (ideas welcome) which this DoctrineMongoODMModule would use and require and which could be used in mezzio + any other psr container I think this is a fine direction to take Doctrine's support of MongoDB.

You already defined the idea in the quote here and if we had a fluffy and non-fluffy support I like that idea a lot. So whether you want to do that now or in the future it would be welcome.

[rieschl]

@rieschl I hope I'm not too late in the process here but if you want to write a new repository let's call it doctrine/mongodb-orm-psr-container (ideas welcome) which this DoctrineMongoODMModule would use and require and which could be used in mezzio + any other psr container I think this is a fine direction to take Doctrine's support of MongoDB.

You already defined the idea in the quote here and if we had a fluffy and non-fluffy support I like that idea a lot. So whether you want to do that now or in the future it would be welcome.

Hm that shouldn't be a problem. It's basically just extracting the service factories into a separate package. I'd name it doctrine/mongodb-odm-psr-container (so odm, not orm, probably a typo).
But then, the question arises if we should/could consolidate the common stuff from this new package and https://github.com/Roave/psr-container-doctrine into a new base package, i.e:

• doctrine-base-factories
  • Roave/psr-container-doctrine
    • Doctrine ORM Module(?)
  • doctrine/mongodb-odm-psr-container
    • DoctrineMongoODMModule

Most of the factories are just copied from roave/psr-container-doctrine anyways. But I'm not sure if the config key names are the same as here.

What do you think?

[alcaeus]

Maybe @alcaeus can help me with the cursor?

Sure. The biggest change is going to be the absence of a count method on the cursor. The main reason for this is because the count method didn't count the results returned in the cursor, but rather fired off a separate count command to the server, which could return different results.

To deal with this, you could change the signature to accept a Query instance, which can then be used to create a count query with the same filters and use that for counting. Alternatively, you could take a look at the solution employed by APIPlatform in its ODM paginator, which uses an aggregation pipeline using $facet to return results and a count at the same time. Be aware that this will return all results at the same time, so memory usage might be an issue.

The workaround for EagerCursor is no longer necessary, as that cursor class no longer exists. Let me know if you need more information.

[TomHAnderson]

Hi @rieschl ,

I've talked with the Doctrine team and they have concerns about taking on a new repository because that has been a problem in the past.

I recognize your solution here is either put it all into this repository or split out a repo which is very similar to the successful Roave repo which has become the standard for Mezzio installations. I can confidently say Doctrine is not interested in a repository so similar to the functional Roave repository. This is not a judgement on the usefulness of such a repository.

But you're still proposing moving to ODM 2.0 (thanks for the earlier typo correction) and that is something I do want to do. Work on this move internal to Doctrine has stagnated and outside contributions are welcome. I want to stop this reply at this point to hear if you're interested in this approach and what you'll need from me to keep driving towards the ODM 2.0 goal.

[rieschl]

I've talked with the Doctrine team and they have concerns about taking on a new repository because that has been a problem in the past.

I recognize your solution here is either put it all into this repository or split out a repo which is very similar to the successful Roave repo which has become the standard for Mezzio installations. I can confidently say Doctrine is not interested in a repository so similar to the functional Roave repository. This is not a judgement on the usefulness of such a repository.

But you're still proposing moving to ODM 2.0 (thanks for the earlier typo correction) and that is something I do want to do. Work on this move internal to Doctrine has stagnated and outside contributions are welcome. I want to stop this reply at this point to hear if you're interested in this approach and what you'll need from me to keep driving towards the ODM 2.0 goal.

Hey @TomHAnderson Alright, it's fine for me. As everything is MIT licensed anyways, I have no problem to copy&paste the service factories into a separate roave-like repo and also keep everything in this one. It just isn't that DRY.
So I'd suggest the following approach here:
I'll try to fix the Paginator stuff with the input from alcaeus, resolve todos, remove unneeded Options classes, increase coverage and keep everything else in place. If everything passes you can merge and tag it as 3.0.
If I need a separate roave-like package without Laminas dependencies for mezzio, I'll just copy the service factories into a separate package (not in Doctrine).
Is that okay for you?

Thanks!

[alcaeus]

As everything is MIT licensed anyways, I have no problem to copy&paste the service factories into a separate roave-like repo and also keep everything in this one. It just isn't that DRY.

Since that's the drama of the week in PHP-land, please adhere to the license and properly attribute code. Thanks ;)

[rieschl]

As everything is MIT licensed anyways, I have no problem to copy&paste the service factories into a separate roave-like repo and also keep everything in this one. It just isn't that DRY.

Since that's the drama of the week in PHP-land, please adhere to the license and properly attribute code. Thanks ;)

hm? I'm no license guru, but afaik MIT license doesn't require to mention the source, just to add the copyright notice in the LICENCE file. Or what exactly do you mean?

[rieschl]

To deal with this, you could change the signature to accept a Query instance, which can then be used to create a count query with the same filters and use that for counting.

Sorry to ask again, but isn't a Query too late? I can't change the type, limit or offset of an already existing query, can I? I never worked with odm 2 before :)

[TomHAnderson]

In MIT licensing you must keep the @copy; line and add your own @copy; line or if importing from another MIT licensed code add their @copy; to the LICENSE too.

[TomHAnderson]

@rieschl

I've talked with the Doctrine team and they have concerns about taking on a new repository because that has been a problem in the past.
I recognize your solution here is either put it all into this repository or split out a repo which is very similar to the successful Roave repo which has become the standard for Mezzio installations. I can confidently say Doctrine is not interested in a repository so similar to the functional Roave repository. This is not a judgement on the usefulness of such a repository.
But you're still proposing moving to ODM 2.0 (thanks for the earlier typo correction) and that is something I do want to do. Work on this move internal to Doctrine has stagnated and outside contributions are welcome. I want to stop this reply at this point to hear if you're interested in this approach and what you'll need from me to keep driving towards the ODM 2.0 goal.

Hey @TomHAnderson Alright, it's fine for me. As everything is MIT licensed anyways, I have no problem to copy&paste the service factories into a separate roave-like repo and also keep everything in this one. It just isn't that DRY.
So I'd suggest the following approach here:
I'll try to fix the Paginator stuff with the input from alcaeus, resolve todos, remove unneeded Options classes, increase coverage and keep everything else in place. If everything passes you can merge and tag it as 3.0.
If I need a separate roave-like package without Laminas dependencies for mezzio, I'll just copy the service factories into a separate package (not in Doctrine).
Is that okay for you?

Thanks!

Yes that seems fine.

[rieschl]

@TomHAnderson I just had a talk with @alcaeus about the DoctrinePaginator and due to the complicate change he suggested to drop the class altogether. The current implementation is quite basic anyways and probably not sufficient enough for lots of use cases.
A simple alternative would be using the Iterator from the Query object and work directly with the array (Iterator::toArray()). But that's not really efficient and pretty much pointless as there's already a native ArrayAdapter in laminas-paginator.

Is it ok for you to drop the paginator?

Also, I added license headers and asked roave for their permission, just to be sure.

[TomHAnderson]

Thank you for letting me know about the paginator. I'm fine dropping that class as you and @alcaeus can agree to that.

[rieschl]

I had some time to work on that PR.
Is there a replacement for getLoggerCallable? Is the CommandLogger the new logging facility?
Thanks!

[malarzm]

@rieschl yes, CommandLogger is what should be used. You may take a page from doctrine/DoctrineMongoDBBundle#569 which brought logging back to the Symfony bundle.

[rieschl]

Hi all! Sorry for the delay. Covid is quite time consuming :/

I (force) pushed the latest version of my changes, going a different path. Instead of changing all factories to be config driven I kept the Options Classes and just adapted everything that's required to work with ODM 2.
Notably, the Paginator and wiring up of the old logger were removed.

What do you think?

Edit: I tried to use the library in on of our bigger projects and everything worked without changing anything in the configuration. Some changes were necessary because of changes in mongodb-odm 2. We get the Mongo Client/Collection in some places from the DocumentManager and getConnection() was changed to getClient(). But that's about it.

Edit2: doctrine/persistence isn't needed directly in the library but without it, Travis breaks with the DEPS=lowest setting.

Edit3: doctrine/persistence is of course used, its MappingDriverChain is referenced in the config.

[rieschl]

Two more questions:

• Should I drop the logger? Or implmement some kind of CommandLogger like @malarzm pointed out? I didn't quite get the grasp of that and I doubt that anyone would use that, so I'd go with dropping it.

• The options in the Options\Connection class are currently unused. Should I remove that or rewrite it to be able to provide uriOptions to the Client?

[alcaeus]

Should I drop the logger? Or implmement some kind of CommandLogger like @malarzm pointed out? I didn't quite get the grasp of that and I doubt that anyone would use that, so I'd go with dropping it.

In the Symfony bundle, we replaced the logger with a CommandLogger instance. This logs all commands that are sent to the server, which means that the behaviour is going to be vastly different from before (where the logger hooked into individual method calls, e.g. aggregate or find). In the end, it depends on the integration with the framework. Since the logger itself is provided by ODM, any user can create an instance themselves and register it with the client if they are interested in command logging.

The options in the Options\Connection class are currently unused. Should I remove that or rewrite it to be able to provide uriOptions to the Client?

If that's what they are being used for, or if they are not being used at all, then go for it. Keep in mind that you may want to deprecate any setOptions and getOptions methods and introduce new setUriOptions and getUriOptions methods to communicate this change to the user.

[rieschl]

Should I drop the logger? Or implmement some kind of CommandLogger like @malarzm pointed out? I didn't quite get the grasp of that and I doubt that anyone would use that, so I'd go with dropping it.

In the Symfony bundle, we replaced the logger with a CommandLogger instance. This logs all commands that are sent to the server, which means that the behaviour is going to be vastly different from before (where the logger hooked into individual method calls, e.g. aggregate or find). In the end, it depends on the integration with the framework. Since the logger itself is provided by ODM, any user can create an instance themselves and register it with the client if they are interested in command logging.

I'll look into it if I can easily rewrite that. I just noticed that this logger is used in the logging facility of the Laminas developer tools. I don't use that myself so I haven't noticed before.

The options in the Options\Connection class are currently unused. Should I remove that or rewrite it to be able to provide uriOptions to the Client?

If that's what they are being used for, or if they are not being used at all, then go for it. Keep in mind that you may want to deprecate any setOptions and getOptions methods and introduce new setUriOptions and getUriOptions methods to communicate this change to the user.

They aren't used anywhere so it's currently a no-op. That's why I suggested to remove it. As the update will be a major version jump anyways, we don't necessarily have to deprecate anything, especially because they don't to anything.
Meh, I just discovered that the options are currently used. Sorry. So it's just the question if I should rename it to uriOptions or not.

[TomHAnderson]

Logger looks good. I'm going to leave this PR alone until I hear from you, @rieschl , that it's ready to be put into a new release.

[rieschl]

Logger looks good. I'm going to leave this PR alone until I hear from you, @rieschl , that it's ready to be put into a new release.

Ah, sorry, I forgot that the PR isn't marked as draft anymore.
Yes, please still wait a bit, I want to check if the logger is correctly wired up in the config and I'll write some tests for it.

[rieschl]

Alright, I added some tests and checked the package in one of my projects, including the Laminas developer toolbar. Everything seems alright. From my point of view it can be merged.
I'll probably add some more information to the CHANGELOG file to ease upgrading.

Travis is failing because of the release of composer 2 and I don't know how to force usage of composer 1 (dealerdirect/phpcodesniffer-composer-installer hasn't tagged the compatibility patch for composer 2, yet).

[TomHAnderson]

Well keep at it. You're in the home stretch. Did you submit a PR to dealerdirect?

[rieschl]

Well keep at it. You're in the home stretch. Did you submit a PR to dealerdirect?

correction.. composer 2 compatibility was already merged some time ago. Problem is the outdated version of doctrine/coding-standard used in this repo. Should I update it in this PR? But it'll probably generate some noise in the commits.

[TomHAnderson]

It needs to be done. Go ahead and update the Doctrine Coding Standard in this repo. Once that PR is in place I'll start running it locally to help the migration to the updated standard. I'll push PRs to you so we can keep this on the same branch and get some peer review. Sound good?

[rieschl]

It needs to be done. Go ahead and update the Doctrine Coding Standard in this repo. Once that PR is in place I'll start running it locally to help the migration to the updated standard. I'll push PRs to you so we can keep this on the same branch and get some peer review. Sound good?

I just looked into the phpcs and related travis configuration. It doesn't use the doctrine coding standard anyways, so I dropped it entirely. It can be implemented in a separate PR.
Locally the phpcs check passes, so hopefully travis will too.

Edit: the travis pipeline also passed. So you can throw your reviews and change requests at me 🙂

[TomHAnderson]

Just one change for the whole PR. Looks excellent and was a flowing read.

[rieschl]

I just noticed that coveralls has stopped working with build 457. it worked until 456.
I'm not that familiar with travis and coveralls. @TomHAnderson Do you know what's going on here?

[TomHAnderson]

Not off the top of my head. But I need to create Docker testing for this repo anyway and I can add fix Coveralls to it (it may be Doctrine has dropped Coveralls; I can't remember for certain).

How soon do you need a release with this PR?

[rieschl]

Not off the top of my head. But I need to create Docker testing for this repo anyway and I can add fix Coveralls to it (it may be Doctrine has dropped Coveralls; I can't remember for certain).

I got it. There was just an env var missing. Everything is now ok.

How soon do you need a release with this PR?

Asap of course 😉 My initial push was on May 18, so I think I can be patient 🙂
But if everything is OK, there's no need to wait, is there?

[TomHAnderson]

https://github.com/doctrine/DoctrineMongoODMModule/releases/tag/3.0.0

@rieschl there is no rush or reason to wait so I've released it right now. I just wanted to get an idea from you about timelines.
I'll be deleting the master branch per new Doctrine standards. Will you please create a PR with your Doctrine CS standard and fixes on the 3.0.x branch?