Skip to content

Use config array instead of object for Auth plugins - #5620

Open
rtloftus wants to merge 60 commits into
vufind-org:devfrom
rtloftus:abstract-base-auth
Open

Use config array instead of object for Auth plugins#5620
rtloftus wants to merge 60 commits into
vufind-org:devfrom
rtloftus:abstract-base-auth

Conversation

@rtloftus

@rtloftus rtloftus commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@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.

I would suggest splitting this up for easier review. All of the auth plugins need to be updated together, but the EmailAuthenticator, LoginTokenManager and plain Manager are each stand-alone classes that could be updated independently of the plugins. Depending on your preferences, you could either do a separate PR for each of these three things, or you could do all three of them together as a grouped "Auth utilities" PR.

Additionally, a little more work is needed to get all of the Auth plugins up to speed. For example, it doesn't look like AlmaDatabase is done (and that's just the first I found; there may be more). You may need to search the Auth namespace for $this->getConfig() calls along with plain $this->config references to find all the bits that need changing.

@rtloftus
rtloftus marked this pull request as draft September 8, 2026 14:25
@rtloftus
rtloftus requested a review from demiankatz September 8, 2026 15:22
@rtloftus
rtloftus marked this pull request as ready for review September 8, 2026 15:22

@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.

Now that #5637 is merged, you should merge dev back into this branch and remove the remaining Config object references from the setConfig call in \VuFind\Auth\Manager to align the signatures.

I'll review more carefully once everything is up to date here.

@rtloftus
rtloftus requested a review from demiankatz September 9, 2026 16:03

@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 see you've re-requested a review, but there are no changes since the last review. Is it possible you forgot to push up your latest changes? (It looks to me like this still needs the dev branch merged into it, and the setConfig call in \VuFind\Auth\Manager updated to match the changed signature in the auth plugins).

dependabot Bot and others added 26 commits September 10, 2026 10:48
Bumps [monolog/monolog](https://github.com/Seldaek/monolog) from 3.10.0 to 3.11.0.
- [Release notes](https://github.com/Seldaek/monolog/releases)
- [Changelog](https://github.com/Seldaek/monolog/blob/main/CHANGELOG.md)
- [Commits](Seldaek/monolog@3.10.0...3.11.0)

---
updated-dependencies:
- dependency-name: monolog/monolog
  dependency-version: 3.11.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Bump libphonenumber-js from 1.13.8 to 1.13.11

Bumps [libphonenumber-js](https://gitlab.com/catamphetamine/libphonenumber-js) from 1.13.8 to 1.13.11.
- [Changelog](https://gitlab.com/catamphetamine/libphonenumber-js/blob/master/CHANGELOG.md)
- [Commits](https://gitlab.com/catamphetamine/libphonenumber-js/commits/v1.13.11)

---
updated-dependencies:
- dependency-name: libphonenumber-js
  dependency-version: 1.13.10
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update vendor files via copyUpdatedDeps

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Bumps [stripe/stripe-php](https://github.com/stripe/stripe-php) from 21.0.0 to 21.3.1.
- [Release notes](https://github.com/stripe/stripe-php/releases)
- [Changelog](https://github.com/stripe/stripe-php/blob/master/CHANGELOG.md)
- [Commits](stripe/stripe-php@v21.0.0...v21.3.1)

---
updated-dependencies:
- dependency-name: stripe/stripe-php
  dependency-version: 21.3.1
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Assisted-by: Sonnet 5 (Anthropic)
Co-authored-by: Demian Katz <demian.katz@villanova.edu>

@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.

See below for a solution to the current build failure mystery. :-)

use Lmc\Rbac\Identity\IdentityInterface;
use Lmc\Rbac\Mvc\Identity\IdentityProviderInterface;
use Psr\Log\LoggerAwareInterface;
use VuFind\Config\Config;

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.

This is still being used on line 206 -- that's the reason the tests are failing.

@rtloftus
rtloftus requested a review from demiankatz September 10, 2026 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants