Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: do not mutate BoolQuery instance in toArray method #2241

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

MrkMrk00
Copy link
Contributor

Change the returned value of the BoolQuery::toArray method instead of mutating the instance.

Issue: #1450

@ruflin
Copy link
Owner

ruflin commented Feb 13, 2025

Thanks for opening the PR. This looks very promising as all tests pass -> nothing broke :-) Could you also add an entry to the changelog and convert this to ready for review?

Do you have any concerns on getting this in? Some side effects I'm not thinking of?

@MrkMrk00 MrkMrk00 marked this pull request as ready for review February 13, 2025 14:44
@MrkMrk00
Copy link
Contributor Author

Hello, thanks. I wasn't sure whether to add the entry to Fixes or Change. Let me know if you want it somewhere else.

There's no side-effects that I can think of. Since _params is private, the only way I can see that someone would access it is through reflection.

Sorry for the commit from my different (work) account.

@ruflin
Copy link
Owner

ruflin commented Feb 14, 2025

Changelog LGTM. No worries about the different users for the commits. I'll squash it all together on merge. Running CI now and if all green, I'll get it in.

Should we also backport this to 7.x?

@ruflin ruflin merged commit b37b636 into ruflin:8.x Feb 14, 2025
18 checks passed
@ruflin
Copy link
Owner

ruflin commented Feb 14, 2025

Thanks for the contribution!

@MrkMrk00
Copy link
Contributor Author

Thank you too!

Changelog LGTM. No worries about the different users for the commits. I'll squash it all together on merge. Running CI now and if all green, I'll get it in.

Should we also backport this to 7.x?

I don't see why not. Do you want me to also create a PR to the 7.x branch?

Also the issue (#1450) could probably be closed, since the original problem has been fixed.

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.

3 participants