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

NEW Add UrlField #11116

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Conversation

emteknetnz
Copy link
Member

src/Forms/UrlField.php Outdated Show resolved Hide resolved
src/Forms/UrlField.php Outdated Show resolved Hide resolved
src/Forms/UrlField.php Outdated Show resolved Hide resolved
src/Forms/UrlField.php Outdated Show resolved Hide resolved
src/Forms/UrlField.php Outdated Show resolved Hide resolved
src/Forms/UrlField.php Outdated Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/5/url-field branch 2 times, most recently from dd16dd3 to 071fa85 Compare January 30, 2024 23:26
@emteknetnz
Copy link
Member Author

@GuySartorelli Since parse_url() doesn't actually validate the url, and symfony/validator was just weird (uses attributes) I've switched back to using filter_var() for the actual validation. It works fine, unit tests pass which include underscores and parenthesis in tests

@emteknetnz emteknetnz force-pushed the pulls/5/url-field branch 2 times, most recently from 2223cde to 4c975d4 Compare January 31, 2024 00:00
@GuySartorelli
Copy link
Member

Assuming you approve and merge #11123, please use symfony/validator for validating the URL.

src/Forms/UrlField.php Outdated Show resolved Hide resolved
@michalkleiner
Copy link
Contributor

Good addition. Have you thought about supporting other URI schemas, not just http(s)?

There are other schemas such as mailto, tel, sftp, git+ssh, protocol-less URLs are also valid starting with //, so I think this is currently underdelivering and if people wanted to use it for other URLs than just web URLs then they would need to resort to plain text fields again and won't benefit from the new field.

For reference e.g. https://en.wikipedia.org/wiki/List_of_URI_schemes and https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Example_URIs.

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

Have you thought about supporting other URI schemas, not just http(s)?

For now this is only intended to support external http(s) URLs - we were originally going to add this directly to the linkfield module (see the associated issue), but decided if it's in core it can be used more widely (within the bounds of its intended purpose).

More schemes could be added in the future, but that would require additional validation logic which means more dev time to (at a minimum) find a library or constraint which handles that validation correctly - which just isn't in scope right now for the work we're doing.

@GuySartorelli GuySartorelli merged commit 5c753d5 into silverstripe:5 Feb 2, 2024
15 checks passed
@GuySartorelli GuySartorelli deleted the pulls/5/url-field branch February 2, 2024 01:48
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.

3 participants