-
Notifications
You must be signed in to change notification settings - Fork 440
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(attachments): allow null expireDate in createShare #12432
Conversation
Signed-off-by: Grigorii K. Shartsev <me@shgk.me>
/backport to stable29 |
/backport to stable28 |
/backport to stable27 |
If I understand the changes correctly, the public interface always allowed it to be null? Which I would then say needs to be fixed in talk? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok so far.
$share->setSharedWith($shareWith); | ||
$share->setPermissions($permissions); | ||
|
||
if ($expireDate !== '') { | ||
if ($expireDate !== null && $expireDate !== '') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty
returns true for both null
and empty string (''
).
if ($expireDate !== null && $expireDate !== '') { | |
if (!empty($expireDate)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I wanted to keep it separated to explicitly show 2 different cases, because ''
and null
have different semantics here
☑️ Resolves
expiryDate
value in server server#44485$expireDate
is nownull
by default and can be not onlystring
🛠️ API Checklist
🚧 Tasks
expireDate
nullable and check for null on setting date🏁 Checklist
docs/
has been updated or is not required