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

Allow relative urls #47

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

Goury
Copy link

@Goury Goury commented Jul 26, 2023

Fixes #45

@Goury Goury mentioned this pull request Jul 26, 2023
Copy link
Owner

@ellmetha ellmetha left a comment

Choose a reason for hiding this comment

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

Thanks for providing this! I think the PR is breaking existing behaviors in its current state, so we may need to fix that in order to be able to proceed and merge it. I also think that we should add new unit tests to verify the that tag is working as expected for relative URLs. 🙂

Comment on lines -121 to -122
rendered = '[url={}]{}[/url]'.format(href, value) if option else \
'[url]{}[/url]'.format(value)
Copy link
Owner

Choose a reason for hiding this comment

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

Why remove this logic? This is going to introduce a change in behavior and I don't think this is necessary in order to allow relative URLs.

Copy link
Author

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯
Just felt this way.
I believe you're right, let's not change it too too much.

@Goury
Copy link
Author

Goury commented Jul 30, 2023

Updated

@Goury
Copy link
Author

Goury commented Jul 30, 2023

Also replaced http:// with https:// because year is 2023

@Goury Goury requested a review from ellmetha August 7, 2023 07:13
Comment on lines +407 to +409
for xss in bbcode_settings.URL_XSS_FILTER:
if xss in url:
return url
Copy link
Owner

Choose a reason for hiding this comment

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

This logic is duplicated in three places. Could we define it in a shared method to avoid the duplication (eg. in core/utils)?

Copy link
Author

Choose a reason for hiding this comment

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

On it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relative urls don't work
2 participants