Repository navigation
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
SirLouen
added
[Package] Components
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Thank you for the PR and fixes. I have one question to ensure we are covering internal anchor links.
| // Underscores are not allowed in domain names (RFC 1035). | ||
| const hasUnderscores = val.includes( '_' ); | ||
|
|
||
| if ( hasUnderscores ) { | ||
| return false; | ||
| } | ||
|
|
There was a problem hiding this comment.
As isUrlLike is a loose check, I think we might want to move the underscore check to after the check for isInternal. I'm assuming _ is valid for internal anchors but I could be wrong.
There was a problem hiding this comment.
If I'm correct then we should add test coverage for internal anchors that contain underscores.
There was a problem hiding this comment.
In fact we have been discussing this in Core, and there is one caveat we should consider:
It's very rare, but since DNS have control over subdomains it could exist something like:
https://this_is_an.example.com
Despite of not being compliant, it could technically exist, and there are ancient sites in blogspot with this format
So we have to account for this. We can only care about protocol, domain and tld, but not the full fqdn.
Also there should be more checks not only underscore., I'm going to raise the level of testing.
0d46c4c to
e00ce83
Compare
getdave
left a comment
getdave
left a comment
There was a problem hiding this comment.
Thanks for continue to iterate on this. Having considered in more detail, I have some UX concerns about the strictness of this approach that I'd like to discuss.
Context: Intent Detection vs Validation
I'm currently working on PR #73486, which adds strict URL validation to LinkControl at submission. This will block invalid URLs and show clear error messages before links are saved.
Given that context, I think we need to reconsider the strictness here, as isURLLike serves a different purpose: determining user intent rather than validation.
How isURLLike Affects UX
The function determines whether to:
- Show globe icon and treat input as URL entry (
true) - Search for Posts/Pages and show "Create new page" (
false)
This makes it crucial that it correctly interprets what users are trying to do.
The Problem
The current implementation applies strict RFC validation for intent detection, which creates scenarios that might be confusing (although I'd value your perspective):
Example: User types http://example_test.com
- User has explicitly typed
http://- intent is unambiguous - Current PR returns
false(underscore in domain) - LinkControl searches for Posts/Pages instead
- User sees "Create new page" rather than globe icon
- Confusing: "I typed a URL with
http://, why is it searching?"
The user clearly intended to enter a URL but never reaches the validation that would give helpful feedback.
Recommendation
Since strict validation is coming in PR #73486, should we keep isURLLike focused on intent detection:
1. Trust explicit protocols
if ( protocolIsValid ) {
return true; // User typed a protocol, trust their intent
}2. Use simpler TLD detection
// Just check for: dot + 2-10 letters + (end or slash)
const regex = /\.[a-zA-Z]{2,10}(?:\/|:|$)/;Flow with this approach:
http://example_test.com→ Recognised as URL intent → User submits → Validation error (clear)example_test.com→ Recognised as URL intent → User submits → Validation errormy blog post→ Searches Posts/Pagesexample(no TLD) → Searches Posts/Pages
3. Your subdomain logic is good - perhaps it would be useful in the validation PR instead?
Re: Internal Anchors with Underscores
Thanks for updating this 👍 Adding test coverage would still be valuable:
it( 'returns true for internal anchor links with underscores', () => {
expect( isURLLike( '#my_section_name' ) ).toBe( true );
} );Summary
My concern is about applying strict validation at the intent detection layer rather than at submission. The validation logic itself is solid - it's just about using it at the right point in the user journey.
What do you think? I'd value your perspective as I may have missed something.
|
@getdave got you. I'm going to review based on that perspective. |
What?
Disallow underscore from domains
Why?
According to RFC 1035 (Section 3.5) and RFC 1123 (2.1) domains and TLD should not contain underscores
How?
Introduce two checks, early returning while preserving the same format, to avoid parsing when underscores are present on the string.
Unit tests have been modified accordingly.
PS: I've also added an equivalent for Core that happens not to be checking also
Testing Instructions
There are Unit Tests in place. But this can be tested by registeing a Link Control component and passing a domain with underscores.
To test this easily do the following
Click on each link. You will see that the valid one has an icon, while the invalid one doesnt have it.
Note: Test adding underscores anywhere else, try with different patterns.
Screenshots
Valid:
Invalid