Composed ILS Driver #3112

ThoWagen · 2023-09-21T12:54:01Z

This new ILS Driver can be used to combine multiple other ILS Drivers. It allows to define multiple drivers for different driver methods and for many methods support drivers can be configured that provide additional information.

Here are some possible use cases for this:

Using the combination of PAIA and DAIA was implemented by letting the PAIA driver inherit from the DAIA driver. With this they can be split into two separate drivers and combined by this driver. PAIA can be used as the default main driver and DAIA can be configured for the methods getHolding, getStatus and getStatuses.
This also allows to use PAIA in combination with other drivers for the record status methods.
One could deactivate all the methods that require a catalog login by using the NoILS-Driver as default main driver and only configure another driver for the methods that don't require a catalog login.
One can supplement the getHolding information with additional information, like summaries, notes etc. from an external API if they are not supported by the main driver.
One can get additional information about a user with the getMyProfile method from another source.

However, the driver does not support MultiILS for login. So currently one can only combine one driver that provides patronLogin with drivers that don't use patron information. An exception to this is when the other drivers can use the patron information of the driver that is used for the patronLogin. But this could be implemented in the future if it is required .

This driver was heavily inspired by the Multibackend driver. Much of the code both drivers have in common was moved into the new AbstractMultiDriver class.

To understand how the driver is supposed to work it might be a good idea to first look into the unit-tests for it.

demiankatz

As the primary maintainer of MultiBackend, I imagine that this work may be of interest to @EreMaijala, so I've asked for a review from him (though I don't think there's a huge rush to complete it).

I also don't object to the name of CombinedDriver, but I wonder if something like ComposedDriver might be slightly more descriptive, since if I understand correctly, the purpose of this driver is to create a single interface by composing functionality from multiple places. Just a thought!

I'll give this a closer look myself after the 9.1 release process is wrapped up.

EreMaijala

This is an interesting development that provides a nice layer of flexibility to the ILS drivers! I didn't see any major issues, so I've commented on some minor ones.

I think that the default return values will need the most work. It might also make sense to make defaultCall throw an ILS exception by default unless instructed otherwise. This would eliminate the need to specify the default return values for cases where no suitable default is not available (or, make it more difficult to return an invalid value by accident).

config/vufind/CombinedDriver.ini

module/VuFind/src/VuFind/ILS/Driver/AbstractMultiDriver.php

module/VuFind/src/VuFind/ILS/Driver/CombinedDriver.php

… per driver

ThoWagen · 2023-09-28T07:40:03Z

@demiankatz I renamed the driver to ComposedDriver.

@EreMaijala I think you are right about throwing exceptions instead of default return values. Since calling driver methods that are not supported should either not be possible or otherwise should be an error. I removed the default return values and now always an exception is thrown.

demiankatz · 2023-09-28T12:18:23Z

Thanks, @ThoWagen -- I've renamed this PR to match the new name of the driver itself.

EreMaijala

Looks good to me! Just one more nit to pick in a comment. :)

config/vufind/ComposedDriver.ini

demiankatz

Thanks, @ThoWagen and @EreMaijala! I gave this another look and found a few minor things to make suggestions about.

Once these are addressed, I think it's ready to merge, but I'm curious whether you think this should be included in release 9.1 or if it's safer to delay to 10.0 due to the significant refactoring of MultiBackend? I'm thinking that a 9.1 release is probably safe, since @EreMaijala is the primary user of MultiBackend and has approved this, but I'm happy to wait if either of you would prefer.

demiankatz · 2023-10-06T10:05:05Z

config/vufind/ComposedDriver.ini

+;drivers_config_path = private
+
+; Definition of all the used drivers in the form <DriverName> = <DriverClass>
+; The name of config file for a driver needs te be the same <DriverName>


Typos -- should be "to be the same as"

demiankatz · 2023-10-06T10:07:07Z

config/vufind/ComposedDriver.ini

+; The format is: support_keys["<DriverName"] = "<key1>,<key2>,...,<keyN>"
+
+; Here are some examples how a configuration could look like for a combination of DAIA and PAIA
+; but it most likely wont work with the Demo driver


"examples of how" might read a little better. "wont" should be "won't". I'm not entirely sure I understand why the Demo driver won't work, or why that is mentioned here (driver2 is not set to Demo in the example above).

fixed. I also explained why the Demo driver won't work.

demiankatz · 2023-10-06T10:09:35Z

module/VuFind/src/VuFind/ILS/Driver/ComposedDriver.php

+     * @param \VuFind\Config\PluginManager $configLoader Configuration loader
+     * @param PluginManager                $dm           ILS driver manager
+     */
+    public function __construct(


Since the configLoader and driverManager are dependencies of the base class, I think you could move this constructor there. I realize that MultiBackend has a different signature, but you could still move this constructor into the base class, and then simplify the MultiBackend constructor to call parent::__construct with the appropriate parameters.

demiankatz · 2023-10-06T10:14:02Z

module/VuFind/src/VuFind/ILS/Driver/Demo.php

@@ -1530,7 +1530,7 @@ function ($loc) use ($excluded) {
     * @param array $holdInfo Contains most of the same values passed to
     * placeHold, minus the patron data.
     *
-     * @return int
+     * @return int|null


?int might be more concise here (though I suspect you copied this from Aleph, so if you prefer to keep it consistent with that, I have no objection).

I did this since @EreMaijala suggested to standardize it here: #3112 (comment)

It looks like it is done like this in a lot of places. So, I don't think this this violates any code style rules. But if you prefer ?int I can change this.

Feel free to leave it alone for now -- since it's consistent with other things, there's no need to change it. I think in the long term I'd prefer the ? syntax instead of the |null syntax since it is more concise -- but fixing that should probably become a separate project, and it's best to back it up with some kind of style checker if we make that decision. All out of scope for what you're doing here. :-)

I didn't mean to imply that you should use int|nullas ?int is indeed better, though I also value keeping the docs of same function consistent. Perhaps we could also add more interfaces and use @inheritDoc to avoid copy-pasting and ensure that the method definitions don't diverge.

The problem with inheritDoc is that php codesniffer doesn't support it properly, so we'd either need to disable comment validation or ignore lots of errors. I wish this would get addressed, but I think it's a very hard problem to solve based on the way codesniffer works.

module/VuFind/src/VuFind/ILS/Driver/ComposedDriverFactory.php

demiankatz · 2023-10-06T10:17:23Z

module/VuFind/src/VuFind/ILS/Driver/MultiBackend.php

@@ -1231,7 +1171,7 @@ protected function getSourceFromParams(
     *
     * @param string $source The source name of the driver to get.
     *
-     * @return mixed  On success a driver object, otherwise null.
+     * @return mixed On success a driver object, otherwise null.


Out of scope for this PR, but maybe something @EreMaijala should consider in a follow-up -- I see that $source can be false-y based on the code below, but the comment and type here don't seem to reflect that.

module/VuFind/tests/unit-tests/src/VuFindTest/ILS/Driver/AbstractMultiDriverTest.php

ThoWagen · 2023-10-06T13:26:05Z

It is absolutely fine for me if it is included in 10.0

demiankatz

Thanks, @ThoWagen -- this looks good to me. I've just made one small edit to add a missing word in a comment.

If @EreMaijala wants to include this in release 9.1, he's welcome to hit the merge button. Otherwise, I'll merge this as soon as we're done with work on release 9.1.

demiankatz · 2023-10-06T13:44:33Z

(Setting a 10.0 milestone to be sure this doesn't get forgotten if it isn't merged sooner).

…#3112)

created combined ils driver

d9dce3c

demiankatz requested a review from EreMaijala September 21, 2023 13:55

demiankatz reviewed Sep 21, 2023

View reviewed changes

EreMaijala requested changes Sep 25, 2023

View reviewed changes

ThoWagen added 3 commits September 27, 2023 14:51

renamed CombinedDriver to ComposedDriver

53431b6

small fixes

a176ebe

throwing exceptions if methods not supported and individual merge key…

4e54148

… per driver

demiankatz changed the title ~~Combined ILS Driver~~ Composed ILS Driver Sep 28, 2023

demiankatz requested a review from EreMaijala September 28, 2023 12:18

EreMaijala requested changes Oct 2, 2023

View reviewed changes

config/vufind/ComposedDriver.ini Outdated Show resolved Hide resolved

renamed head to section

0c9a506

demiankatz requested a review from EreMaijala October 5, 2023 13:07

EreMaijala approved these changes Oct 6, 2023

View reviewed changes

demiankatz requested changes Oct 6, 2023

View reviewed changes

small fixes

963a216

Add missing word.

7160983

demiankatz approved these changes Oct 6, 2023

View reviewed changes

demiankatz added this to the 10.0 milestone Oct 6, 2023

demiankatz merged commit 31f7c84 into vufind-org:dev Oct 19, 2023
6 checks passed

EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Dec 18, 2024

Fix multiselect selection clicks to apply to right target (vufind-org…

4d0e2d1

…#3112)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Composed ILS Driver #3112

Composed ILS Driver #3112

ThoWagen commented Sep 21, 2023

demiankatz left a comment

EreMaijala left a comment

ThoWagen commented Sep 28, 2023 •

edited

Loading

demiankatz commented Sep 28, 2023

EreMaijala left a comment

demiankatz left a comment

demiankatz Oct 6, 2023

ThoWagen Oct 6, 2023

demiankatz Oct 6, 2023

ThoWagen Oct 6, 2023

demiankatz Oct 6, 2023

ThoWagen Oct 6, 2023

demiankatz Oct 6, 2023

ThoWagen Oct 6, 2023

demiankatz Oct 6, 2023

EreMaijala Oct 9, 2023 •

edited

Loading

demiankatz Oct 9, 2023

demiankatz Oct 6, 2023

ThoWagen commented Oct 6, 2023

demiankatz left a comment

demiankatz commented Oct 6, 2023

Composed ILS Driver #3112

Composed ILS Driver #3112

Conversation

ThoWagen commented Sep 21, 2023

demiankatz left a comment

Choose a reason for hiding this comment

EreMaijala left a comment

Choose a reason for hiding this comment

ThoWagen commented Sep 28, 2023 • edited Loading

demiankatz commented Sep 28, 2023

EreMaijala left a comment

Choose a reason for hiding this comment

demiankatz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EreMaijala Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThoWagen commented Oct 6, 2023

demiankatz left a comment

Choose a reason for hiding this comment

demiankatz commented Oct 6, 2023

ThoWagen commented Sep 28, 2023 •

edited

Loading

EreMaijala Oct 9, 2023 •

edited

Loading