Skip to content
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

Reduce share ID casts #37512

Closed
wants to merge 1 commit into from
Closed

Reduce share ID casts #37512

wants to merge 1 commit into from

Conversation

provokateurin
Copy link
Member

Summary

Split out from #37390

Checklist

@come-nc
Copy link
Contributor

come-nc commented Apr 5, 2023

Also, doing (int)$string will convert to string even if the string is weird, like '10km' will convert to 10. Here if someone tries to call an API method on an invalid share id, it should return an error, not silently work with another share id. So I would add tests for is_numeric and error out if it’s not.

Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

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

Cannot judge. I have the impression that there is a mixture of int and string uses on IDs still.

@provokateurin
Copy link
Member Author

Also, doing (int)$string will convert to string even if the string is weird, like '10km' will convert to 10. Here if someone tries to call an API method on an invalid share id, it should return an error, not silently work with another share id. So I would add tests for is_numeric and error out if it’s not.

This stuff is already done in the setId implementation, but it just fails silently. Before the path was like string -> (int) -> (string) and I removed that extra cast to int.

@provokateurin
Copy link
Member Author

Cannot judge. I have the impression that there is a mixture of int and string uses on IDs still.

I know, I tried fixing it, but it's not completely possible.

@provokateurin
Copy link
Member Author

@come-nc @blizzz I should have made more clear what this PR is about. I'm trying to minimize the amounts of casts that happen everywhere for share ids. They get changed quite often because different places use either string or int. IShare uses string, so I converted some places to also use string instead of casting to int and then back to string.

@blizzz
Copy link
Member

blizzz commented Apr 6, 2023

Please excuse the dumb question, i am not familiar with all the aspects there, but going string everywhere, as by API design, is not possible?

@provokateurin
Copy link
Member Author

I think we can do that in most cases. Internally the id is an integer in the database so there needs to be some casting at least. We'd need to make sure that the output of API calls stay integer if they are integers right now. One downside I see is that devs might not be aware that the ids should be valid integers, but then the code is a mess anyway, so it's not much worse.

@provokateurin provokateurin changed the title Fix types of share related things Reduce share ID casts Apr 12, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv deleted the fix/type-shares branch March 14, 2024 07:50
@skjnldsv skjnldsv removed this from the Nextcloud 28 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants