Display image files #126

KuiKui · 2013-01-13T11:43:35Z

Does it fix this issue : #119 ?

ratibus · 2013-01-14T09:18:09Z

pmartin · 2013-01-14T09:33:48Z

@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 · 2013-01-15T07:28:53Z

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 · 2013-01-18T08:40:08Z

Ping @pmartin @ratibus

srogier · 2013-01-18T09:58:17Z

le rendu des écrans Retina est sympa

pmartin · 2013-01-22T05:59:04Z

(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) :

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 · 2013-02-07T09:24:10Z

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

ratibus · 2013-02-07T10:11:37Z

En utilisant getimagesizefromstring ? :p

KuiKui · 2013-02-07T10:15:42Z

Ok...
RTFM 👊

Email notifications added

KuiKui · 2013-03-27T14:06:18Z

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

pmartin · 2013-03-27T14:21:38Z

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 · 2013-03-27T14:27:02Z

👌

KuiKui · 2013-04-05T09:21:04Z

Ping @pmartin 😄

…y-image-files

Images (at least PNG) were not detected as binary files. Removed the sed call from getDiffFilesFromBranch(), and replaced it with a PHP explode on '\t' instead of ' '.

Added a execReturnRaw() method, to execute a shell command and return its output, without modifying / breaking it. It uses passthru(), which doesn't modify the output, as opposite to exec(), which ignores trailling whitespaces -- which means binary files' content gets broken when using 'git show' with exec(). Updated getShowFile() to use the new passthru()-base execReturnRaw() method, instead of the exec()-based one.

pmartin · 2013-04-06T13:56:36Z

@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 · 2013-04-08T07:31:34Z

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

pmartin · 2013-04-20T14:16:25Z

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 · 2013-04-23T13:20:51Z

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

KuiKui · 2013-04-29T15:02:07Z

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

srogier · 2013-04-29T15:04:35Z

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

srogier · 2013-04-29T15:07:54Z

apps/front/modules/default/actions/fileAction.class.php

-      ->filterByType(CommentPeer::TYPE_LINE)
-      ->find()
-    ;
+        $this->oldImageExists  = !(strpos($oldBinaryContent, 'fatal: Path') === 0);


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)

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 · 2013-05-24T07:19:40Z

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

KuiKui · 2013-05-31T07:29:15Z

@pmartin ça dit quoi ? 😄

KuiKui added 2 commits January 12, 2013 08:07

remove unused variable

9a656fc

handling of some types of binary files : gif, jpeg, png and bmp

f08ed61

Merge pull request crew-cr#132 from quochuy/develop

39ca82f

Email notifications added

pmartin added 3 commits April 5, 2013 19:14

Merge remote-tracking branch 'kuikui/display-image-files' into displa…

a6c5bd4

…y-image-files

Fix detection of binary files

6da5ada

Images (at least PNG) were not detected as binary files. Removed the sed call from getDiffFilesFromBranch(), and replaced it with a PHP explode on '\t' instead of ' '.

srogier reviewed Apr 29, 2013
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display image files #126

Display image files #126

KuiKui commented Jan 13, 2013

ratibus commented Jan 14, 2013

pmartin commented Jan 14, 2013

KuiKui commented Jan 15, 2013

KuiKui commented Jan 18, 2013

srogier commented Jan 18, 2013

pmartin commented Jan 22, 2013

KuiKui commented Feb 7, 2013

ratibus commented Feb 7, 2013

KuiKui commented Feb 7, 2013

KuiKui commented Mar 27, 2013

pmartin commented Mar 27, 2013

KuiKui commented Mar 27, 2013

KuiKui commented Apr 5, 2013

pmartin commented Apr 6, 2013

KuiKui commented Apr 8, 2013

pmartin commented Apr 20, 2013

KuiKui commented Apr 23, 2013

KuiKui commented Apr 29, 2013

srogier commented Apr 29, 2013

srogier Apr 29, 2013

srogier Apr 29, 2013

KuiKui commented May 24, 2013

KuiKui commented May 31, 2013

Display image files #126

Are you sure you want to change the base?

Display image files #126

Conversation

KuiKui commented Jan 13, 2013

ratibus commented Jan 14, 2013

pmartin commented Jan 14, 2013

KuiKui commented Jan 15, 2013

KuiKui commented Jan 18, 2013

srogier commented Jan 18, 2013

pmartin commented Jan 22, 2013

KuiKui commented Feb 7, 2013

ratibus commented Feb 7, 2013

KuiKui commented Feb 7, 2013

KuiKui commented Mar 27, 2013

pmartin commented Mar 27, 2013

KuiKui commented Mar 27, 2013

KuiKui commented Apr 5, 2013

pmartin commented Apr 6, 2013

KuiKui commented Apr 8, 2013

pmartin commented Apr 20, 2013

KuiKui commented Apr 23, 2013

KuiKui commented Apr 29, 2013

srogier commented Apr 29, 2013

srogier Apr 29, 2013

Choose a reason for hiding this comment

srogier Apr 29, 2013

Choose a reason for hiding this comment

KuiKui commented May 24, 2013

KuiKui commented May 31, 2013