Skip to content

better error msg when commit url is not accessible #151

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

better error msg when commit url is not accessible #151

wants to merge 2 commits into from

Conversation

@davewood
Copy link

@davewood davewood commented Feb 10, 2016

see #150

@dregad
Copy link
Member

dregad commented Feb 10, 2016

Thanks for your contribution.

I had a look at URL API in MantisBT core, and as far as I can tell it never actually returns false... the "last resort" call to curl via shell_exec() will return null in case of failure; the exit code is not captured, and the actual error message is printed on STDERR which is not captured either.

To make a long story short, the false === $t_input condition at line 230 is never true... I believe we should simply check for null instead of false.

@davewood
Copy link
Author

davewood commented Feb 10, 2016

Can't we look for truth which would also take care of the case where the string is empty?

The second check should be if the document appears to be HTML. m/<html>.*<\/html>/xmsi or something similiar.

if ( !$t_input ) {
    echo "failed.\n";
    echo "$t_commit_url\n"; # DEBUG
    continue;
}

$t_pattern = /^<html>.*<\/html>$/i;
if( !preg_match( $t_pattern, $t_input, $t_matches ) ) {
    echo "data does not look like a HTML document.\n"; # DEBUG
}

@dregad
Copy link
Member

dregad commented Feb 11, 2016

Can't we look for truth which would also take care of the case where the string is empty?

Yes, it would make sense not to use a strict comparison here.

The second check should be if the document appears to be HTML. m/.*</html>/xmsi or something similiar.

I'm not 100% convinced this is really useful, but as I don't use Gitweb myself, I'm not the best judge.

Assuming it is, we need to make sure the regex can handle whatever html Gitweb can throw back at us. As it stands, I don't think this is the case with your proposed pattern[1]; IMO you should at the very least remove the ^, use <html.*> and specify the s pattern modifier.

[1] based on sample output from gitweb, which looks like this

<?xml version="1.0" encoding="utf-8"?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en-US" lang="en-US">
<!-- git web interface version 1.7.10.4, (C) 2005-2006, Kay Sievers <kay.sievers@vrfy.org>, Christian Gierke -->
<!-- git core binaries version 1.7.10.4 -->
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8"/>
<meta name="generator" content="gitweb/1.7.10.4 git/1.7."
...
</html>

@davewood
Copy link
Author

davewood commented Feb 11, 2016

you are right, lets go with the check for truth for a start.

dregad pushed a commit that referenced this pull request Feb 11, 2016
@davewood @dregad
Gitweb: error handling when commit URL returns no data
25d2959 
Prior to this, the code did a strict-type check of url_get()'s return
value === false, but the function actually returns NULL in case of
failure.

We now check for a non-empty string, which should properly catch any
error encountered by url_get().

In addition, the error message was improved to provide more useful
information to the user.

Fixes #150, #151

Signed-off-by: Damien Regad <dregad@mantisbt.org>

Change from original contribution:
- fixed whitespace
- reworded error message
- detailed commit message
@dregad dregad closed this Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@davewood @dregad