I searched and I did not see this reported, so I figured I would go ahead and file it. According to the W3C validator, links created using Upload are not HTML5 when files have spaces in their names. The validator throws the following exception: "Whitespace in path component. Use %20 in place of spaces."...."Syntax of IRI reference:
Any URL. For example: /hello, #canvas, or http://example.org/. Characters should be represented in NFC and spaces should be escaped as %20."

If you would like to check it out, go ahead and try this validating this page: http://www.croomsalumni.com/about using this validation service http://validator.w3.org/ (just remember to change the DocType to HTML5).

If I am correct, this should be pretty easy to fix, a simple replace function type of deal.

CommentFileSizeAuthor
#40 1277140-2.patch616 bytesEverett Zufelt
#36 1277140.patch614 bytesEverett Zufelt
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

frank0051’s picture

Version: 6.22 » 6.x-dev

Just changing to 6.x-dev

Everett Zufelt’s picture

Version: 6.x-dev » 8.x-dev
Component: upload.module » file.module
Issue tags: +html5

Moving to 8.x as bugs are fixed in dev and backported where appropriate.

Does this happen with image / file fields in Drupal 8?

I am not familiar with the html5 requirements for this, but the validator is experimental and not to be trusted (it doesn't move as quickly as the spec). As far as I can tell this is the requirement http://www.w3.org/TR/html5/urls.html

frank0051’s picture

Is that not what RFC3986 requires - which is supposed to be adhered to in HTML5 (see your link above). Shoot me if I'm off base, haha. Based on my reading (and also what I have seen on other sites), spaces are considered unsafe and must be escaped to meet the standards. Example of such documentation: http://www.tutorialspoint.com/html5/html5_url_encoding.htm

PHP already has a function for it and Stackoverflow touches on it (and the HTML5) requirement here: http://stackoverflow.com/questions/4768262/how-can-i-make-wordpresss-php...

Everett Zufelt’s picture

Can someone please confirm that file URLs are still generated with spaces in 8.x?

frank0051’s picture

Well, I don't have 8.0; I only have access to 6 and 7; you are the one that changed it to 8. It appears Drupal 7.x replaces the white space with %20. So, again, this should only be a Drupal 6 problem unless Drupal 8 reverted its handling of files. So, do you want to change it back to a version 6.x?

Everett Zufelt’s picture

As long as we know that head 8.x isn't exhibiting the same issue then we can set it back to 6.x (bugs are fixed in 8.x and then backported where necessary). I assume it is fixed in 8.x if it is fixed in 7.x.

Fixing the bug in 6.x might be difficult. At the most we would want to fix the URL generated for all new uploads, or we * could * break the URLs used / saved that reference the old files.

frank0051’s picture

Is there an 8.x sandbox or something I could test out without having to install it onto my own server? I can try it there if that is a possibility.

Everett Zufelt’s picture

Version: 8.x-dev » 6.x-dev
Component: file.module » upload.module

Tested and 8.x also replaces spaces with properly. So this is a 6.x issue.

Not quite sure the best way to go about solving. For the most part 6.x sites aren't going to be attempting to validate against html5. And, changing the file path for files already uploaded will possibly break existing links.

Ideas?

Damien Tournoud’s picture

Title: Upload links are not HTML5 compliant » file_create_url() fails to encode paths
Component: upload.module » file system

The problem is file_create_url() in Drupal 6. We are missing a drupal_urlencode() in the FILE_DOWNLOADS_PUBLIC case.

frank0051’s picture

Well, I'm not sure Drupal should worry about fixing links that were already uploaded [since it isn't technically a bug]. Rather, lets just make sure that it is fixed for future uploads with D6. Then, if someone else posts on this subject after the patch is included in the next D6 release, individuals can just tell that person to re-upload the files. Just because it is D6 doesn't mean the community shouldn't try its best to future proof it - and, part of that includes readying it for HTML5.

Just my two cents.

Everett Zufelt’s picture

Taking a look at theme_upload_attachments(), it appears that files are not stored with their URL, but that the URL is generated from file_create_url() on the theme layer. This means that any changes to file_create_url() to add the encoding would change how all uploaded file URLs are rendered. This might break saved links.

Can someone confirm that a file path encoded with drupal_urlencode() is able to be accessed from the old URL that is not encoded? I expct it can be, and that it shouldn't be a problem to make the change.

frank0051’s picture

If drupal_urlencode() simply replaces white space with %20 escape characters than it shouldn't matter. Ie9 and Opera already convert whitespace to escaped characters themselves. I imagine the other browsers can handle it as well.

Everett Zufelt’s picture

Please do not 'bump' an issue. This is an open source community with volunteers. I know that for myself if I see an issue getting 'bumped' I am less likely to work on it.

frank0051’s picture

Well, unfortunately, I have found some issues go weeks or even months without reply if they aren't bumped. While I understand your point of view and appreciate all the hard work volunteers put into the project, sometimes I find it helps to just bring this back to the top of people's list. Sometimes people get busy and overlook things.

Damien Tournoud’s picture

I described what we need to fix in #13. Now we need someone to contribute a patch!

See http://drupal.org/patch/create for more information.

frank0051’s picture

Damien, I'll give it a try in the next hour or two; I've never done one before and I might break it, so you guys will have to check it, haha. I'm just layman.

frank0051’s picture

Reading through the directions, I'm definitely not comfortable with trying to create a patch. But here is what I think needs to happen.

In includes/file.inc, there is this function

function file_create_url($path) {
  // Strip file_directory_path from $path. We only include relative paths in urls.
  if (strpos($path, file_directory_path() .'/') === 0) {
    $path = trim(substr($path, strlen(file_directory_path())), '\\/');
  }
	
  switch (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC)) {
    case FILE_DOWNLOADS_PUBLIC:
      return $GLOBALS['base_url'] .'/'. file_directory_path() .'/'. str_replace('\\', '/', $path);
    case FILE_DOWNLOADS_PRIVATE:
      return url('system/files/'. $path, array('absolute' => TRUE));
  }
}

I think you just need to add one line of code (bolded)

function file_create_url($path) {
// Strip file_directory_path from $path. We only include relative paths in urls.
if (strpos($path, file_directory_path() .'/') === 0) {
$path = trim(substr($path, strlen(file_directory_path())), '\\/');
}
$path=drupal_urlencode($path);
switch (variable_get('file_downloads', FILE_DOWNLOADS_PUBLIC)) {
case FILE_DOWNLOADS_PUBLIC:
return $GLOBALS['base_url'] .'/'. file_directory_path() .'/'. str_replace('\\', '/', $path);
case FILE_DOWNLOADS_PRIVATE:
return url('system/files/'. $path, array('absolute' => TRUE));
}
}

frank0051’s picture

Anyone think they can develop a patch using the suggestion I made above?

cweagans’s picture

Instead of continually bumping the issue, why don't you develop a patch? There's a first time for everything, this is an easy change, and documentation for creating a patch was already posted.

And seriously, don't bump issues like that. It just annoys people. If you want it done quicker, do it yourself or hire someone to do it for you.

frank0051’s picture

I spent 8 hours attempting to do simple CSS over the last two days, you really don't want me to attempt to develop a patch. And, I'm not a developer. I realize this is an open source community, so I'm contributing in what ways I can. I have brought the issue to your attention and I laid out exactly where you need to insert the ONE line of code that I think is needed. All you have to do is "patch-ify" it and test it.

And, I'm going to keep bumping when something doesn't get a reply for over a week or two. The last time I bumped before the most recent one was 23 days before that. I'm regularly used to having Drupal issues go unsolved, so I'm trying something new. It's actually working better than I had hoped. Now, if I were an ass, I would say why don't you do it sense you appear to know how instead of lecturing me. I'm willing to bet you could have done it in the time you spent lecturing me. Meanwhile, I would be taking 7 hours just to figure out how to draw a CSS box.

cweagans’s picture

I realize this is an open source community, so I'm contributing in what ways I can. I have brought the issue to your attention and I laid out exactly where you need to insert the ONE line of code that I think is needed. All you have to do is "patch-ify" it and test it.

You misunderstand how this works. I don't have to do anything. If this is affecting you, then do something about it. Read the patch documentation posted in #20, follow the directions, and post a patch. The worst thing that can happen is that it doesn't work. There was a first time for all of us, but the important thing is that you try. Plus, it helps that you've already done all the hard work (figuring out what code needs to go where).

And, I'm going to keep bumping when something doesn't get a reply for over a week or two.

Unless it's a critical issue (which this is not), it's extremely bad form to bump issues. It might be working for you, but it's just because people get annoyed enough to comment on the issue again. The entire reason that the "Follow" button was implemented on Drupal.org was that people were tired of one-word "subscribe" comments: they didn't add anything but noise to the discussion. Bump comments are along the same lines. Plus, it comes off as "I want somebody else to fix this for me". While that may be the case, we're all volunteers and most of us don't like being told what we're going to work on. The only reason I saw this post in the first place was that somebody (I won't say who) was annoyed with you on Twitter.

I'm unsubscribing from this thread now. If you'd like to try your hand at contributing a patch, feel free to reach out to me or anyone on this issue via the Drupal IRC channels: I'm certain that if you say "I'm interested in making a patch for issue 1277140, can somebody walk me through the process?" on IRC, you'll get lots of people lining up to help you. Good luck.

frank0051’s picture

You misunderstand how this works. I don't have to do anything. If this is affecting you, then do something about it. Read the patch documentation posted in #20, follow the directions, and post a patch. The worst thing that can happen is that it doesn't work. There was a first time for all of us, but the important thing is that you try. Plus, it helps that you've already done all the hard work (figuring out what code needs to go where).

That was a YOU as YOU the community. Context clues buddy. And, no, I don't have another 7 hours to exhaust like I did over Thanksgiving Break. Perhaps you may have that type of time, but I work 55-60 hour weeks along with managing the various side projects that take up another 15-25 hours in any given week.

Bump comments are along the same lines. Plus, it comes off as "I want somebody else to fix this for me". While that may be the case, we're all volunteers and most of us don't like being told what we're going to work on. The only reason I saw this post in the first place was that somebody (I won't say who) was annoyed with you on Twitter.

Damn straight I want someone else to fix it. I've identified the issue, provided the code, now I want some experienced people to write it and help test it in order to not mess it up. Open source communities seem to be behind the curve when realize not everyone that uses their software is a developer and some people would like some good ol' fashion customer support, but will contribute where they can to the cause.

chx’s picture

frank0051, at your next bump I we will ban you. You will not receive any more warnings. Do not answer to this comment.

Edit: I deleted all your bumps from this and other issues.

frank0051’s picture

See my reply to your issue and maybe you will understand why: http://drupal.org/node/1374846#comment-5378108

This thread has been open since September. The solution has basically when proposed, all I need someone to do is "patch-ify" it. That's literally all that remains. The other issue (JS for Opera) is literally a core issue that has made my three installs of Drupal useless because Javascript doesn't work properly in the core of 6.x.

frank0051’s picture

I will pay anyone, right now, $20 to patch-ify that solution in this thread and help test it. I've already done the heavy-lifting when it comes to figuring out what the problem is and IDing the PHP code.

Everett Zufelt’s picture

Status: Active » Needs review
FileSize
614 bytes
frank0051’s picture

I'll play with this tonight. Just find it funny that this is something that was probably 30 minutes of work for you (especially since you have already commented on this thread), and it wasn't the fact that this was a core usability issue that motivated you, but rather money. That's a sad statement on the open source community that has developed here at Drupal. In any case, I'm a man of my word; message me your Pay-Pal address and if this works I'll send you the money.

Everett Zufelt’s picture

@Frank

Although your comment is off topic, and a little insulting, I will comment here.

As a contributor ranked by CertifiedToRock.com as being in the 99th percentile, and a contributor to other communities as well, like the HTML WG, I give far more than the average individual back to the open source world. Nevertheless, the last time I asked my landlord if I could get credit toward rent by showing a commit log, he said no.

Furthermore, I did not roll this patch for money, although I'll take that, but because I thought that this was evidence of you actually showing respect to the community process (i.e. do it yourself, wait for someone to do it for free, or pay someone to do it for you).

frank0051’s picture

Status: Needs review » Needs work

Again, I go back to this is a CORE USABILITY ISSUE. The whole point is it shouldn't have taken over 4 months to get solved, it shouldn't have resulted in a threat to ban me, and it shouldn't have required money to solve. Are you individually to blame, of course not, but the community as a whole (myself included) should be ashamed that a core issue like this could go outstanding for so long. And, BTW, Drupal is still difficult to Opera users because of the JS error in the other thread I have mentioned.

That being said, the patch does not appear to work correctly. Instead of generating http://www.croomsalumni.com/sites/default/files/Historical%20Listing%20o...

it generates

http://www.croomsalumni.com/http%253A/%25252Fwww.croomsalumni.com/sites/...

Thank you for your attempt Everett, but unfortunately I can not pay for a solution that does not work.

Everett Zufelt’s picture

Status: Needs work » Needs review
FileSize
616 bytes

Goes to show you should read the docs before writing a patch.

frank0051’s picture

Works perfect, I'll send payment in 5 minutes. How do we get it committed?

Everett Zufelt’s picture

@Frank

There isn't really a hard and fast guideline. Normally a couple people, who didn't write the patch, review the patch. If it is found to be working and not causing any problems someone sets the status to RTBC. This is a pretty straight forward patch, literally modifying a single line, wih highly predictable results, so it would be little work for another person to give a quick review.

Once the issue status is RTBC someone with permission to commit to the 6.x branch will take a look, and if they agree they will apply the patch and commit the changes. It will then be part of the next Drupal 6.x release.

As this is an issue that clearly very few people have found important then I don't expect to see that happening immediately, but I expect that it will happen in a reasonable timeframe.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Pretty straightforward.

Damien Tournoud’s picture

The patch matches my spec from #13. I support committing to 6.x. As previously stated, 7.x and 8.x are not affected.

hass’s picture

Title: file_create_url() fails to encode paths » file_create_url() creates invalid public paths

I have tested this patch and it fixes #1525146: Space in filename triggers 404. Thanks for sharing. RTBC.

Why is this patch not committed to D6 after 1 year of being RTBC? Bumping for commit.

hass’s picture

Priority: Normal » Critical

Marking as release blocker for next D6.

hass’s picture

2 years bump. Is Drupal really maintained?

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 40: 1277140-2.patch, failed testing.

asb’s picture

Issue summary: View changes

Could someone please update the status of this issue? Is it RTBC, as #45 reports, or is it not, as #48 suggests?

Status: Needs work » Needs review

hass queued 40: 1277140-2.patch for re-testing.

hass’s picture

Status: Needs review » Reviewed & tested by the community

Just a broken testbot, nothing else.

frob queued 40: 1277140-2.patch for re-testing.

Status: Reviewed & tested by the community » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.