Add binary font file types to .gitattributes

Thanks for the patch! I think the proper fix is for Drupal to stop treating arbitrary files as text files. That is trying to be achieved in #2333243: Relax .gitattributes global rule. Thus, I am closing this as duplicate, because I think your problem would be fixed by that issue as well. Please re-open in case I'm incorrect.

@tstoeckler -- #2333243: Relax .gitattributes global rule has been committed and does not address this problem. Tested today. Attaching new patch to include .eot files as well.

I don't see a problem with this patch since they're probably common enough for site builders but I wasn't able to recreate it. For the sake of understanding the problem, is it possible you've got a global setting that it defaulting everything to text?

@neclimdul If there's a global setting somewhere I can not find it. See https://www.drupal.org/node/2333243#comment-11448493 for test methodology.

Lets discuss here.

$ drush dl drupal-8.2.x --drupal-project-rename=drupal-8.2.x-2333243
$ cd drupal-8.2.x-2333243
$ curl https://fonts.gstatic.com/s/roboto/v15/CWB0XYA8bzo0kSThX0UTuA.woff2 --create-dirs -o themes/junk/fonts/Roboto.woff2
$ git init -q && git add -A && git commit -q -m "Initial commit."
$ truncate -s +10 themes/junk/fonts/Roboto.woff2
$ git add -A && git commit -q -m "Added 10 bytes to the end of Roboto.woff2"
$ git diff HEAD^ HEAD

The resulting diff simply indicates that the files differ -- this is not the desired behavior with binary files.

$ echo "*.woff2 -text diff" >> .gitattributes
$ git diff HEAD^ HEAD

The resulting diff is correct.

I've attached a bash script if you'd prefer to do it the easy way.

So both seem to be treating the file as binary and I would expect not corrupting the file. The difference seems to only be triggering Include binary diff in patches instead of "binary files differ."

Regarding #10, correct. And this renders the patch for a binary file useless.

Again not against this, but the git process for files like this is to add --binary option to your git diff command.

There isn't any corruption and this is a standard git behavior so pull it down to minor.

I suppose we have explicity specified other binary file types as binary in the .gitattributes file as a matter of convenience, so that we do not have to remember to use the --binary option when our changes include binary files.

It becomes even more convenient, and mitigates errors, when both binary and text files are added or modified. For example, I might make several changes to a theme: include a new font file, modify an existing image, add a new css file, and modify an existing template. Some of the files are binary, and some are not.

This became an annoyance when I ran into problems with this patch:
https://www.drupal.org/node/2716557#comment-11284831

It is malformed, and the changes include both binary and text files.

I agree this is minor, but an annoyance nevertheless!

We manually specify a number of different image file formats as binary. This is great, because it avoids everybody in the world knowing to do git add --binary. I guarantee you, most of the folks who will need to do this (front-end developers and site builders) do not know they need to do this, and expecting that seems wrong. Font files are pretty commonly needed for sites. What's the harm in adding this to gitattributes so that this is an easy thing and not a hard thing?

The alternative is that projects will not use core's gitattributes file and will instead override it, which means any future updates to gitattributes won't get applied to sites. So... better to be friendly and add .woff, woff2, and .eot to the list of binary files. Particularly because we already have .ttf, which is also a font file.

If we're able to backport this to 8.2.x and even 8.1.x, that would be great. It seems harmless enough.

Agreed, fonts easily become corrupted with git. If new versions are added and fonts are not treated as binary, you will see the following in console..

OTS parsing error: Failed to convert WOFF 2.0 font to SFNT
(index):1 Failed to decode downloaded font

If *.ttf -text diff (TrueType) is included now then webfonts using todays web standards should be as well.

Adding .otf as well. Ran into that one today.

Still needs review by someone other than me, then change status to RTBC.

The patch certainly applies and does what it's supposed to do. Do we need a subsystem maintainer's sign-off?

Please alphabetize the attributes then I see nothing blocking this if it makes lives easier. Thank you.

Hi Meenakshi Gupta:

Thank you for helping out. We use needs work when there is a patch needing changes.

@cilefen after alphabetizing the attributes

@Meenakshi, please reverse png and phar.

@toomanypets done !

Perfect, thanks @Meenakshi Gupta !

Let's get this in.

Thanks @Meenakshi Gupta for your persistence. You really hit some bad luck there. Still good to go.

random failure, green again.

Random fail in Drupal\KernelTests\Core\Cache\DatabaseBackendTest.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!