Skip to content

Improve types in Folio getUserWithCql/escapeCql#5086

Open
berndoberknapp wants to merge 3 commits intovufind-org:dev-12.0from
ubfr:fix-folio-escapecql-null-string
Open

Improve types in Folio getUserWithCql/escapeCql#5086
berndoberknapp wants to merge 3 commits intovufind-org:dev-12.0from
ubfr:fix-folio-escapecql-null-string

Conversation

@berndoberknapp
Copy link
Contributor

Depending on the PHP version and configuration str_replace might throw a TypeError when the third parameter is null. This can happen in the Folio driver for the password, for example when Shibboleth authentication is used. This PR fixes the issue by adding an empty string as default value when the password is null.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @berndoberknapp, this looks great, but I would suggest breaking the solution into two parts:

Part 1: let's add the null coalescing to blank string when escaping the password in a separate PR against the release-11.0 branch -- this way, we can get the fundamental part of the fix into the next minor release without changing any method signatures.

Part 2: after that is done, we can merge the rest of this into the dev-12.0 branch, so the BC-breaking signature changes can go into the next major release.

I think that gets us an immediate functional fix while putting the signature changes into the least disruptive place.

If you don't have time to open the separate release-11.0 PR, I'm happy to do that work -- but I wanted to make sure you are okay with this strategy before I implement it.

@demiankatz demiankatz added this to the 12.0 milestone Feb 19, 2026
@demiankatz demiankatz changed the base branch from dev to dev-12.0 February 19, 2026 14:38
@demiankatz
Copy link
Member

For now I'm re-targeting this PR against dev-12.0 so we don't lose track of it or merge it prematurely, but I'll hold off on approving/merging until we've done the backporting of the null coalescing.

@berndoberknapp
Copy link
Contributor Author

I can create a separate PR with just the null coalescing for release-11.0 and remove that part from this PR.

@demiankatz
Copy link
Member

Thanks, @berndoberknapp. Don't worry about making changes to this PR -- if you just open the separate one against release-11.0, I can merge it through to this one, which should resolve without conflicts and leave the other changes to be merged separately.

@demiankatz demiankatz changed the title Fix null string issue in Folio getUserWithCql/escapeCql Improve types in Folio getUserWithCql/escapeCql Feb 19, 2026
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for #5088. Unfortunately, it looks like I don't have permission to push to your PR branch, so I was not able to merge dev-12.0 into this on your behalf to finish it up. If you could do that at your convenience, then I can get this approved and merged!

Thanks again (both for the fix, and for accommodating my various requests :-) )!

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.

2 participants

Comments