Regression: images not showing with PHP versions where mime_content_type is deprecated

pgorod commented 8 years ago

Issue

Not my problem, I'm just copying over an issue from the forums: https://suitecrm.com/forum/suitecrm-7-0-discussion/10348-image-field-not-working

It seems that in this fix https://github.com/salesagility/SuiteCRM/pull/1509

Some code was introduced that causes this error in some systems: Call to undefined function mime_content_type() in /home/domain/public_html/crm/download.php on line 174 The browser shows an HTML error 500 on the console, and the image doesn't show.

It's confusing, from what I gathered, because mime_content_type was deprecated (around version 5.5?) but then was un-deprecated in 7.0. Cool, I didn't even know they did "undeprecations".

But we need to support some PHP versions that can't handle this code...

Expected Behavior

Image should show; code should run on all our supported PHP versions.

Actual Behavior

Image doesn't show on detail views, etc. Notice that now the problem is with every browser, not just IE and Edge like the original problem...

Possible Fix

There are a couple of useful StackOverflow questions with several alternatives.

Steps to Reproduce

See the forum post above. But essentially:

add an image field to Accounts or Contacts.
upload an image for a given record.
see that record's detail view.
Context

Your Environment

SuiteCRM Version used: 7.6 and 7.7
Browser name and version (e.g. Chrome Version 51.0.2704.63 (64-bit)): IE 9, Firefox and Chrome
Environment name and version (e.g. MySQL, PHP 7): PHP 5.6
Operating System and version (e.g Ubuntu 16.04): ?

604media commented 8 years ago

@pgorod Can you please enable php_fileinfo extension in your php.ini file and check.

Add the below line in your php.ini file

extension=php_fileinfo.dll

pgorod commented 8 years ago

@604media sorry, I can't... it's not my bug, I'm just reporting the experience of another user from the forums (see link at the beginning of the issue description).

But even if that fixes it, the point here is to make the code work out-of-the-box for everybody within the supported PHP versions group.

samus-aran commented 8 years ago

Hi @pgorod Depending on the OS of the original poster I'm pretty sure that this is a duplication of this issue.

https://github.com/salesagility/SuiteCRM/issues/1725

And as @604media is suggesting would resolve it. We do support that PHP version, but (un)surprisingly Windows seem to have different PHP packages and require extra steps because of those differences.

pgorod commented 8 years ago

@samus-aran he just answered on the forums that he's using Linux, so this theory about being a Windows problem doesn't seem to explain our issue.

Even if it was just a Windows problem, I wouldn't just close this. If we find something that is a potential pitfall for some users, we can try to catch the exception, provide a better message, work around the problem, or, if possible, simply avoid that pitfall for everyone. It is possible in this case...

pgorod commented 7 years ago

I have a case of this bug in front of me right now, and it's not on Windows, it's on Linux.

And PHP version is 7.0.18.

SuiteCRM 7.8.3, fresh install.

So I can't install the extension because it's a Windows extension, right? And this wasn't even supposed to be an error in PHP 7.0... what do I do now?

chris001 commented 7 years ago

This is the best solution, found by combining two very good posts. It continues to work on all versions of PHP, and whether or not the currently running PHP has the function mime_content_type available or if it becomes deprecated in the future.

// Thanks to http://php.net/manual/en/function.mime-content-type.php#87856

function getMimeContentType($filename, $ext)
{
    if(!function_exists('mime_content_type'))
    {
        if($mime_types = getMimeTypes())
        {
            if (array_key_exists($ext, $mime_types))
            {
                return $mime_types[$ext];
            }
            elseif (function_exists('finfo_open'))
            {
                $finfo  = finfo_open(FILEINFO_MIME);
                $mimetype = finfo_file($finfo, $filename);
                finfo_close($finfo);
                return $mimetype;
            }
        }
        return 'application/octet-stream';
    }
    return mime_content_type($filename);
}

// Thanks to http://php.net/manual/en/function.mime-content-type.php#107798

function getMimeTypes()
{
    $url = 'http://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types';

    $mimes = array();
    foreach(@explode("\n",@file_get_contents($url)) as $x)
    {
        if(isset($x[0]) && $x[0]!=='#' && preg_match_all('#([^\s]+)#', $x, $out) && isset($out[1]) && ($c = count($out[1])) > 1)
        {
                for($i=1; $i < $c; $i++)
            {
                    $mimes[$out[1][$i]] = $out[1][0];
            }
        }
    }
    return (@sort($mimes)) ? $mimes : false;
}

//Use it like this:

$filename = '/path/to/the/file.pdf';
$ext = strtolower(array_pop(explode('.',$filename)));
$content_type = getMimeContentType($filename, $ext);

pgorod commented 7 years ago

Thanks Chris, I will test this when I get a chance. The WannaCry ransomware has been making my life complicated in the last few days - and I don't even have any infected computers under my responsibility, I just have panicked humans (ok, and a server which decided to break and stop booting when I was patching it for the vulnerability)... :-)

pgorod commented 7 years ago

@chris001 @samus-aran I tested the fix and it works.

I suggest to reopen this issue (it is a bug, and it's not a Windows bug as suggested in the other similar issue), and I will make a PR next week when I get a chance.

I'm not sure where is the best place to put these functions. Maybe include/utils.php, but I'll have to check that download.php is loading that file, since it works a bit independently from the rest of the app. In my tests I just put the functions directly into download.php.

This breaks a lot of things - pictures in Users module is one thing. I have a complaint in this system I'm working on about images in Email Templates not working - I bet it's the same issue and I will inform as soon as I get this confirmed.

The PR will take some work because it needs to deal with all of these: https://github.com/salesagility/SuiteCRM/search?utf8=%E2%9C%93&q=mime_content_type&type=

pgorod commented 7 years ago

Another concern here is that this is issuing a web request in the middle of a simple auxiliary function. It's not good for performance but, more importantly, if the external site happens to be down, we have broken SuiteCRM systems everywhere.

Maybe we should think of an improved approach, like keeping a version of this file (http://svn.apache.org/repos/asf/httpd/httpd/trunk/docs/conf/mime.types) in the SuiteCRM package, but it would need to be updated regularly, and it's easy to forget doing that.

There's a composer package for this also, but I don't know to what extent it can be trusted to be kept updated... https://github.com/ralouphie/mimey

pgorod commented 7 years ago

@samus-aran I'm requesting reopening this and labeling as bug. @chris001 what do you think about that web request in your code, I believe we really need to avoid that, what do reckon could be a good way around this?

chris001 commented 7 years ago

It could download the file the first time, save it on disk, and only download it again when the version online is newer. Then use the file from disk.

pgorod commented 7 years ago

@chris001 Yeah, I would say we put the file in our package for the original install. This way we avoid having an extra step that must work for installs to succeed. And we cover those weird cases of people who have keep SuiteCRM offline all the time.

Then we could add a daily (or weekly) scheduler job called "Check for updates online". This would give the process more visibility for sysadmins. Maybe this file is all we need to fetch now, but this job could be used in the future for other things, like update notifications nudging admins to upgrade (when there are security releases, for example).

Then the function could just assume the file is there, and hopefully it's updated (except for people who are offline for too long, but that's mostly their problem, not ours).

chris001 commented 7 years ago

It makes the most sense to wait until they merge #3600 and #3237 into master, hotfix, develop, and feature.

There's no use wasting all that energy developing a PR and just to have it sit there because no human is available to physically test it.

Wait until we have the automated testing #3600 merged and running on all PRs, and Composer #3237 is finally managing the dependencies like all the modern PHP apps- including SugarCRM, since version 7.5 released in Dec 2014. http://support.sugarcrm.com/Documentation/Sugar_Developer/Sugar_Developer_Guide_7.7/Introduction/Composer/

pgorod commented 6 years ago

My suggestion for this would be to simplify in the following way:

download the mime types file and include it in our own source tree
use the code provided above to parse it

This will require regular updates, but not very regular. So I think we can just leave it static and whenever somebody in the future complains that they're having trouble with a file extension, we update the file. I think it will occur very rarely, 99.9% of the used extensions are already there.

This makes fixing the problem a very low effort task, I'd say one hour developer time.

pgorod commented 5 years ago

This just came up in the Forums again today...

https://suitecrm.com/suitecrm/forum/gestion-de-errores/20941-error-al-clickar-un-documento-subido

MedcenterOwner commented 5 years ago

Hi. For this configuration.

I'm using Suite CRM Versión 7.10.9 Sugar Versión 6.5.25 (Compilación 344) PHP version 7.0.32 MYSQLServer 5.7

Hosted in Azure over Windows 2008 R2 Datecenter

Need to do this.

Deleted these lines

// Fix for issue 1506 and issue 1304 : IE11 and Microsoft Edge cannot display generic 'application/octet-stream' (which is defined as "arbitrary binary data" in RFC 2046). $mime_type = mime_content_type($local_location); if ($mime_type == null || $mime_type == '') { $mime_type = 'application/octet-stream'; }

and let this one

$mime_type = 'application/octet-stream';

now it's working

pgorod commented 5 years ago

It seems you're reverting this commit: https://github.com/salesagility/SuiteCRM/commit/a693000827448b11dc1585142ac6416e603325fb

But that was supposedly fixing a bug, so I guess a better fix is needed...

Are you a developer? Can you please put back the original code and tell us what you have in $local_location, and what gets returned by mime_content_type($local_location)?

pgorod commented 5 years ago

A couple of technical updates on this issue, from this link.

This was never really deprecated, it was a bug in the documentation.
the link contains some code to replace mime_content_type with simple functions that don't require an external file, and should work across all versions and flavors of PHP.

lazka commented 5 years ago

An alternative might be warn at installation time that the extension is missing and the php installation is broken.

At least according to this the function should be available in all relevant PHP versions: https://3v4l.org/ZJ1lA

pgorod commented 5 years ago

This came up in the Forums again.

It occurred to me that now that we have Composer, there's probably a package somewhere to take care of this, fetching an up-to-date list of mime types.

Turns out there is: https://github.com/xobotyi/php-mime-type/

This way it would update normally when we update composer, it wouldn't be an extra thing for us to remember.

SuiteBot commented 2 years ago

This issue has been mentioned on SuiteCRM. There might be relevant details there:

https://community.suitecrm.com/t/php-fatal-error-after-file-upload-on-v7-and-v8/84177/1

salesagility / SuiteCRM