Skip to content

Use config array instead of object for Facet Recommend modules - #5645

Merged
demiankatz merged 18 commits into
vufind-org:devfrom
rtloftus:recommend
Sep 10, 2026
Merged

Use config array instead of object for Facet Recommend modules#5645
demiankatz merged 18 commits into
vufind-org:devfrom
rtloftus:recommend

Conversation

@rtloftus

@rtloftus rtloftus commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@rtloftus

rtloftus commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

I'm not sure failing tests here have to do with the goal of this PR

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@rtloftus, I saw your comment about the failing test, so I focused on diagnosing that problem. I haven't reviewed the rest of your changes here yet, since I wasn't sure if you were completely done yet. Just let me know when you're ready and I'll take another look!

Comment thread module/VuFind/src/VuFind/Recommend/ConsortialVuFind.php Outdated
@rtloftus
rtloftus requested a review from demiankatz September 9, 2026 14:20

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @rtloftus -- see below for some additional work that's needed to get everything working here.

It may also be worth thinking about the scope of this PR. There are a lot of recommendation modules, so doing them all at once might get unwieldy. If we only do some, maybe it would make sense to do them in related batches -- for example, split this PR into separate ones for ConsortialVuFind, LibGuidesProfile, and facet-related recommendations. I don't mean to be a pest about breaking things apart, so if that's more work than it's worth, I don't mind finishing this up as-is, so long as we don't keep having to expand the scope until it becomes unwieldy. :-)

Comment thread module/VuFind/src/VuFind/Recommend/LibGuidesProfile.php Outdated
Comment thread module/VuFind/src/VuFind/Recommend/ConsortialVuFind.php Outdated
Comment thread module/VuFind/src/VuFind/Recommend/AbstractFacets.php Outdated
Comment thread module/VuFind/src/VuFind/Recommend/AbstractFacets.php
@rtloftus
rtloftus requested a review from demiankatz September 9, 2026 15:45
@rtloftus rtloftus closed this Sep 9, 2026
@rtloftus rtloftus reopened this Sep 9, 2026
@rtloftus
rtloftus marked this pull request as draft September 9, 2026 15:53
@rtloftus rtloftus changed the title Use config array instead of object for Recommend modules Use config array instead of object for Facet Recommend modules Sep 9, 2026
@rtloftus
rtloftus marked this pull request as ready for review September 9, 2026 16:15

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, @rtloftus, this looks good! I edited SideFacets.php to unwrap some lines to improve readability but made no functional changes there. I have one suggestion below which I will immediately apply. After that, I'll run tests and approve/merge if they pass!

Comment thread module/VuFind/src/VuFind/Recommend/TopFacets.php Outdated
@demiankatz demiankatz added this to the 12.0 milestone Sep 10, 2026
@demiankatz demiankatz added the architecture pull requests that involve significant refactoring / architectural changes label Sep 10, 2026

@demiankatz demiankatz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tests passing!

@demiankatz
demiankatz merged commit ca73c07 into vufind-org:dev Sep 10, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

architecture pull requests that involve significant refactoring / architectural changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants