Make MARC subject heading sort behavior configurable

[rominail]

Subjects appear in item display in the same order they appear in the MARC record, e.g. if a 651 is entered before a 650, the display matches that.
Example : https://catalog.lib.msu.edu/Record/folio.in00006796642#details

TODO

• Update changelog when merging

[demiankatz]

Excellent work, @rominail -- thank you for the contribution!

I have just a couple of minor stylistic suggestions below (and please feel free to disagree with me if you have strong feelings to the contrary).

It might also be nice to add a test case to SolrMarcTest.php to cover the new scenario. I'm happy to help with that if you need me to!

[demiankatz]

Thanks, @rominail -- a few more very minor points of style.

Please let me know if you'd like me to add a test for you, or if you plan to work on that. Once we have test coverage, this should be just about ready to merge.

[rominail]

I made all the modifications
I modified the testSubjectHeadings function, however I am not sure if it should be split in 2 sub-functions.
Also I don't know neither how to alter the record to be able to test if the results are sorted the way we expect instead of just making sure it returns the right values

[demiankatz]

Thanks, @rominail. If you have an example MARC record containing out-of-sequence subject headings, we could create a new test fixture by indexing it and then capturing the Solr response for a request to retrieve it. We could add the test fixture to the fixtures directory and adjust the test case to use it.

It would also probably make sense to use a @dataProvider annotation on the test to separate the different configuration cases, instead of looping through them inside a single test case. If you haven't done this before, I can help refactor.

If you're not sure how best to set up the fixture, I'm also happy to help with that, but if you could at least provide a suitable MARC record that would be a useful starting point.

[rominail]

I made the test, let me know what you think of it

[demiankatz]

Thanks, @rominail, the test looks great! My last thought is simply that we might want to rename the new config setting for clarity; see below. Please let me know what you think about that (and my related comments), and once that detail is settled, this can be merged.

[demiankatz]

Thanks, @rominail -- sorry for breaking your PR. I was trying to save you some time by making small fixes via GitHub while my test VM was tied up with another process, but I clearly didn't quite do everything correctly. :-)

[demiankatz]

A couple of proposed finishing touches:

[rominail]

Thanks, @rominail -- sorry for breaking your PR. I was trying to save you some time by making small fixes via GitHub while my test VM was tied up with another process, but I clearly didn't quite do everything correctly. :-)

NP, thanks for helping :)

[demiankatz]

This is working as expected now. Thanks again!