UrlHelper::isValid() should handle hyphens, '-', correctly

We definitely have a problem!
Domain names cannot start or end with hyphens, but:

  valid_url('http://-example.com-', TRUE); // true

filter_var works as it should:

  filter_var('http://-example.com-', FILTER_VALIDATE_URL); // false
  filter_var('http://-', FILTER_VALIDATE_URL); // false

It all probably comes down to * as the only correct regex to validate URLs with today's and future TLDs (http://වෙබ්.පාර්ලිමේන්තුව.ලංකා/ anyone?).

valid_url doesn't exist in D8 anymore so moving the issue to D7.

Fix valid_url() (Url::isValid() in Drupal 8)

ah!

There isn't any Url::isValid() method, in Drupal 8.8.x.

Here's a test-only patch, to cover leading and trailing hyphens in domain names.

- Separate out the IPv4 test - it's now slightly more strict by requiring 4 number segments, but still allows invalid addresses such as 000.299.300.999
- Require that each domain segment is at least one alphanumeric character, and hyphens are only valid prior to an alphanumeric character or percent-encoded character.

Rerolled patch to 9.1.x.

Retesting with 9.2.x

If that passed, then this is ready for review.

I have tested the last patch on 9.2 dev with few examples
Tested code

Before patch

After patch

The tests in #22 seem to indicate this is working properly. Also updating the issue summary to make it easier to find what the "right" behavior is.

+++ b/core/tests/Drupal/Tests/Component/Utility/UrlHelperTest.php
@@ -101,6 +101,11 @@ public function providerTestInvalidAbsolute() {
+      '-',
+      '-example.com',
+      'example-.com',
+      'example.-com',
+      'example.com-',

I wonder whether it would be worth testing some combinations of minuses as well, like -`example-.com` or so, this way more cases are covered

Just a heads-up - upstream PHP has some changes, that parse_url function returns different results on latest PHP builds. I will update with more concrete examples soon.

Applied #17 patch in drupal-9.3.x-dev looking good and applied successfully
Thanks for the patch

Thanks to everyone who has worked on this. Regular expressions are so much fun, aren't they?

I reviewed the issue summary, the comments, the code, and the test. I did not read the RFCs referenced in the issue summary. I would like to suggest several changes.

The most complicated problem is that the current code has a single regular expression that is supposed to match either a domain name or an IPv4 address: (?:[a-z0-9\-\.]|%[0-9a-f]{2})+. That single regular expression is not strict enough, and I think it makes sense to have separate expressions for the two options (as @gapple pointed out in #14).

The problem is that, when you do that, you are making the match for IPv4 addresses more strict. That is a good thing to do, but it is outside the scope of this issue.

We should change either the patch or the scope so that they match. I suggest we change the scope. That means updating the issue title and the description. When you do that, please be as clear as you can in the "Proposed resolution" section.

In #25, @Ayesh pointed out that there are changes to the parse_url() function in PHP 8. I reviewed the PHP docs for parse_url(). I think the only changes are whether "empty" query strings and fragments are represented by NULL or ''. That should not affect this issue.

I would like to point out that testValidAbsolute() already includes ex-ample.com as one of its test cases.

The following suggestions mostly refer to the main part of the patch:

-        (?:
-          (?:[a-z0-9\-\.]|%[0-9a-f]{2})+                        # A domain name or a IPv4 address
+        (?:                                                     # A domain name
+          (?:[a-z0-9]
+            (?:-?(?:[a-z0-9]|%[0-9a-f]{2}))*
+            (?:\.([a-z0-9]
+              (?:-?(?:[a-z0-9]|%[0-9a-f]{2}))*
+            ))*
+          )
+          |(?:[0-9]{1,3}(?:\.[0-9]{1,3}){3})                    # or a IPv4 address
           |(?:\[(?:[0-9a-f]{0,4}:)*(?:[0-9a-f]{0,4})\])         # or a well formed IPv6 address

1. In #24, @dawehner suggested more test cases, with multiple hyphens. I think that is a good idea, and it is easy to add more test cases.
2. Minor point: "or a IPv4 address" should be "or an IPv4 address". Better: match the format of the IPv6 comment.
3. Move the "A domain name" comment to the correct line (down 1).
4. The regular expression for a part of the domain name does not enforce either part of "start with a letter, end with a letter or digit". It should be something like [a-z]...[a-z0-9]. Except that "letter" should allow a URL-encoded character, so make that (?:[a-z]|%[0-9a-f]{2})...(?:[a-z0-9]|%[0-9a-f]{2})
5. The regular expression -?(?:[a-z0-9]|%[0-9a-f]{2}) requires each hyphen to be followed by an alphanumeric character or URL-encoded non-ASCII character. This is stricter and more complicated than the correct regular expression, [a-z0-9\-]|%[0-9a-f]{2}.
6. Add more test cases, such as "ex--ample.com", related to that last point.