Split OAI server attachNonDeleted function #4160
Conversation
There was a problem hiding this comment.
Thanks, @LuomaJuha, this is a good idea! Just a couple of comments....
There was a problem hiding this comment.
I suggested adding a comment below.
Also, I think perhaps there is a simpler and clearer approach to the return value. My understanding is that if the XML is empty, it means there is no XML and the record should be skipped... but if the XML is false, it means there was some kind of error. So I think we could change this in two possible ways:
Option 1: Change the return value of getRecordAsXML to string|false, and then return the XML directly. In the empty check, you can return $xml !== false; so you return false for an error and true for any other empty condition.
Option 2: Change the default_return key of getRecordAsXML to error_encountered (or something similar), and instead of setting it to false in the default case, set it to $xml === false.
What do you think?
There was a problem hiding this comment.
Thanks, @LuomaJuha, I think we're almost done, but your most recent change highlighted a flaw in the existing code that I think we might as well clean up while we're here. See below for details!
| { | ||
| if ('oai_vufind_json' === $format && $this->supportsVuFindMetadata()) { | ||
| // Return empty string instead of false for BC. | ||
| return $this->getVuFindMetadata($record) ?: ''; |
There was a problem hiding this comment.
I appreciate the commitment to backward compatibility here, but I actually think in this instance, it was a bug that we didn't handle false return values from getVuFindMetadata(). I'd suggest allowing the false return, and also fixing the return type annotation and comment on getVuFindMetadata() to reflect the possibility that an error can occur.
There was a problem hiding this comment.
oh, well, from an improvement to a bugfix, will adjust :)
demiankatz
added
the
architecture
There was a problem hiding this comment.
Looks good to me, and tests are still passing. Thanks, @LuomaJuha!
Splits attachNonDeleted metadata fetching part to its own function for easier local modifications.