Skip to content

fixes infinitely repeating LDPA search results with PHP <= 7.2 - #21108

Merged
rullzer merged 1 commit into
masterfrom
fix/20745/eor-old-php-api
May 26, 2020
Merged

fixes infinitely repeating LDPA search results with PHP <= 7.2#21108
rullzer merged 1 commit into
masterfrom
fix/20745/eor-old-php-api

Conversation

@blizzz

@blizzzblizzz commented May 25, 2020

Copy link
Copy Markdown
Member

fixes#20745

@blizzzblizzz added this to the Nextcloud 20 milestone May 25, 2020
@blizzz

blizzz commented May 25, 2020

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@blizzz
fixes infinitely repeating LDPA search results with PHP <= 7.2
15008a1
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/20745/eor-old-php-api branch from 9a9f830 to 15008a1CompareMay 25, 2020 22:37
rullzer
rullzer approved these changes May 26, 2020
View reviewed changes

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

🐘

juliusknorr
juliusknorr approved these changes May 26, 2020
View reviewed changes

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

Code looks good, however I fail to reproduce the issue on my PHP 7.2 instance, so I cannot verify that it actually works 🙈

@rullzer
rullzer merged commit 69e2aa0 into masterMay 26, 2020
@rullzer
rullzer deleted the fix/20745/eor-old-php-api branch May 26, 2020 09:54
@backportbot-nextcloudbackportbot-nextcloudBot mentioned this pull request May 26, 2020
Merged
@CHBMBCHBMB mentioned this pull request Jun 14, 2020
Closed
hashworks
hashworks reviewed Jun 17, 2020
View reviewed changes
Comment threadapps/user_ldap/lib/Access.php
if (!$this->hasMoreResults()) {
// when the cookie is reset with != 0 offset, there are no further
// results, so stop. This if block is not necessary with new API
// and can be removed with dropping PHP 7.2

@hashworkshashworksJun 17, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why can we remove this block when we drop PHP 7.2 when this solves an issue that occured on 7.4 (#20745)?
Or is that new API not implemented yet?

@blizzzblizzzJun 17, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I failed to reproduce it on 7.4 for some unknown reason. Ought to double check and remove this part of the comment.

@kenyhskenyhs mentioned this pull request Jun 19, 2020
Closed
@RenWal

RenWal commented Jun 26, 2020

Copy link
Copy Markdown

One word of caution for those applying this fix (and maybe something that can be automatically solved in the release):

This bug caused the ScanFiles job to run to an ever-increasing user offset (since it thought there are still more users because the LDAP app ignored the offset). At about offset 39000, this caused a Maximum function nesting level of '256' reached, aborting! since the LDAP paged user search is implemented recursively. (Monitor your log files, folks)

Now, this fix would solve the issue regarding the ever-increasing offset, but the cron job now fails before that can happen because of the recursion error. It's solved by manually forcing the offset to zero.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer approved these changes
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@juliusknorrjuliusknorrjuliusknorr approved these changes
@Dennis1993Dennis1993Awaiting requested review from Dennis1993
@splitt3rsplitt3rAwaiting requested review from splitt3r
+1 more reviewer
@hashworkshashworkshashworks left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugfeature: ldapregression

Projects

None yet

Milestone

Nextcloud 20

Development

Successfully merging this pull request may close these issues.

Repeated user results on search page (LDAP, (not only) PHP 7.2)

6 participants

@blizzz@RenWal@rullzer@MorrisJobke@hashworks@juliusknorr