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

UserResource Service Migration #28

Closed
wants to merge 4 commits into from

Conversation

skellamp
Copy link

@skellamp skellamp commented Jan 6, 2024

No description provided.

Copy link
Owner

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

@skellamp, thanks for getting started on this!

I see you have this marked as a draft, so maybe it's not ready for review yet. I just ran the test suite to see if things were passing, and I'm getting these failures:

There were 2 failures:

1) VuFindTest\Mink\FavoritesTest::testAddRecordToFavoritesNewAccount
Element not found: .savedLists a index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:499
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/FavoritesTest.php:159
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

2) VuFindTest\Mink\ListViewsTest::testFavoritesInTabMode
Element not found: #information_cd588d8723d65ca0ce9439e79755fa0a-content .savedLists ul index 0
Failed asserting that null is of type "object".

/usr/local/vufind/module/VuFind/src/VuFindTest/Integration/MinkTestCase.php:407
/usr/local/vufind/module/VuFind/tests/integration-tests/src/VuFindTest/Mink/ListViewsTest.php:125
/usr/local/vufind/module/VuFind/src/VuFindTest/Feature/AutoRetryTrait.php:104

FAILURES!
Tests: 2621, Assertions: 14467, Failures: 2, Skipped: 20.

Perhaps you're already aware of these, but sharing in case it's helpful. If you need me to dig in deeper to identify what is going wrong, let me know and I'm happy to help!

@demiankatz
Copy link
Owner

(I should also note that while I was testing anyway, I also merged the latest dev branch into doctrine, and the latest doctrine branch into here, resolving some minor conflicts along the way; I don't think that's the cause of the failing tests, but I suppose it's possible if you weren't seeing these before).

@demiankatz
Copy link
Owner

@skellamp, thanks again for all of the valuable work you did to help us move forward with the Doctrine migration. While I will not be merging this PR as-is, I have incorporated most of the work you did here (in slightly modified form) in vufind-org#3751 and commit vufind-org@ac1d867. I will now close it as completed.

Because of the huge scale of the Doctrine migration work, we ended up deciding to take a more incremental approach -- in release 10.0, we are refactoring all of our code to use database services, but leaving the low-level interactions as Laminas\Db. This will eventually reduce vufind-org#2233 down to simply porting Laminas\Db code to Doctrine code, which makes it much easier to review and understand. The full migration will be completed next year in release 11.0.

Some things are changing shape a little bit while we re-evaluate all of the refactoring, but a large amount of your work will be in release 10.0, and the remainder will be in 11.0. You set a very helpful foundation for us to build upon -- thank you again for all of the effort you put into the project during your time at Villanova!

@demiankatz demiankatz closed this Jun 6, 2024
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.

2 participants