template_preprocess_file_link() should not add microformats attribute

Patch applies cleanly to 9.1.x.

Thanks for the issue and patch.

1) Code changes are straightforward.

2) Marking for manual testing.

3) Kicking of tests for 9.1.x.

Need a patch for 9.1

I'm working on it, will update in few hours.

Rerolled the patch

Tested manually in my system, seems to be working file.

For historical context, this is from git blame:

ac30bf8a82a modules/file/file.module      (Dries Buytaert      2009-08-29 12:52:32 +0000 1458)   // http://microformats.org/wiki/file-format-examples
0413834556f core/modules/file/file.module (Nathaniel Catchpole 2014-11-10 14:30:41 +0000 1459)   $options['attributes']['type'] = $mime_type . '; length=' . $file->getSize();

so the comment is from 2009 and the code was last updated in 2014 trivially as so:

+  $mime_type = $file->getMimeType();
   // Set options as per anchor format described at
   // http://microformats.org/wiki/file-format-examples
-  $options['attributes']['type'] = $file->getMimeType() . '; length=' . $file->getSize();
+  $options['attributes']['type'] = $mime_type . '; length=' . $file->getSize();

I'm trying to understand why this should be removed. The issue summary doesn't explain why, just that it should be removed.

I have tested with and without the patch and it works as expected. I'm just unclear why it should be removed. I see it defined here as well: https://www.w3.org/TR/html4/struct/links.html#adef-type-A and https://www.w3.org/TR/2011/WD-html5-author-20110809/the-link-element.htm....

type
Hints at the linked URL’s format with a MIME type. No built-in functionality.

See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a

A MIME type (now properly called "media type", but also sometimes "content type") is a string sent along with a file indicating the type of the file (describing the content format, for example, a sound file might be labeled audio/ogg, or an image file image/png).

See https://developer.mozilla.org/en-US/docs/Glossary/MIME_type

Per the docs for the a tag, I think <a href="URL" type="text/plain">A file</a> is good. But <a href="URL" type="text/plain; length:1024">A file</a> with length appended is not good. 1) length is not a part of type obviously, and 2) maybe it's not good for security leaking the file size? -- one can see the url, but no access to the file.

So to me, this issue could be removing the length metadata from the type. Removing the whole type attribute does not make sense to me.

To clarify a bit. The problematic output is not the microformat. It is, I believe, an alternate solution highlighted in the page linked in the git blame. In any case, I agree that adding file details into the type field is not likely to be expected by anyone, and agree that it should not be added by default. As @jungle mentions, there is nothing wrong with including the type parameter with a mimetype.

Looking at the w3c HTML 5 working draft for web authors (https://www.w3.org/TR/2011/WD-html5-author-20110809/links.html#hyperlink) I find the following text:

The type attribute, if present, gives the MIME type of the linked resource. It is purely advisory. The value must be a valid MIME type.

So clearly the file size and other info doesn't belong there.

I guess the main question left about this issue is whether the type should be inserted there or not.

Personally I think if we have the mimetype there already we may as well use it. I'll upload a tiny change to the patch to reenable the mimetype shortly.

Right. Well if we have two patches, one that removes all from above and a new one that just removes the length then the core maintainers could choose which patch to use. Thanks to you both for the input.

Adding the mime-type back in without the file size.

Sorry, I got my comment number wrong, but exactly as #20 mentions. We now have the choice of two approaches.

Thanks for the patch. Marking RTBC since:

1) Code looks fine.

2) Tests pass.

3) Works as expected. See screenshot.

4) Maintainers can choose between patch in #12 that removes the type completely or #21 which only removes the length data.

Whoops.

A known random fail. Re-queued.

Can we get an issue summary update detailing why we are doing this? It's not clear to me from reading the comments.

The microformats wiki still has examples that reference file size

I'm not sure but, if I'm understanding this correctly, then only Firefox has built-in support and some other browsers support it with an extension:

http://microformats.org/wiki/browsers

Unless I’m completely misunderstanding the page at http://microformats.org/wiki/file-format-examples, this is not even a micro format as proposed by micro formats.org. That is either how it has been observed having been done or one of the existing suggestions for how it could be done. The example of this format is in the other attempts to solve the problem section, not any kind of micro format definition.

In short, I don’t think this format is even a defined micro format, and I’m not sure why it ever got added. Type is an officially defined HTML parameter for a tags. The definition says that if it is there, it should contain the mime type. There seems to be no wiggle from for a file micro format to be defined using that parameter.

As a spot check, I did a quick browse through
the php-mf2 Codebase including the list of mf1 formats it parses, and could find no evidence that it would pick up and parse the type parameter as a micro format at all.

@ultrabob Thanks for the info. Would you update the issue summary with the justification for switching this? Sounds like you have a good handle on it. :)

Thanks @ultrabob, applying the IS template and setting back to RTBC.

As we're removing something here, I think we need to communicate this via a change record.

Tagging as such.

Added change record: https://www.drupal.org/node/3163231

Thanks for the change record.

1) Note that the change record is for just removing the size and not removing the type.

2) Assuming that's ok, I'm not sure if change records ever include quotes in them but I'll leave that. I did add an example.

Back to RTBC.

@Kirsten Pol -- I only added it as a quote because I thought it summed up the justification perfectly, and didn't want to take credit for that wording :-)

Agreed, great quote :) just don't know if it's customary or not in change records.

Crediting @jungle for taking part in the discussion and @partyka for the change notice.

Committed d7d9ebd and pushed to 9.1.x. Thanks!

Amended and published the change record.

Because there is a slight chance of disruption here (someone may be depending on this feature, as odd as it is), not going to put this forward for backport. In addition, its only a minor bug, so there's not huge demand for it to be backported.