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.

Sign up for GitHub

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

Jump to bottom

Fix incorrect relative/absolute URL usages #29531

Merged
merged 4 commits into from
Mar 2, 2024
Merged

Fix incorrect relative/absolute URL usages #29531

merged 4 commits into from
Mar 2, 2024

Conversation

wxiaoguang
Copy link
Contributor

Add two "HTMLURL" methods for PackageDescriptor. And rename "FullWebLink" to "VersionWebLink"

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 2, 2024
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 2, 2024
@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Mar 2, 2024
@wxiaoguang wxiaoguang added type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 2, 2024
@github-actions github-actions bot added modifies/api This PR adds API routes or modifies them and removed type/bug backport/v1.21 This PR should be backported to Gitea 1.21 labels Mar 2, 2024
@wxiaoguang wxiaoguang mentioned this pull request Mar 2, 2024
Merged
@wxiaoguang wxiaoguang added the backport/v1.21 This PR should be backported to Gitea 1.21 label Mar 2, 2024
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 2, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 2, 2024
@wxiaoguang wxiaoguang added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Mar 2, 2024
@wxiaoguang
Copy link
Contributor Author

Wait for more reviews from #20280

Registry: pd.VersionWebLink(), // TODO: is it right to use VersionWebLink here?

It really looks strange to me.

@lunny lunny requested a review from KN4CK3R March 2, 2024 11:39
@silverwind
Copy link
Member

silverwind commented Mar 2, 2024 •
edited
Loading

What is a HTML URL? I find that a confusing term we should remove.

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Mar 2, 2024 •
edited
Loading

What is a HTML URL? I find that a confusing term we should remove.

Absolute URL with full scheme / host like https://gitea/user/repo/foo/bar. It has been widely used in code.

Maybe you could be interested in #22831 and #22839

@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 2, 2024

What is a HTML URL? I find that a confusing term we should remove.

Agree, I would prefer something like AbsoluteURL and RelativeURL,

@wxiaoguang
Copy link
Contributor Author

What is a HTML URL? I find that a confusing term we should remove.

Agree, I would prefer something like AbsoluteURL and RelativeURL,

I have the same feeling long time ago. But GitHub also uses "html_url" https://docs.github.com/en/rest/repos/contents?apiVersion=2022-11-28

And this PR is just a quick fix. If most people would like to rename, I think it needs a plan & design ahead.

@wxiaoguang
Copy link
Contributor Author

TLDR: at the moment, in most cases, "Link" means relative URL (issue.Link()), while "HTMLURL" means absolute URL (issue.HTMLURL())

@wxiaoguang
Copy link
Contributor Author

Wait for more reviews from #20280

Registry: pd.VersionWebLink(), // TODO: is it right to use VersionWebLink here?

It really looks strange to me.

If there is no more idea about whether it is right, I will just keep it as the old code (a relative link), and merge. New fixes could be done in the future.

@wxiaoguang wxiaoguang added reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. and removed status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR labels Mar 2, 2024
@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 2, 2024

It really looks strange to me.

If there is no more idea about whether it is right, I will just keep it as the old code (a relative link), and merge. New fixes could be done in the future.

Currently looking for an answer. 👍

@KN4CK3R
Copy link
Member

KN4CK3R commented Mar 2, 2024

Should be an absolute URL but not to a package but to the registry itself.
Same as used here:

gitea/routers/api/packages/npm/npm.go

Lines 74 to 77 in 70c126e

resp := createPackageMetadataResponse(
setting.AppURL+"api/packages/"+ctx.Package.Owner.Name+"/npm",
pds,
)

@wxiaoguang wxiaoguang removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 2, 2024
@wxiaoguang
Copy link
Contributor Author

Thank you very much. Done in 0625d18

@wxiaoguang wxiaoguang enabled auto-merge (squash) March 2, 2024 17:35
@wxiaoguang wxiaoguang merged commit bf6502a into go-gitea:main Mar 2, 2024
26 checks passed
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Mar 2, 2024
@wxiaoguang @GiteaBot
Fix incorrect relative/absolute URL usages (go-gitea#29531)
be47b5b 
Add two "HTMLURL" methods for PackageDescriptor. 
And rename "FullWebLink" to "VersionWebLink"
@GiteaBot GiteaBot mentioned this pull request Mar 2, 2024
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Mar 2, 2024
@wxiaoguang wxiaoguang deleted the fix-htmlurl branch March 2, 2024 17:49
wxiaoguang added a commit that referenced this pull request Mar 2, 2024
@GiteaBot @wxiaoguang
Fix incorrect relative/absolute URL usages (#29531) (#29547)
8723389 
Backport #29531 by wxiaoguang

Add two "HTMLURL" methods for PackageDescriptor. 
And rename "FullWebLink" to "VersionWebLink"

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 3, 2024
@zjjhot
Merge remote-tracking branch 'upstream/main'
b476683 
* upstream/main:
  Breaking summary for template refactoring (go-gitea#29395)
  [skip ci] Updated translations via Crowdin
  Fix incorrect cookie path for AppSubURL (go-gitea#29534)
  gitea.service: Remove syslog.target (go-gitea#29550)
  Add option to set language in admin user view (go-gitea#28449)
  Fix elipsis button not working if the last commit loading is deferred (go-gitea#29544)
  Fix incorrect relative/absolute URL usages (go-gitea#29531)
  Add support for API blob upload of release attachments (go-gitea#29507)
  Fix queue worker incorrectly stopped when there are still more items in the queue (go-gitea#29532)
  remove util.OptionalBool and related functions (go-gitea#29513)
  Rename Action.GetDisplayName to GetActDisplayName (go-gitea#29540)
  Make PR form use toast to show error message (go-gitea#29545)
@wxiaoguang wxiaoguang mentioned this pull request Mar 4, 2024
Merged
wxiaoguang added a commit that referenced this pull request Mar 4, 2024
@wxiaoguang
Fix incorrect package link method calls in templates (#29580)
7ec4c65 
Fix #29562
Follow  #29531
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@delvh delvh delvh approved these changes

@KN4CK3R KN4CK3R KN4CK3R approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
6 participants
@wxiaoguang @silverwind @KN4CK3R @lunny @delvh @GiteaBot