fix #17011 Graph « Cumulative by date » is not displayed in Summary > Advanced Summary #142

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Contributor

jeckyhl commented Feb 20, 2014

This fix an issue in Summary > Advanced Summary with SQL Server
SQL Server fails to execute the query for the first graph (cumulative by date)
This is because it's expecting a parameter of type VARCHAR and receive an integer

Contributor

grangeway commented Feb 20, 2014

No need to cast to string in php

SQL Server is known not to work in the current version. We are replacing the DB Layer soon to fix support for non-mysql DB's properly. Adding random casts is just going to make that harder and more confusing later to remove

grangeway closed this Feb 20, 2014

Contributor

jeckyhl commented Feb 20, 2014

I understand, I hope you'll succeed in providing a better non-mysql DB support
Thanks for feedback

Contributor

grangeway commented Feb 20, 2014

For reference, the 'new' version of the query / code looks something like:

$query = "SELECT {bug}.id, last_updated, date_modified, new_value, old_value
        FROM {bug} LEFT JOIN {history}
        ON {bug}.id = {history}.bug_id
        WHERE $specific_where
                    AND {bug}.status>=%d
                    AND ( ( {history}.new_value>=%d
                            AND {history}.field_name = 'status' )
                    OR {history}.id is NULL )
        ORDER BY {bug}.id, date_modified ASC";
$result = db_query( $query, array( $t_res_val, $t_res_val ) );

Note: we are using %d to indicate that it's a decimal.

The issue you raise is however somewhat interesting as new_value is a
string (as you say). However, from initially adding mssql support to
mantis years ago. And reason I really dont want to get into doing
'random patches now', PHP used to support:

php_mssql
php_odbc
and from memory I think there was some ado_mssql thing

Then PHP added pdo so you also have a choice of pdo_odbc.

On linux you have the option of using pdo-dblib.

And then more recently Microsoft came out a couple of years back with
sqlsrv/pdo_sqlsrv. The first version of which lacked some support for
cursors/counting - but I think that situation might have changed.

Historically, php_odbc used to be the most reliable way to use mantis
(and support large file uploads etc)

Our previous agreed goal was to try and use PDO as an interface, which
should give a minor performance boost to the PHP code by dropping the
adodb, and try and come up with some better DB support across the
board.

Would you be interested in helping test the new layer once it's been
ported against the master branch ?

On Thu, Feb 20, 2014 at 5:10 PM, jeckyhl notifications@github.com wrote:

I understand, I hope you'll succeed in providing a better non-mysql DB
support
Thanks for feedback

Reply to this email directly or view it on GitHubhttps://github.com/mantisbt/mantisbt/pull/142#issuecomment-35644546
.

Contributor

jeckyhl commented Feb 21, 2014

PHP Data Objects seems very interesting as it is shipped with standard distribution of PHP. The issues that comes in my mind are the lack of UTF-8 support in SQL Server (but that problem won't be worse with PDO than it is currently with AdoDB) and some specific SQL syntax (for example selecting the first result is archived with SELECT TOP(1) * FROM ... in MSSQL vs SELECT * FROM ... LIMIT 1 in Mysql)

So the task may not be easy, but if Mantis developpers think it's worth the try I'm sure they will succeed

I haven't very much time to give (neither you do I guess !) and my english is very bad (however it doesn't seem to be a so big problem in technical discussions...) but I can spend some time on tests. Feel free to email me if I can help (<my github username>@users.sourceforge.net)

dregad reopened this Feb 26, 2014

Owner

dregad commented Feb 26, 2014

@grangeway the type cast trick is a quick and easy fix for a current problem which I believe is already used elsewhere in Mantis, therefore I see no reason to summarily close this pull request. I don't think this fix would cause any issues with the future db api, therefore I would recommend to merge it.

Contributor

grangeway commented Feb 26, 2014

The field should be an int - I've spoken to Jeckyhl via email so he's going
to help test the new db layer in a few weeks.

Paul

On Wed, Feb 26, 2014 at 8:21 AM, Damien Regad notifications@github.comwrote:

@grangeway https://github.com/grangeway the type cast trick is a quick
and easy fix for a current problem which I believe is already used
elsewhere in Mantis, therefore I see no reason to summarily close this pull
request. I don't think this fix would cause any issues with the future db
api, therefore I would recommend to merge it.

Reply to this email directly or view it on GitHubhttps://github.com/mantisbt/mantisbt/pull/142#issuecomment-36101400
.

Member

rombert commented Feb 26, 2014

AFAIU the db layer will be rewritten, not patched, so I see no harm in merging this PR.

If we can make SQL servers a bit happier a bit earlier, I don't see why not.

Owner

dregad commented Feb 27, 2014

The field should be an int

Which field ? history.new_value ? That's not possible since it needs to store strings as well, e.g. if the history entry is an update of the bug summary. So we can only treat the parameter as an int for this specific query, but I am not sure {history}.new_value>=%d would actually solve the problem raised by @jeckyhl

Contributor

jeckyhl commented Feb 27, 2014

I am not sure {history}.new_value>=%d would actually solve the problem raised by @jeckyhl

No it won't

The two valid approaches with SQL Server are

SELECT ...
FROM mantis_bug_history_table
WHERE new_value = '90' AND ...

(was implemented until 7d76827) and something like

DECLARE @res_val VARCHAR(255); -- or VARCHAR(MAX)
SET @res_val = '90'
SELECT ...
FROM mantis_bug_history_table
WHERE new_value = @res_val AND ...

But this (with no quotes around « 90 ») will result in a conversion error :

SELECT ...
FROM mantis_bug_history_table
WHERE new_value = 90 AND ...
Owner

dregad commented Feb 27, 2014

@jeckyhl thanks for feedback. I guess this means that even with @grangeway's new API we would have to typecast this, and that's one more reason to merge this pull request now.

Contributor

grangeway commented May 26, 2014

So far, I've not spotted this as being an issue in the new api - I assume this happens anytime you go to view graphs?

Contributor

jeckyhl commented Jun 6, 2014

@grangeway yes, I can reproduce the problem by just clicking on menu "Summary", then "Advanced Summary" sub-menu. I see a blank space in place of the graph "Cumulative by date" (tested on Mantis 1.2.16)

@dregad dregad added a commit that referenced this pull request Jun 23, 2014

@jeckyhl @dregad jeckyhl + dregad Fix graph display in advanced summary with MSSQL
Graph « Cumulative by date » is not displayed in Summary > Advanced
Summary, SQL Server complains that it cannot convert varchar to int.

Fixes #17011, #142

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

@dregad dregad added a commit that referenced this pull request Jun 23, 2014

@jeckyhl @dregad jeckyhl + dregad Fix graph display in advanced summary with MSSQL
Graph « Cumulative by date » is not displayed in Summary > Advanced
Summary, SQL Server complains that it cannot convert varchar to int.

Fixes #17011, #142

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

dregad closed this Jun 23, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment