Expand LinkWidget test coverage

Adding issue credit for folks who spent 20+ mins triaging the related issue in the OP

Are there any other test cases that should be added?

Did some searching and found a list of valid and invalid URLs and added those to the list. This assumes that list is correct (I haven't read the relevant RFC). There may be some duplication with existing tests which I didn't catch. I found it a bit of an eye strain to compare the new ones with the existing ones.

The valid URLs pass tests but some of the invalid ones don't. I did manual testing to determine that these invalid ones are accepted in the link field.

The following invalid URLs are accepted.

http://.
http://..
http://../
http://foo.bar?q=Spaces should be encoded
//a
foo.com
http:// shouldfail.com
http://foo.bar/foo(bar)baz quux
http://-error-.invalid/
http://-a.b.co
http://a.b-.co
http://0.0.0.0
http://3628126748
http://.www.foo.bar/
http://www.foo.bar./
http://.www.foo.bar./

Found some related issues, going back to 2008. I still need to read them thoroughly.

I've taken a look at each of those issues, which all have patches. The solution I like best is 2652236-comment #34 where Charlie ChX Negyesi posts a custom solution that uses the the Symfony Url validator. I prefer that instead of re-inventing the wheel.

So, here we could keep in scope and remove failing tests. Well, instead of removing how about adding in a todo block with a reference to a followup. That followup could be one of the existing issues or a new one to discuss using Symfony Url validator.

Getting this back in scope. This patch removes the false positives from the list of invalid URLs added in #7. That leaves a patch that only adds more test coverage to the LinkWidget. Fixing/Improving the validation will happen in one of the other issues.

Missed one.

One concern I have here is that this is a functional test, so we're adding quite a lot of overhead/execution time to our tests by adding all of these cases, as each one of them does an extra 2 HTTP requests.

So the question is, can we test this using a kernel test - the validate method is static and public, so in theory yes.

Removing the Novice tag, because refactoring to a kernel test is an advanced topic.

Alright another try here. This just adds a kernel and does not change the functional tests. The Functional test already includes a test for the error message from validateUriElement and results from 3 of the 4 constraints. I don't see a test for "LinkNotExistingInternal", but I could be wrong.

No interdiff because this removes all previous changes and adds a new test.

1. +++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
   @@ -0,0 +1,121 @@
   +  public function testExternalLinkValidation() {
   

   🤩 amazing work.

2. +++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
   @@ -0,0 +1,121 @@
   +    $test_links = $this->getTestLinks();
   

   At first I was going to suggest this could use a dataProvider but then we're paying the install cost for each test-case, so I think it is better as it stands

3. +++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
   @@ -0,0 +1,121 @@
   +      $link_item->setValue($data[0]);
   ...
   +      $expected_count = count($data[1]);
   

   we could use list($value, $violations) = $data
   to make this a bit more readable than $data[0] and $data[1]

4. +++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
   @@ -0,0 +1,121 @@
   +  public function getTestLinks() {
   

   This can be protected

thanks for the review.

1. Thank you, I have had good teachers!
2. I meant to leave a comment explaining why I did that, sorry. It was just too slow as a dataprovider.
3. Fixed.
4. Fixed

And fixed the order of the parameters in $this->assertEquals so the expected value is the first parameter.

+++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
@@ -0,0 +1,122 @@
+      [$value, $expected_violations] = $data;

This keeps us on 9.x only here - which I guess is ok. (Array de-structuring is not supported in php 7.0)

+++ b/core/modules/link/tests/src/Kernel/LinkItemUrlValidationTest.php
@@ -0,0 +1,122 @@
+      ["rdar://1234", [$violation_0]],

I went to commit this, but unfortunately, this is failing with a spelling error.

perhaps we should make it radar?

Moving to needs review for ^

that passes the spelling check - but would it change the functionality of the test? I don't think so

I agree that 'radar' doesn't change the functionality of the test. Patch updated.

@larowlan, apologies for the inconvenience. Looks like today I will getting cspell up and running.

Committed e2fc217 and pushed to 9.1.x. Thanks!

Backported this to 9.0.x for consistency and because the risk of disruption is low.

Held off backporting to 8.9.x because it uses PHP 7.1+ array destructuring syntax.

Thanks everyone, this was a great demonstration of the collaborative feel of the Bug Smash Initiative at work.