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

URL: Conform to URL Living Standard definition of valid URL #19871

Merged
merged 6 commits into from
Feb 4, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Jan 24, 2020

Previously: #19816
Related: #9067
Cherry-picks e2c0f74 from #19823 (would require core back-port)
Unblocks: #19775

This pull request seeks to revise the implementation of @wordpress/url isURL to accept more of what should be considered as valid URLs based on the URL Living Standard definition of a valid URL string.

This includes a number of newly-acceptable URLs:

  • mailto strings
  • Non-HTTP schemes (ftp:, ssh:, git:, etc)

The original implementation was introduced in #9067, and based on the discussion in that pull request, it seems it was largely intended as a refactoring to replace a simple (naive) regular expression, and wasn't explicitly concerned with dealing with HTTP URLs. In fact, the changes with this pull request should further improve upon the intended changes in #9067, where a link will automatically be applied to e.g. mailto:hello@example.com if the text is highlighted in a paragraph.

Testing Instructions:

Ensure unit tests pass:

npm run test-unit packages/url/src/test/index.test.js

Verify there are no regressions in the intended handling of mailto: and tel: links in the Navigation Block link editor experience (#17846), nor in the behavior of URL paragraph text auto-linking (#9067).

@aduth aduth added [Type] Bug An existing feature does not function as intended [Package] Url /packages/url labels Jan 24, 2020
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

I tested this and couldn't see any regressions with the Nav Block. Simple and effective.

packages/url/src/test/index.test.js Show resolved Hide resolved
packages/url/src/test/index.test.js Show resolved Hide resolved
@aduth
Copy link
Member Author

aduth commented Jan 29, 2020

@getdave @WunderBart Would one or the other of you be able to approve the pull request?

Copy link
Member

@WunderBart WunderBart left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks! 🚢

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Thank you for this 👍

@aduth
Copy link
Member Author

aduth commented Feb 4, 2020

Upstream core patch: https://core.trac.wordpress.org/ticket/49360

The branch has been rebased to resolve conflicts, to add references to the above ticket (and a note about when the code can be removed from Gutenberg), and an additional check to make sure that the polyfill handling does not occur if the script is already registered (anticipating that this would be handled by core in the future).

@aduth aduth merged commit 056d7c1 into master Feb 4, 2020
@aduth aduth deleted the update/is-url-more-scheme branch February 4, 2020 20:26
@github-actions github-actions bot added this to the Gutenberg 7.5 milestone Feb 4, 2020
@aduth aduth added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 7, 2020
etoledom added a commit that referenced this pull request Feb 11, 2020
Changes on this PR: #19871 broke the native `is-url` version since it won't throw if the given string is not a url.
@etoledom etoledom mentioned this pull request Feb 11, 2020
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 12, 2020
@aduth
Copy link
Member Author

aduth commented Feb 14, 2020

The polyfill chosen here will not be workable. An alternative is proposed in #20225.

etoledom added a commit that referenced this pull request Feb 14, 2020
* Fix mobile version of is-url.
Changes on this PR: #19871 broke the native `is-url` version since it won't throw if the given string is not a url.

* Mobile unit tests to is-url + better REGEXP

* Fix lint issues

* Remove not-needed comment

* Adding inline comments to explain `.match()` decision over `.test()`.

* Sharing test suit with web.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Url /packages/url [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants