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

Use array_merge when reading cached groups members #25024

Merged
merged 2 commits into from
Jun 10, 2016

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Jun 8, 2016

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @blizzz, @leo-b and @alexweirig to be potential reviewers

@PVince81 PVince81 added this to the 9.1-current milestone Jun 8, 2016
@nickvergessen
Copy link
Contributor

nickvergessen commented Jun 8, 2016

Can't test, but looks way better logicwise to me 👍

@rullzer
Copy link
Contributor

rullzer commented Jun 8, 2016

👍

@DeepDiver1975
Copy link
Member

unit tests?

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 8, 2016

unit tests?

😢

I can have a try but it might take a day or two, especially considering that there is no test for this section of the code.

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 9, 2016

Let's see whether I can mock my way into that specific branch of the code... that function is huge!

@PVince81
Copy link
Contributor Author

PVince81 commented Jun 9, 2016

Ha, take this! 059778b

Reverting the array_merge change will make this fail.

Please review!

@alexweirig
Copy link
Contributor

@PVince81
I changed the line of code and tested in the 9.1 daily build I had setup last week and the merge doesn't seem to cause any problems.
👍

@PVince81
Copy link
Contributor Author

@alexweirig thanks a lot!

@PVince81 PVince81 merged commit 1abe5b0 into master Jun 10, 2016
@PVince81 PVince81 deleted the ldap-cachedgroupsbymember branch June 10, 2016 09:06
@DeepDiver1975
Copy link
Member

DeepDiver1975 commented Jun 10, 2016

Backports are pending - down to which version?

  • 9
  • 8.2
  • 8.1
  • 8

@PVince81
Copy link
Contributor Author

Sorry, forgot to remove the label.

This was a special case: we already had a backport to 9.0 #24950 (comment) but @nickvergessen found a problem. This PR here fixes it and I cherry-picked the fix to the backport PR, which is also merged already.

@alexweirig
Copy link
Contributor

@DeepDiver1975 maybe backport to 8.2 if possible?

@nickvergessen
Copy link
Contributor

maybe backport to 8.2 if possible?

Not needed, because the breaking commit was only introduced with 9.0+

@alexweirig
Copy link
Contributor

But 8.2 didn't even have dynamic groups support, we implemented it based on 8.2 and we're still using 8.2.1 because later 8.2 versions don't have it, or am I wrong?

@lock
Copy link

lock bot commented Aug 5, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants