Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add finfo_close to File::mime #6779

Closed
wants to merge 1 commit into from
Closed

add finfo_close to File::mime #6779

wants to merge 1 commit into from

Conversation

Graziel
Copy link
Contributor

@Graziel Graziel commented Jun 10, 2015

im not sure if its even needed as ive seen examples without it but still better safe then sorry, also fixed a bit of logic

im not sure if its even needed as ive seen examples without it but still better safe then sorry, also fixed a bit of logic
@ADmad
Copy link
Member

ADmad commented Jun 10, 2015

What is this supposed to fix?

@ADmad ADmad added this to the 3.0.7 milestone Jun 10, 2015
if (!$finfo) {
return false;
}
list($type) = explode(';', $finfo);
$mime = finfo_file($finfo, $this->pwd());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading mimetype could fail for some reason, so there should be a check for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah sorry i got mislead by old c habbits of docs reading where finfo_file says it returns string

so full should be like

$finfo = finfo_open(FILEINFO_MIME);
if (!$finfo) {
     return false;
}
$mime = finfo_file($finfo, $this->pwd());
if (!$mime) {
    finfo_close($finfo);
    return false;
}
finfo_close($finfo);
list($type) = explode(';', $mime);
return $type;

or anything else to change you see with your eyes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OOP style usage looks much cleaner:

try {
    $finfo = new finfo(FILEINFO_MIME);
    $type = $finfo->file($this->pwd());
    if (!$type) {
        return false;
    }
    list($type) = explode(';', $type);
    return $type;
} catch (\Exception $e) {
    return false;
}

Here I am assuming it throws an exception in case of an error, can't find any docs relating to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see any throws in:
https://github.com/php/php-src/blob/PHP-5.4.1/ext/fileinfo/fileinfo.c

so looks like all that is needed is

$finfo = new finfo(FILEINFO_MIME);
if (!$finfo ) {
    return false;
}
$type = $finfo->file($this->pwd());
if (!$type) {
    return false;
}
list($type) = explode(';', $type);
return $type;

as i understand it will autoclose on destroy

@markstory
Copy link
Member

Closing the finfo resource makes sense, but the point @ADmad made around error checking should be addressed.

@markstory markstory self-assigned this Jun 13, 2015
@markstory markstory modified the milestones: 3.0.7, 3.0.8 Jun 15, 2015
markstory added a commit that referenced this pull request Jun 15, 2015
Improves upon the code suggested in #6779

Refs #6779
@markstory markstory mentioned this pull request Jun 15, 2015
@markstory
Copy link
Member

Superseded by #6810

@markstory markstory closed this Jun 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants