Files with spaces in URIs fail entity TypedData validation

Will start to work on this.

This will also fail if uri contains non-ASCII characters. Do we wanto to fix that also?

See: http://php.net/manual/en/filter.filters.validate.php (FILTER_VALIDATE_URL row)

Replaced filter validation url with a regular expression match.

Reference to regex: http://www.ietf.org/rfc/rfc3986.txt (Page 50)

Patch now contains both the test and the validation.

1. +++ b/core/lib/Drupal/Core/Validation/Plugin/Validation/Constraint/PrimitiveTypeConstraintValidator.php
   @@ -49,7 +49,7 @@ public function validate($value, Constraint $constraint) {
   -    if ($typed_data instanceof UriInterface && filter_var($value, FILTER_VALIDATE_URL) === FALSE) {
   +    if ($typed_data instanceof UriInterface && preg_match('|^(([^:/?#]+):)?(//([^/?#]*))?([^?#]*)(\?([^#]*))?(#(.*))?|', $value) == FALSE) {
   

   I suggest you put the actual regex string in a constant on this class (const URI_VALIDATION_REGEX = ... and add the reference to the RFC standard to the docblock.

2. +++ b/core/modules/system/src/Tests/TypedData/TypedDataTest.php
   @@ -236,8 +236,8 @@ public function testGetAndSet() {
        $this->assertNull($typed_data->getValue(), 'URI wrapper is null-able.');
        $this->assertEqual($typed_data->validate()->count(), 0);
   -    $typed_data->setValue('invalid');
   -    $this->assertEqual($typed_data->validate()->count(), 1, 'Validation detected invalid value.');
   +    $typed_data->setValue('public://field/image/Photo on 4-28-14 at 12.01 PM水.jpg');
   +    $this->assertEqual($typed_data->validate()->count(), 0, 'Validation detected invalid value.');
   

   Now you *replaced* the invalid validation, don't do that, add yours instead, we still want to make sure that invalid strings don't validate.

   As we replace the API function with our own, I think we should considerably extend our test coverage here to test a list of valid and invalid strings. You could set up an array of $uri => 1/2 and then loop over that and validate it, making sure the value is as expected.

Well I did my best but cannot completely figured out how the validation should be handled.

What I did do is write a test with a list of valid and invalid URI's according to several references.
For the test see my TEST-ONLY patch.

If you'd like the test including some regular expressions I tried (but which failed) then download my full patch file.

What I do notice is that FILTER_VALIDATE_URL comes quite close to a solution, there are only a few problems with FILTER_VALIDATE_INT it does not validate correctly on white spaces in the filename, some special characters in filename and IDN characters in domain names.

I'll follow this issue and will help wherever I can.

For my references check out:
http://www.ietf.org/rfc/rfc3986.txt
http://en.wikipedia.org/wiki/Filename
http://www.domainnameshop.com/faq.cgi?id=8&session=106ee5e67d523298
http://en.wikipedia.org/wiki/Filename

Thank you @ChrisjuhB. Having an extensive test suite for this will help us a lot!

I went through http://www.ietf.org/rfc/rfc3986.txt a bit and as far as I understand it it does not allow URIs with spaces at all. It says spaces (and other stuff like unicode, ...) should always be %-encoded. This kind of makes sense. Let's say we allow spaces in URIs. That would also make "http://exam ple.com" valid, which is obviously wrong. Is it possible that we're going a bit against the specs when it comes to handling URIs?

Taking what I wrote above into consideration it seems that the only option we have here is %-encoding URIs before we send then to filter_var.

Any thoughts?

Well, we're calling it uri, but doesn't that simply mean that $file->uri is *not* a valid URI? What if we just don't claim it is? Should possibly have custom validation instead that does things like checking for matching stream wrapper and if possible, check for existence of that file?

Changing the validation would affect validation of URI's that actually *are* valid, we can't just change it for $file->uri.

Yes, this probably makes sense. I wouldn't check if file exists though as that might bring to much performance overhead.

Rerolled the patch.

Taking another way to fix this : sticking with "uri" datatype and perform URI validation through file_create_url().

Needs a reroll.

Also removing the 'needs tests' tag as the patch above has a test (although it would be nice to upload separately to illustrate the current failing behavior).

DrupalSouth code sprint "reroll" to come

Re-rolled

It seems that changes introduced in #2412509: Change LinkItem.uri to type 'uri' rather than 'string' and introduce user-path: scheme fixed the original issue as the last patch only adds the related unit test which passes.

I'm not setting this RTBC as I contributed to the issue several months ago.

RTBCing, I had alook at this with Jonathan yesterday. Original issue fixed, but no test coverage; this patch adds it.

Nice. +1 for RTBC

This is only adding test code therefore permitted under beta evaluation rules. Committed 3363c81 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/system/src/Tests/TypedData/TypedDataTest.php b/core/modules/system/src/Tests/TypedData/TypedDataTest.php
index 649ff6f..9cfeb8d 100644
--- a/core/modules/system/src/Tests/TypedData/TypedDataTest.php
+++ b/core/modules/system/src/Tests/TypedData/TypedDataTest.php
@@ -240,7 +240,7 @@ public function testGetAndSet() {
     $typed_data->setValue('invalid');
     $this->assertEqual($typed_data->validate()->count(), 1, 'Validation detected invalid value.');
     $typed_data->setValue('public://field/image/Photo on 4-28-14 at 12.01 PM.jpg');
-    $this->assertEqual($typed_data->validate()->count(), 0, 'Filename with spaces is valid (see https://www.drupal.org/node/2278073).');
+    $this->assertEqual($typed_data->validate()->count(), 0, 'Filename with spaces is valid.');
 
     // Generate some files that will be used to test the binary data type.
     $files = array();

Adding a link to the issue is not necessary. Fixed on commit.