Skip to content

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

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

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

Conversation

@susnux

@susnuxsusnux commented Oct 12, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@susnuxsusnux added bug 3. to review Waiting for reviews php Pull requests that update Php code labels Oct 12, 2024
@susnuxsusnux 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 teamOctober 12, 2024 15:54
@susnux

susnux commented Oct 12, 2024

Copy link
Copy Markdown
ContributorAuthor

/backport to stable30

provokateurin
provokateurin requested changes Oct 13, 2024
View reviewed changes

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

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

susnux commented Oct 14, 2024

Copy link
Copy Markdown
ContributorAuthor

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

come-nc commented Oct 14, 2024

Copy link
Copy Markdown
Contributor

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

@provokateurin

provokateurin commented Oct 14, 2024

Copy link
Copy Markdown
Member

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
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 commented Oct 14, 2024

Copy link
Copy Markdown
ContributorAuthor

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

Done

@provokateurinprovokateurin mentioned this pull request Oct 14, 2024
Closed
@susnux
susnux merged commit b964e05 into masterOct 14, 2024
@susnux
susnux deleted the fix/null-label branch October 14, 2024 17:10
@susnux

susnux commented Oct 14, 2024

Copy link
Copy Markdown
ContributorAuthor

/backport to stable29

@backportbotbackportbotBot mentioned this pull request Oct 14, 2024
Merged
@backportbotbackportbotBot mentioned this pull request Oct 14, 2024
Merged
2 tasks
@provokateurin

provokateurin commented Oct 14, 2024

Copy link
Copy Markdown
Member

@susnux not 28?

@susnux

susnux commented Oct 16, 2024

Copy link
Copy Markdown
ContributorAuthor

/backport to stable28

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

Reviewers

@kesselbkesselbkesselb approved these changes
@provokateurinprovokateurinprovokateurin approved these changes
@come-nccome-nccome-nc approved these changes
@AltahrimAltahrimAwaiting requested review from AltahrimAltahrim is a code owner automatically assigned from nextcloud/server-backend
@sorbaughsorbaughAwaiting requested review from sorbaughsorbaugh is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugphpPull 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