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

MSSQL: avoid warnings if result is invalid #102

Closed
wants to merge 2 commits into from
Closed

MSSQL: avoid warnings if result is invalid #102

wants to merge 2 commits into from

Conversation

Gargaj
Copy link
Contributor

@Gargaj Gargaj commented Sep 19, 2014

No description provided.

@vrana
Copy link
Owner

vrana commented Sep 19, 2014

What problem is this trying to solve? Ignoring errors usually lead to more problems. It should at least return false in case of an error.

@Gargaj
Copy link
Contributor Author

Gargaj commented Sep 22, 2014

@vrana It's not about ignoring errors; Adminer throws unnecessary PHP warnings when the SQL query fails because the query result is false: http://pbrd.co/1ud3Si2

@vrana
Copy link
Owner

vrana commented Sep 22, 2014

If there's a problem then it should return false.

@Gargaj
Copy link
Contributor Author

Gargaj commented Sep 22, 2014

@vrana It doesn't in the original either: https://github.com/vrana/adminer/blob/master/adminer/drivers/mssql.inc.php#L65 - is that a bug there too?

@vrana
Copy link
Owner

vrana commented Sep 23, 2014

Yes. The original code at least triggers a warning which reports the error in some way. This code mutes it completely.

@Gargaj
Copy link
Contributor Author

Gargaj commented Sep 26, 2014

@vrana It doesn't mute it at all, look at the screenshot above: http://pasteboard.co/ooveElb.png - the "Error in query" line still will be visible, just without the unnecessary PHP warnings around it.

@Gargaj
Copy link
Contributor Author

Gargaj commented Sep 26, 2014

Either way, I modified the pull to return false.

@vrana
Copy link
Owner

vrana commented Oct 6, 2014

Merged, thanks.

@vrana vrana closed this Oct 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants