Convert from Laminas\Db to Doctrine#2233
Conversation
|
I spent some more time on proof-of-concept work today. I decided to try revising some of the code in the admin module's tag controller, since that does complex queries and seemed like a potentially interesting starting point. So far, results are promising -- not only is the DQL query language more readable than the Laminas\Db object-oriented query builder, but I also managed to find and fix a bug along the way. Some notes on today's progress: 1.) We need to have a place to put database-related logic. In the previous code, this was in the row and table classes. In the new code, I propose a set of VuFind\Db\Service classes designed to deal with various types of entities. I've started work on a TagService here, and I've set up the necessary plugin mechanism for interacting with services. This may need more granularity, but we can decide that as we gain more experience working with Doctrine. 2.) For extensibility purposes, we also need a way to abstract entity classes so that they can be overridden. I have created a VuFind\Db\Entity\PluginManager for this purpose, and I defined a marker interface for entities so that the plugin manager can validate what it is creating. Note that overriding entities appears to have one major limitation: annotations are not inherited from parent classes, so any local custom entity needs to fully duplicate its parent's properties and annotations. Perhaps there is a way around this, but given how infrequently I expect people to need to extend/override entities, it may also be a limitation that we can live with. There is still a massive amount of work that needs to be done here, but I'm encouraged by this start. I will continue to chip away as time permits, and I welcome input or offers of collaboration. Eventually, it may be best to "divide and conquer" on all the conversion work, since there is so much of it. |
|
Just a quick update on progress, because I believe this branch is currently in a somewhat broken state: In order to incorporate Doctrine queries with the Laminas Paginator, I had to add the The Anyway, I've created TODO checkboxes to capture all these balls that are in the air, and I apologize for leaving this in a somewhat broken state, but I didn't want to lose my work in progress, and I'm nearly out of time for today. I'll pick this up again as soon as practical. |
|
Hi @demiankatz, this change is very exciting because our Admin Interface has been using I wonder whether it would make sense to develop the new Doctrine based VuFind database layer as a separate module/package, in a similar way that has been discussed in #2272 and #2283 (which I hope to continue working on as soon as time allows)? In any case it would be great to have this developed in a way that would allow for code reuse, even if only by copying the classes over! |
|
Thanks, @aleksip, I'm glad that you're happy with this approach. I'm open to organizing the code any way that is helpful; I think it's just a question of what works best. Perhaps one approach could be to create a VuFindDb module to contain the Entity classes, ConnectionFactory, etc. I'm not sure if the actual Service classes would fit there as well, or if they'd be better off in the main VuFind module due to their potential entanglement with other dependencies. I think I'd have to get deeper into the project to have a stronger opinion about that, but I'm open to input! In any case, right now, my priority is finishing up all the outstanding work on VuFind 9, so this is more or less on hold, but I'm hoping this will be one of the priority changes for VuFind 10, and I expect I'll be spending a lot of time on it in 2023. I'm definitely happy to discuss/plan in the meantime so we have a clear path forward when it comes time to execute the work. I'm also very open to brainstorming strategies for collaboration if you or anyone on your team might be able to help with the updates, as this is a very big project. I'm hoping that, in addition to updating everything, we can make strides toward introducing better test coverage for core code, since if we do this right, it should make things easier to test than the old database library did. |
|
@demiankatz Sounds good! I am currently experimenting with potential |
|
Thanks, @aleksip, that's great! Certainly if you have classes from your admin code that it would make sense to move over here for use in the core, that might save us some time and trouble. I'm not sure if the existing auto-generated Entity classes are actually the best they could be, as most of them have not yet been tested. |
demiankatz
left a comment
demiankatz
left a comment
There was a problem hiding this comment.
@aleksip, thanks for the progress on configuration here -- just leaving a couple of comments/questions related to the latest changes.
| 'service_manager' => [ | ||
| 'allow_override' => true, | ||
| 'factories' => [ | ||
| 'Doctrine\ORM\Mapping\Driver\AnnotationDriver' => 'VuFind\Db\AnnotationDriverFactory', |
There was a problem hiding this comment.
@aleksip, I see that you deleted this factory, but the configuration is still here. We should clean something up.
There was a problem hiding this comment.
Yes, forgot to remove that line. Should I push the change to your branch or what would be the best way to work on this together?
There was a problem hiding this comment.
Please feel free to push directly here, as long as the code still works. I'm not going to be working on this much until next year, so I welcome any progress you can make in the meantime, and I don't anticipate that we'll "step on each other," so to speak.
There was a problem hiding this comment.
Also, if at any point it would be helpful to get a second opinion or anything, or if you would like me to run any tests, just let me know and I'll be happy to help!
There was a problem hiding this comment.
@demiankatz I meant to push the changes to my repository and then point you there. Not sure how multi-user PRs should work... Should I push future suggested changes to my clone so you can then decide whether to merge to your branch from there?
There was a problem hiding this comment.
@aleksip, it is possible to make a pull request against a pull request -- you can just target a PR against this branch. That might be the easiest workflow going forward, especially when multi-user collaboration picks up... but since things are quiet right now, feel free to work directly in this branch, at least until you get things into a correct/stable state again. (I think it's easier to just move forward and fix things than to roll back changes and make a separate PR, at least under the present circumstances).
There was a problem hiding this comment.
@demiankatz Seems like I can create a PR for the branch in your repository! Should I do that? Edit: missed your above reply.
There was a problem hiding this comment.
If you want to try it as a proof-of-concept, feel free... but otherwise, as I said, you can just go ahead and push. :-)
|
@demiankatz I have accidentally pushed my changes to your branch, sorry about that! I have been refactoring our Admin Interface database implementation to be based on this PR, and those were the changes I needed to make. They were needed to support two entity managers with connections to different databases. I don't claim to fully understand how Doctrine's configuration works, but the I also wondered about the need for |
|
@aleksip, I am certainly not an expert on Doctrine's configuration either, and I've been learning as I go... so if the annotation driver can be replaced by pure configuration, I'm entirely in favor of that, and if a different naming convention would be more robust, I also have no objection to changes. |
demiankatz
added
the
architecture
|
I spent some time today working on PostgreSQL compatibility. The test suite is now in parity with the dev branch when running against PostgreSQL (there is one known bug causing a single test to fail in both places). |
Thanks for that pointer -- when time permits, I'll see if I can do something with that! If anyone else has time in the meantime, please just let me know so we can coordinate our work. |
| $parameters['includeFilter'] = $includeFilter; | ||
| } | ||
| if (!empty($excludeFilter)) { | ||
| $where[] = 'ul NOT IN (:excludeFilter)'; |
There was a problem hiding this comment.
Might have noticed a small error here, but not totally sure. Here the list is being compared and in include filter its comparing the id.
There was a problem hiding this comment.
Yes, this definitely merits deeper testing. I'll try to find time to take a closer look soon!
There was a problem hiding this comment.
I did some experimentation and found that the syntax is equivalent -- whether you use ul or ul.id, the query works correctly when you pass in an array of integers or an array of entity objects. But to make this more consistent, I've pushed a commit to use ul in both places so it's less confusing!
It's definitely possible -- I've created a proof-of-concept DoctrineSchemaValidationTest.php in another branch. The problem is that I don't really understand how to fix the errors it is revealing. It seems that Doctrine wants to drop and recreate many of the indexes; I'm not sure why it doesn't recognize the existing ones as being valid and correct. I imagine some of this may be related to naming issues, but I don't think that's the whole story. I also strongly suspect there will be additional challenges getting this test to pass consistently in both MySQL and PostgreSQL. Any ideas? |
Co-authored-by: sambhavp96 <40562292+sambhavp96@users.noreply.github.com>
|
Okay, thanks to @sambhavp96's excellent work, several schema inconsistencies have been sorted out, and the Doctrine schema validation test I developed is now passing. See f8526d2 for details. |
| public function getSearchById(int $id): ?SearchEntityInterface | ||
| { | ||
| return $this->getDbTable('search')->select(['id' => $id])->current(); | ||
| return $this->entityManager->find(SearchEntityInterface::class, $id); |
There was a problem hiding this comment.
@demiankatz, this line raises a runtime exception in my local test installation:
Ausnahme: Doctrine\Persistence\Mapping\MappingException Class 'VuFind\Db\Entity\SearchEntityInterface' does not exist
Should it be Search::class or \VuFind\Db\Entity\Search::class here (and in the rest of the file)?
There was a problem hiding this comment.
The SearchEntityInterface is defined in https://github.com/vufind-org/vufind/blob/dev/module/VuFind/src/VuFind/Db/Entity/SearchEntityInterface.php, and SearchEntityInterface is set up as an alias in the PluginManager, so I'm not sure why you are encountering a problem. I would expect the code to work as written (and indeed, if it didn't, I would expect some of our integration tests -- particularly SavedSearchesTest -- to fail).
I'll try to find time to double-check this more carefully in my own test environment when I'm back in the office tomorrow. In the meantime, is there anything unusual about your environment? Can you specify what steps you're taking to raise the exception? Are you able to run integration tests?
There was a problem hiding this comment.
I get more similar exceptions ("Class 'VuFind\Db\Entity\ResourceEntityInterface' does not exist" and others).
There was a problem hiding this comment.
Does the whole system fail to work, or are you just seeing these types of errors occasionally? What PHP version are you using? Is there any possibility that your Composer dependencies are out of date or that there's something odd in your cache?
There was a problem hiding this comment.
I don't think that macOS is a very common environment for VuFind tests, so yes, that's unusual. The integration tests (vendor/bin/phing qa-php) show lots of messages like Unable to connect to localhost:80 (Connection refused), so it is also unusual that the Homebrew Apache listens on port 8080.
There was a problem hiding this comment.
I believe that @EreMaijala has had success getting things working in the environment, so he might have some advice. Have you seen the notes on the testing wiki page?
There was a problem hiding this comment.
That exception happens at runtime if you catch all exceptions in a debugger, but does not affect functionality. I suppose the mechanism is trying to look for a corresponding class but doesn't find it for an interface.
There was a problem hiding this comment.
I catch "everything" (which includes all exceptions) in my debugger because it typically indicates errors or at least weaknesses in the code. Here an AI chatbot also says that looking for a class with the name of the interface is wrong, and the name of the class which is derived from the interface should be passed instead.
There was a problem hiding this comment.
@stweil, it is conventional when using Laminas service managers to name services for interfaces, to make it easier to install local custom versions without having to alias concrete classes in potentially confusing ways. That is the pattern that has been followed here, and it is set up in our custom EntityManagerFactory. This may be a conflict between Laminas practices and Doctrine practices, but it was an intentional choice based on Laminas practice.
I don't really have strong feelings about this one way or the other; I think the likelihood of locally overriding entities is relatively small. However, it is a fairly large change to make this close to a major release, so I would be hesitant to change it further unless there's a very strong reason to do so. Maybe the thing worth investigating further is why exceptions are being thrown and caught when aliases are properly set up -- but I imagine that would require a deep dive into the Doctrine code.
9 participants
This is work in progress on replacing VuFind's database abstraction layer.
TODO
doctrine/doctrine-orm-moduleandroave/psr-container-doctrinein composer.json, or if we can migrate fully from the psr-container-doctrine to the doctrine-orm-module.createNativeQueryworks correctly across platforms (e.g. make sure tag deduplication works in PostgreSQL)