Skip to content

LDAP: fix null where IUser is expected in update groups background job - #23799

Merged
blizzz merged 2 commits into
masterfrom
fix/noid/ldap-update-groups-null
Nov 2, 2020
Merged

LDAP: fix null where IUser is expected in update groups background job#23799
blizzz merged 2 commits into
masterfrom
fix/noid/ldap-update-groups-null

Conversation

@blizzz

Copy link
Copy Markdown
Member

This can happen when a user was already removed, but the DB table was having old information. Previously – with the old events – it did not cause a problem, because an old user id was passed. Now, an IUser object was required. Comes with tests and thus some refactoring against the necessary methods of the job. The actual fix is the second commit.

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/noid/ldap-update-groups-null branch from 89d2b10 to 3a51160CompareOctober 30, 2020 15:30
@faily-bot

faily-botBot commented Oct 30, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 34809: failure

mysql8.0-php7.4

Show full log
There was 1 error:
1) Test\Files\Storage\Wrapper\PermissionsMaskTest::testScanNewWrappedFiles
Error: Call to a member function getPermissions() on bool
/drone/src/tests/lib/Files/Storage/Wrapper/PermissionsMaskTest.php:126
--
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

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

🐘

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

even test.
🐘

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

->from('ldap_group_members');
$result = $qb->execute()->fetchAll();

$this->groupsFromDB = [];

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.

you could use array_map :)

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.

yes. doesn't make much difference, but perhaps lookin a bit fancier than good ol' foreach. iirc it is also more memory hungry than foreach.

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.

who cares about memory footprint if you can have fancy 😉

for me it's mostly about readability. If I see an array_map I immediately can guess what's happening.

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.

Actually can be a concern, depending how users and groups you throw on it. In this case the readability wouldn't really change as it is a one-line-transformation of the result array, either way.

Comment threadapps/user_ldap/lib/Jobs/UpdateGroups.php
@blizzzblizzz mentioned this pull request Nov 2, 2020
@blizzz
blizzz merged commit 1a3daa6 into masterNov 2, 2020
@blizzz
blizzz deleted the fix/noid/ldap-update-groups-null branch November 2, 2020 17:14
@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable20

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable19

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable18

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

Labels

3. to reviewWaiting for reviewsbugfeature: ldaptechnical debt🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@blizzz@rullzer@nickvergessen@ChristophWurst@skjnldsv