Skip to content

Conversation

@lippl
Copy link
Contributor

@lippl lippl commented Mar 23, 2020

When initializing the $pwdGraceUseTimeCount it is not checked if the $pwdGraceUseTime may be null, when pwdgraceusetime attribute my be not defined in LDAP. Starting PHP 7.2 count throws a warning when called on null.
[PHP 7.2] count(): Parameter must be an array or an object that implements Countable
As the ldap search also only returns arrays, [] is used instead of null.
The redundant check for the variable in the if query is also removed, as only arryas, which implement countable, can be passed.
Additionally this is harmonized for all other LDAP attributes querried within the handlePasswordExpiry function.

@georgehrke
Copy link
Member

@lippl Is this related to any existing issue?

@georgehrke
Copy link
Member

@lippl Can you please add unit tests for it? Thx!

@blizzz blizzz added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 24, 2020
@blizzz
Copy link
Member

blizzz commented Mar 24, 2020

@lippl looks good from my point of view, just perhaps you can stash the commits into one?

Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

If you only count to see you have more than 0 items, use !empty()

@lippl
Copy link
Contributor Author

lippl commented Mar 25, 2020

@lippl Is this related to any existing issue?

I have not found any open issue for this yet but rather decided to go straight for a fix

If you only count to see you have more than 0 items, use !empty()

@nickvergessen Thanks for the hint! I applied your suggested changes and also extended this to the initial count($pwdGraceUseTime) for a variable by removing the variable, checking for !empty first and only later in case it is true use the count(), as this should be rare cases.
As you already started some pretty printing I also removed some unnecessary blank spaces as well.

@lippl looks good from my point of view, just perhaps you can stash the commits into one?

If that is all fine with you, I would squash everything into one commit.

@blizzz
Copy link
Member

blizzz commented Mar 25, 2020

If that is all fine with you, I would squash everything into one commit.

fine with me :)

@juliusknorr juliusknorr added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 25, 2020
@lippl lippl force-pushed the bugfix/ldap-user_count-warning branch from 2f52660 to 58bc784 Compare March 26, 2020 07:34
Signed-off-by: Philipp Staiger <philipp@staiger.it>
@lippl lippl force-pushed the bugfix/ldap-user_count-warning branch from 58bc784 to 8769d97 Compare March 26, 2020 08:01
@blizzz blizzz added the bug label Mar 26, 2020
@blizzz
Copy link
Member

blizzz commented Mar 26, 2020

Thanks @lippl

@blizzz blizzz merged commit 62403d0 into nextcloud:master Mar 26, 2020
@welcome
Copy link

welcome bot commented Mar 26, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

@blizzz
Copy link
Member

blizzz commented Mar 26, 2020

/backport to stable18

@blizzz
Copy link
Member

blizzz commented Mar 26, 2020

/backport to stable17

@blizzz
Copy link
Member

blizzz commented Mar 26, 2020

/backport to stable16

@backportbot-nextcloud
Copy link

backport to stable18 in #20175

@backportbot-nextcloud
Copy link

backport to stable17 in #20176

@backportbot-nextcloud
Copy link

backport to stable16 in #20177

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

Labels

4. to release Ready to be released and/or waiting for tests to finish bug feature: ldap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants