Use config array instead of object in ProQuestFSG search factory - #5615
Use config array instead of object in ProQuestFSG search factory#5615rtloftus wants to merge 27 commits into
Conversation
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @rtloftus. This one fails when I try to run phing.sh startup, and I believe that's because of the issue noted below. Let me know if you want to discuss further!
|
There are a few changes made a couple commits ago to Backend.php and RecordCollection.php; they made the tests pass but were just results of me tinkering and trying to satisfy the test conditions. I have reverted them in later commits, but can revert them if needed. |
demiankatz
left a comment
There was a problem hiding this comment.
I haven't taken a close look at this yet, I just resolved conflicts after merging #5616 and noticed that the build was failing. I suspect the below suggestion will help, but I'll look at everything more closely when I have a bit more time!
demiankatz
left a comment
There was a problem hiding this comment.
I took a deeper look at this; see below for a few more suggestions!
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…tory.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…tory.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…Json/RecordCollection.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…Json/RecordCollection.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
Co-authored-by: Demian Katz <demian.katz@villanova.edu>
…er.php Co-authored-by: Demian Katz <demian.katz@villanova.edu>
demiankatz
left a comment
There was a problem hiding this comment.
Thanks for the progress, @rtloftus!
This remains a large and complex PR, and I think we would benefit from chipping off chunks of it to make it smaller and easier to test/review. Two suggestions for starters:
1.) As previously mentioned, if you open a separate PR with SimilarBuilder, SimilarBuilderTest, and the related change to AbstractSolrBackendFactory::createSimilarBuilder, I think that work can stand alone, and that will get two files out of this PR.
2.) As noted below, the work on the Blender backend is not complete, because the Backend class doesn't match the factory that builds it. I believe that the Blender work can be done independently of the rest of this PR, so it may be worth moving the existing changes to code in the \VuFindSearch\Blender namespace (and related tests) to a separate PR and finishing up the missing factory change there -- and that should eliminate even more files from this PR.
Once we've chipped off those two parts, I'll do some more analysis and see if this is complete, or if we can further reduce it to component parts.
demiankatz
left a comment
There was a problem hiding this comment.
@rtloftus, this still remains incomplete -- this PR now contains only factory changes, but not changes to the classes being built. Most notably, the EdsBackendFactory changes here complement the EDS Backend changes in #5654.
I note that the BlenderBackendFactory changes missing from #5654 are also not implemented here -- so I think that work still needs to be done (and should be handled as part of #5654).
I'd recommend making this PR a draft for now, and we can circle back to it after #5654 is done. My current thought is that we should refine #5654 down to just Blender changes, and then we can create an EDS PR by extracting the relevant files from here and from #5654. After Blender and EDS are done, we can circle back here to determine whether it's best to do all remaining work here, or split things into chunks. There are a lot of Solr-related backend classes, so it might be that the next chunk to split off would be Solr-related work, and after that, the remaining parts might be small enough to finish off here.
waiting on Blender for this one