Bug #23348

Array returned by t3lib_stdGraphic::imageMagickIdentify() contains useless value for file extension

Added by Grigori Prokhorov almost 9 years ago. Updated 19 days ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Image Generation / GIFBUILDER
Target version:
-
Start date:
2010-08-04
Due date:
% Done:

100%

TYPO3 Version:
4.4
PHP Version:
4.3
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

The current version of the method t3lib_stdGraphic::imageMagickIdentify() does what it's supposed to do according to documentation.

Nevertheless the returned value for "file extension" is of no real use and ignores ImageMagick's identifying capabilities by returning a ereg result on the originally passed filename instead of extracting the actual file type determined by ImageMagick.

In other words - the documentation states, that imageMagickIdentify() "Returns an array where [0]/[1] is w/h, [2] is extension and [3] is the filename. Using ImageMagick".
The point here is, that [2] is filled with $reg0 which is created via ereg('([^\.]*)$',$imagefile,$reg);
While this works for images which provide a file extension as part of their filename, it obviously fails on files missing it.

This is the case when you try to identify an image e.g. before copying it to your uploads-folder in order to ensure it's a valid image file and thus run the check on a temporary file (which does not have an extension).

This issue could easily be solved by simply using the info ImageMagick provides by itself instead of relying on the filename.

A proposed patch is included with the report and should be checked for use.

The patch basically includes only two changes (line numbers according to r8200):

1. remove call to ereg on line 2672
2. change return array to use $splitinfo1 instead of $reg0.

The second change bases on a reliable assumtion, because ImageMagick's return string always starts according to the following syntax:

"fullFileName fileType imageDimension moreInfo_0 moreInfo_1 moreInfo_2 moreInfo_n"
e.g. rose.jpg JPEG 640x480 DirectClass 87kb 0.050u 0:01

This patch would make sure that the file type is always returned no matter what the filename is as long as the identification succeeds.

With kind regards,
Grigori Prokhorov

(issue imported from #M15347)

t3lib_stdGraphic-imageMagickIdentify-patch.php View (971 Bytes) Administrator Admin, 2010-08-04 12:32

Associated revisions

Revision 4e807a8a (diff)
Added by Benni Mack about 2 months ago

[BUGFIX] Deliver detected file type for IM identify

Although not used in TYPO3 Core directly, the identify
command by ImageMagick now delivers the file extension
from IM plus the file type identified by IM as well.

This saves some regexp magic, and a foreach() loop,
as the "-format" output delivers proper IM results.

As this functionality is not actually fixing a core
bug, but rather a small improvement and correction
in the identified usage, the change is targeted to master-only.

Resolves: #23348
Releases: master
Change-Id: Ia380ee34dbbef7f9f763ecc088ee0c9b83167ce6
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311
Tested-by: Sebastian Rosskopf <>
Tested-by: Jochen <>
Tested-by: TYPO3com <>
Tested-by: Anja Leichsenring <>
Reviewed-by: Sebastian Rosskopf <>
Reviewed-by: Markus Klein <>
Reviewed-by: Jochen <>
Reviewed-by: Anja Leichsenring <>

History

#1 Updated by Jigal van Hemert over 8 years ago

First of all the third item in the result array is defined as the file extension of the file which is identify-ied. This in formation is used in this way throughout the core.

It won't be a good thing to change it to the detected file type. We could however add an extra element to that array with this information.

I found no reliable way to implement this with all supported versions of ImageMagick and GraphicsMagick. Output of identify from the manuals of the various vesions:
IM 4.2.9: images/aquarium.miff 640x480 PseudoClass 256c 308135b MIFF 1s
IM 5.2 : images/aquarium.miff 640x480 PseudoClass 256c 308135b MIFF 1s
IM 6.6.7: rose.jpg JPEG 640x480 DirectClass 87kb 0.050u 0:01
GM : images/aquarium.miff 640x480 PseudoClass 256c 308135b MIFF 0.000u 0:01

More problems arrise when there are spaces in the image path:
D:/TYPO3/TYPO3 Core Sources SVN/trunk/typo3/gfx/fileicons/3ds.gif GIF 18x16+0+0 PseudoClass 64c 8-bit 370 0.000u 0:01
splitting on spaces will result in the file type "Core".

Recent versions of IM/GM know the option -format to set up the format of the output of identify. This way only the type and size could be shown, but the manuals of IM 4.2.9 and IM 5.2 did not mention this option.

Do you see a reliable way on all supported versions to get the file type information?

#2 Updated by Stefan Galinski over 7 years ago

  • Target version deleted (0)

Maybe there can be found an easy solution yet as the old IM versions are not supported anymore?

#3 Updated by Alexander Opitz almost 6 years ago

Hi,

as this issue is very old. Does the problem still exists within newer versions of TYPO3 CMS (4.5 or 6.1)?

#4 Updated by Grigori Prokhorov almost 6 years ago

Hi Alexander,

yes, the issue still exists unchanged in versions 4.5 troughout 6.1, see the following links:

As you can see, the code still relies on the return value of the regex for the file-extension.

Best regards,
Grigori Prokhorov

#5 Updated by Jigal van Hemert almost 6 years ago

Proposal: turn this into a feature for 6.2 and use the -format option of IM/GM to return only the necessary information. The detected file type can then be added to the array that is returned.
The rest of the core expects the file extension instead of the file type, so the only possibility is an extra item in the array as a new feature. Will that help?

#6 Updated by Grigori Prokhorov almost 6 years ago

Yes, I would also vote for Jigal's proposal to simply add the file type to the returned array and to gather the file type from a new method which implements an Image-/Graphics-Magick version-dependent extraction procedure for the file type from the string returned by identify.

Edit: On a second thought, if the versions < 6.2 can be considered as not patch-worthy in the context of this issue, we can use the -format option and save the effort of a new method.

Edit 2: All in all, Jigal's proposal is just fine, let's agree on this way, I don't believe there is another realistic option to adress this issue. Kudos!

#7 Updated by Alexander Opitz almost 6 years ago

  • Status changed from Needs Feedback to New

#8 Updated by Mathias Schreiber over 4 years ago

  • Target version set to 7.2 (Frontend)
  • Is Regression set to No

#9 Updated by Benni Mack almost 4 years ago

  • Target version changed from 7.2 (Frontend) to 7.4 (Backend)

#10 Updated by Susanne Moog almost 4 years ago

  • Target version changed from 7.4 (Backend) to 7.5

#11 Updated by Benni Mack over 3 years ago

  • Target version changed from 7.5 to 7 LTS

#12 Updated by Mathias Schreiber over 3 years ago

  • Target version deleted (7 LTS)

#13 Updated by Susanne Moog over 1 year ago

  • Category set to Image Generation / GIFBUILDER

#14 Updated by Sybille Peters about 1 year ago

Thank you for your report.

Even though it has been some time, would you consider checking if your patch idea is still up to date and upload it to our Gerrit review server?

Someone could do this for you, but I am thinking you might like the opportunity to contribute to TYPO3 yourself.

You can find a description of the TYPO3 contribution workflow here: https://docs.typo3.org/typo3cms/ContributionWorkflowGuide/

Hint: If you get stuck anywhere, ask on Slack in the #typo3-cms-coredev channel. You can register in the TYPO3 slack workspace here: https://forger.typo3.com/slack

Thank you in advance!

#15 Updated by Gerrit Code Review 2 months ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#16 Updated by Gerrit Code Review 2 months ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#17 Updated by Gerrit Code Review 2 months ago

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#18 Updated by Gerrit Code Review 2 months ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#19 Updated by Gerrit Code Review about 2 months ago

Patch set 5 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#20 Updated by Gerrit Code Review about 2 months ago

Patch set 6 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/60311

#21 Updated by Benni Mack about 2 months ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#22 Updated by Benni Mack 19 days ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF