Display image files

[KuiKui]

Does it fix this issue : #119 ?

[ratibus]

ping @pmartin

[pmartin]

@KuiKui I would say (according to your screenshot, at least ; I didn't test) that it does indeed fix #119 (for images -- which is the kind of binary file we have the most).
Thanks !

[KuiKui]

I need your help to review my code : I'm not sure that is the correct way to render images...
With this method, many images are corrupted (even on the screenshot).
Can you test it on real data ?

[KuiKui]

Ping @pmartin @ratibus

[srogier]

le rendu des écrans Retina est sympa

[pmartin]

(sorry for those who only read english ; what follows is in french -- as a summary, I wonder if this image corruption might not be caused by end-of-line normalization)

Je ne prétend pas avoir de solution, mais je pense avoir trouvé une cause possible.

A titre d'exemple, je bosse avec une image en PNG ; en regardant ce à quoi elle ressemble avec un éditeur hexa, elle commence par ceci :

89 50 4E 47 0D 0A 1A 0A 00 00 00 0D 

Maintenant, si je modifie ImageUtils::getImageTypeFromContent() en ajoutant la portion de code suivante tout en haut de la méthode (j'aurais probablement aussi pu me brancher sur $newBinaryContent dans fileAction.class.php) :

for ($i=0 ; $i<12 ; $i++) {
  $char = $binaryContent[$i];
  echo bin2hex($char) . ' ';
  //var_dump(substr($binaryContent, 0, 10)); die;  
}
die;

J'obtiens le dump suivant :

89 50 4e 47 0a 1a 0a 00 00 00 0d 49

Si je met les deux dans le même bloc de code, pour faciliter la comparaison (première ligne = ce que j'ai via éditeur hexa ; seconde ligne = ce que j'ai via mon bout de code branché en plein milieu de Crew) :

89 50 4E 47 0D 0A 1A 0A 00 00 00 0D 
89 50 4e 47 0a 1a 0a 00 00 00 0d 49

Autrement dit : l'enchainement 0D 0A (retour à la ligne "Windows") que j'ai dans mon fichier PNG lorsque je le regarde à coup d'éditeur hexa ne se retrouve pas dans la sortie obtenue depuis le code PHP de Crew : il est vu par Crew comme 0A (retour à la ligne "Linux") uniquement.

J'ai donc un octet qui disparait en plein milieu de mon flux binaire ; avec deux conséquences :

• Cet octet qui disparait en tout début de fichier PNG fait parti de ce que tu as utilisé comme signature pour identifier un fichier PNG ('png' => "\x89\x50\x4e\x47\x0d\x0a", dans le code) ; en conséquence, mon fichier PNG n'est pas reconnu par Crew comme étant un ficier PNG (il n'est pas reconnu comme étant un fichier binaire, en fait ; je ne sais pas si c'est lié -- file.is_binary était à 0 en DB)
• Cela entraine un décalage dans le flux binaire, puisqu'une séquence de deux octets est remplacée par un seul octet ; je n'ai plus en tête exactement la structure selon laquelle un JPEG (comme l'image utilisée dans ton screenshot plus haut) est enregistrée, mais il y a des chances que ce décalage d'octets dans le flux binaire puisse se traduire par le genre de corruption que tu constatais.

Je ne sais pas comment empécher ce remplacement de 0D0A en 0A, qui doit être fait plus ou moins automatiquement par Git, mais est-ce que ça ne correspond pas à une fonctionnalité (après tout, à la base, Git est surtout fait pour bosser sur du texte, et pas sur des fichiers binaires) paramétrable par core.autocrlf ?

Sinon, il y a peut-être moyen de jouer avec .gitattributes ? Mais c'est encore plus loin dans les trucs que je n'ai jamais manipulé...

En vrac, deux-trois liens sur lesquels je suis tombé (je n'ai pas eu le temps de tester plus) :

• Dealing with line endings
• Best way to disable Git end-of-line normalization (CRLF to LF)

Sinon, côté "revue de code", deux/trois notes rapides : - La détection des types de contenus via comparaison des quelques premiers octets : bon, y'a de bonnes chances que ça marche à quasiment tous les coups ; mais dans le principe, je me serais plutôt attendu à voir du [fileinfo](http://php.net/manual/en/function.finfo-file.php) ; ou à la limite du [`mime_content_type()`](http://php.net/manual/en/function.mime-content-type.php) si on veut toujours que Crew soit compatible PHP < 5.3. - Passer par des images "intégrées" _(src="data:image/...;base64,...")_ : est-ce que ça ne risque pas d'être un peu lourd pour de grosses images ? _(c'est une question, je n'ai pas la réponse)_ et ça ne marche pas sous des vieux navigateurs _(genre IE6 -- ce qui ne me dérange aucunement ^^ )_ ; mais je comprends le "pourquoi" : ça permet d'éviter la mise en place d'une nouvelle action, qui renverrait le flux binaire. - Cela dit, une nouvelle action renvoyant le flux binaire, ça pourrait à terme être utilisé pour d'autres types de fichiers binaires que les images, pour pouvoir les télécharger/ouvrir ; genre pour des PDF par exemple _(utilité pas gigantissime non plus : dans le principe, on n'est pas tellement censés mettre des fichiers de ce type sous git)_.

[KuiKui]

@ratibus Comment puis-je utiliser la fonction getimagesize avec un flux binaire directement dans une chaine (sans créer de fichier temporaire) ?

[ratibus]

En utilisant getimagesizefromstring ? :p

[KuiKui]

Ok...
RTFM 👊

[KuiKui]

@pmartin Ça te dirait de reprendre cette PR pour la mener à terme ? 😏

[pmartin]

Vais essayer de regarder ; pas "tout de suite", mais week-end prochain ou semaine prochaine, si je trouve un peu de temps :-)
(au pire, harcèle moi si j'ai pas donné de nouvelle d'ici une semaine ou deux)

[KuiKui]

👌

[KuiKui]

Ping @pmartin 😄

[pmartin]

@KuiKui > I have just pushed two commits to my fork => https://github.com/pmartin/Crew/commits/display-image-files

Those should help with :

• detecting if a file is binary, when doing the review request (The PNG image I was using wasn't detected as a binary file) ; basically, I removed a call to sed, and replaced by some PHP code.
• displaying binary files properly.

The binary content that was gotten from git show was broken : the PHP code was using the exec() function, imploding the lines it return -- but, as http://www.php.net/manual/en/function.exec.php says :

Trailing whitespace, such as \n, is not included in this array.

Those \n removed from the output could break binary contents.

I've added a new GitCommand::execReturnRaw() method, that uses passthru() to not alter the output of git show in any way ; and GitCommand::getShowFile() now uses that one.

I have not changed the way images files are detected in ImageUtils :

• The getimagesizefromstring() function @ratibus proposed requires PHP >= 5.4.0 ( http://www.php.net/getimagesizefromstring ), and, as such, is not OK with the current "PHP 5.2" requirement of Crew.
• The current way "kind of" works ; and could be improved later if necessary ?

What's the next step : do I do a PR from my fork/branch ?
(I hope I didn't break anything ^^ I would suggest some tests might prove useful ;-) )

[KuiKui]

Cool !
Par contre, tu pourrais reprendre cette PR, histoire de garder l'historique ?
(donc juste mettre tes 2 commits sur cette branche)

[pmartin]

Hello,
Heu ; comment est-ce que je peux mettre à jour une PR faite par toi ?
Vu que je suis parti de ta branche pour créer la mienne et que j'ai ensuite uniquement rajouté quelques commits, est-ce que ce n'est pas toi qui peu re-merger la mienne dans la tienne, et re-faire la PR pour la mettre à jour ?

[KuiKui]

Ayé.
ping @ratibus @srogier @agallou pour review 😛

[KuiKui]

re-ping @ratibus @srogier @agallou pour review ? 😸

[srogier]

si tu mets des chats dans tes demandes de revue de code, ça va pas le faire :o

[srogier]

initialiser ces paramètres ? pour toujours les avoir même quand on ne passe (même si dans la template on semble passer par une condition équivalente sur getIsBinary)

[srogier]

et puis, c'est pas un peu triché cette detection par rapport au message git pour déterminer si le fichier est nouveau :o ?

[KuiKui]

@pmartin tu peux valider le fonctionnement de cette branche en interne ?

[KuiKui]

@pmartin ça dit quoi ? 😄