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

Drop "/{username}.png" in favor of "/user/avatar/{username}" #23908

Conversation

6543
Copy link
Member

@6543 6543 commented Apr 4, 2023

the user pattern "*.png" was never reserved, so we either have to reserve it or drop the path.
I propose to drop the path in favor of an alternate one

for the full dialog read #23874 (comment)

alternative / close #23992

@6543 6543 added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label Apr 4, 2023
@6543 6543 added this to the 1.20.0 milestone Apr 4, 2023
6543 added a commit to 6543-forks/gitea that referenced this pull request Apr 4, 2023
@6543 6543 changed the title Drop "/{username}.png" in fafour of "/user/avatar/{username}" Drop "/{username}.png" in favour of "/user/avatar/{username}" Apr 4, 2023
@6543 6543 added the type/bug label Apr 4, 2023
@6543 6543 changed the title Drop "/{username}.png" in favour of "/user/avatar/{username}" Drop "/{username}.png" in favor of "/user/avatar/{username}" Apr 4, 2023
@6543 6543 requested a review from a team April 4, 2023 04:49
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

users that end in *.png will already be broken, so we should add a blocker to prevent new users from creating such users. that wouldn't be a breaking change.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 4, 2023
@6543

This comment was marked as resolved.

@codecov-commenter

This comment was marked as outdated.

@techknowlogick
Copy link
Member

I had read that, but am still preferring to add the *.png exclusion, as any user already created with that would be broken per the URLs routes. So by adding in the exclusion, then there is no breaking change, but by dropping the route it is a breaking change. I use that route somewhat regularly (at least once a month), and so that is where I am coming from.

@6543
Copy link
Member Author

6543 commented Apr 7, 2023

alternative -> #23992

@silverwind
Copy link
Member

silverwind commented Apr 10, 2023

Do we know of any external integrations that use these endpoints? When were they introduced?

@6543
Copy link
Member Author

6543 commented Apr 10, 2023

this is no api so there was never a promise to not break ... btw.

@6543
Copy link
Member Author

6543 commented Apr 10, 2023

I personally don not have a strong opinion when it comes to either drop or reserve the png case ...

but would like to not see the reserve pattern list increase at all if it comes to new things ...

@silverwind
Copy link
Member

silverwind commented Apr 10, 2023

I guess the ideal solution would be to keep supporting it and just be more intelligent during the routing. Assuming users user and user.png exist:

  • /user.png -> return avatar of user
  • /user.png.png -> return avatar of user.png

That could be done for all these endpoints and it would not be breaking.

@silverwind
Copy link
Member

Ah, I see that can't work because we shadow the user page with such a route.

@silverwind
Copy link
Member

I guess I prefer to add to reserved users, just to not break any existing integrations.

@silverwind
Copy link
Member

I guess a /avatar/{username} may still be nice to have for future SVG avatars where a /avatar/{username}/{size} route does not make sense.

techknowlogick pushed a commit that referenced this pull request Apr 10, 2023
Org/User names ending with ".png" where not functional, so reserve them

alternative / close  #23908
@6543 6543 deleted the drop_/{username}.png_in_fafour_of_/user/avatar/{username} branch April 11, 2023 15:05
@GiteaBot GiteaBot removed this from the 1.20.0 milestone Apr 23, 2023
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants