Skip to content

fix(share): Return empty string if no label is set#48673

Merged
susnux merged 2 commits intomasterfrom
fix/null-label
Oct 14, 2024
Merged

fix(share): Return empty string if no label is set#48673
susnux merged 2 commits intomasterfrom
fix/null-label

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Oct 12, 2024 •
edited
Loading

Summary

While the database supports NULL, the typing has always said it only returns string. So to not break any apps that might trust the typings we should return '' if the database is set to NULL.

Checklist

@susnux susnux added bug 3. to review Waiting for reviews php Pull requests that update Php code labels Oct 12, 2024
@susnux susnux added this to the Nextcloud 31 milestone Oct 12, 2024
@susnux susnux requested review from a team, Altahrim, come-nc and sorbaugh and removed request for a team October 12, 2024 15:54
@susnux
Copy link
Contributor Author

susnux commented Oct 12, 2024

/backport to stable30

@susnux susnux requested a review from provokateurin October 12, 2024 18:44
provokateurin
provokateurin requested changes Oct 13, 2024
View reviewed changes
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Oh then you can actually revert #47551 where I thought this was intended but wrongly documented!
It was already broken before 30 as well, so please backport to 29 and 28 as well.

@susnux
Copy link
Contributor Author

susnux commented Oct 14, 2024

Oh then you can actually revert #47551 where I thought this was intended but wrongly documented!

I am not sure. The PHP documentation is only stating strings, but the DB accepts null. I personally think null is much better for representing the empty label then an empty string. But then again we would break some usage of the API if you trust the doc comments.

@come-nc
Copy link
Contributor

come-nc commented Oct 14, 2024

I thought some DB will treat null and empty string the same anyway and so we cannot differentiate in DB, no?

@provokateurin
Copy link
Member

provokateurin commented Oct 14, 2024

Yeah, Oracle treats empty strings as NULL.
While I agree that it is nicer in the API to use null for a missing label we should fallback to an empty string to always have consistent behavior.

susnux added 2 commits October 14, 2024 15:15
@susnux
fix(share): Return empty string if no label is set
2e4d3e4 
* Resolves: #48629

While the database supports NULL, the typing has always said it only returns *string*.
So to not break any apps that might trust the typings we should return `''` if the database is set to `NULL`.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux
revert: "fix(files_sharing): Make share labels nullable"
64dd4ce 
This reverts commit 01c4fa3.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux requested a review from provokateurin October 14, 2024 13:17
@susnux
Copy link
Contributor Author

susnux commented Oct 14, 2024

Oh then you can actually revert #47551 where I thought this was intended but wrongly documented!

Done

@provokateurin provokateurin mentioned this pull request Oct 14, 2024
Closed
@susnux susnux merged commit b964e05 into master Oct 14, 2024
@susnux susnux deleted the fix/null-label branch October 14, 2024 17:10
@susnux
Copy link
Contributor Author

susnux commented Oct 14, 2024

/backport to stable29

@backportbot backportbot bot mentioned this pull request Oct 14, 2024
Merged
@backportbot backportbot bot mentioned this pull request Oct 14, 2024
Merged
2 tasks
@provokateurin
Copy link
Member

provokateurin commented Oct 14, 2024

@susnux not 28?

@susnux
Copy link
Contributor Author

susnux commented Oct 16, 2024

/backport to stable28

@backportbot backportbot bot mentioned this pull request Oct 16, 2024
Merged
2 tasks
@fcharlaix-opendsi fcharlaix-opendsi mentioned this pull request Nov 28, 2024
Closed
8 tasks
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kesselb kesselb kesselb approved these changes

@provokateurin provokateurin provokateurin approved these changes

@come-nc come-nc come-nc approved these changes

@Altahrim Altahrim Awaiting requested review from Altahrim Altahrim is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

3. to review Waiting for reviews bug php Pull requests that update Php code

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

[Bug]: Sharing links with null labels broken after upgrade to NC-30

4 participants

@susnux @come-nc @provokateurin @kesselb