Unresolvable query not reported in debug mode

[hemberger]

I was hoping to use debug mode to find all queries that phpstan-dba fails to resolve, since these will not report any errors in non-debug mode. However, there appears to be a class of queries that silently fail to resolve, which makes it challenging to find and fix the queries.

Here are some examples that I used to explore this behavior:

• If the query is joined with a mixed type, we are alerted to the failure to resolve the query:

  $query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_mixed();

  Unresolvable Query: Cannot simulate parameter value for type: mixed.

• If the query is joined with an int type, the query is properly analyzed:

  $query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_int();

  Query error: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'dbname.table_does_not_exist' doesn't exist (42S02).

• If the query is joined with a string type, the error is silently ignored:

  $query = 'SELECT * FROM table_does_not_exist WHERE col1 = ' . return_string();

  No errors

Obviously, in this particular example, a prepared query would solve the problem, but there are legitimate use cases where we might join a non-literal string to a query string, and in such a case we'd want to get an error when the query cannot be resolved (in debug mode, at least). Is this possible?

Please let me know if further information is needed. I'm also happy to look into this myself if you can point me in the right direction.

Thanks again for this awesome library! I'm really looking forward to using it to help refactor an old codebase that used manual escaping instead of prepared queries.

[hemberger]

Would #243 solve this issue?

[staabm]

Obviously, in this particular example, a prepared query would solve the problem, but there are legitimate use cases where we might join a non-literal string to a query string, and in such a case we'd want to get an error when the query cannot be resolved (in debug mode, at least). Is this possible?

I was hesitant adding error for all unresolvable cases when I have implemented debug mode, because I thought it might be a problem for new-comers to see value in the tool, when e.g. in legacy code-bases most queries would error.

since the tool evolved and we are understanding queries better (and I also think not a lot of people are using debug mode), I think we could change debug mode to also report errors for the cases a string is contained.

if I understand you correctly, you would prefer having this error in the phpstan baseline over not having them reported, right?

[staabm]

I guess you would need to report the error here in debug mode:

phpstan-dba/src/QueryReflection/QuerySimulation.php

Lines 96 to 97 in e0f23fd

	// plain string types can contain anything.. we cannot reason about it
	return null;

similar to how we do it here

phpstan-dba/src/QueryReflection/QuerySimulation.php

Lines 115 to 117 in e0f23fd

	if (QueryReflection::getRuntimeConfiguration()->isDebugEnabled()) {
	throw new UnresolvableQueryException('Cannot simulate parameter value for type: '.$paramType->describe(VerbosityLevel::precise()));
	}

[hemberger]

if I understand you correctly, you would prefer having this error in the phpstan baseline over not having them reported, right?

Yes, that is definitely my preference. I tried throwing an UnresolvableQueryException as you described above, and it was very helpful for identifying queries that were not being checked.

I was hesitant adding error for all unresolvable cases when I have implemented debug mode, because I thought it might be a problem for new-comers to see value in the tool, when e.g. in legacy code-bases most queries would error.

That's a reasonable concern, but I think that most (if not all) people would try out the tool with debug mode turned off to start - I know I did. In fact, I thought debug mode was just going to be more verbose, but now that I know what it does I can't imagine using phpstan-dba without it!

If you'd like me to submit a PR, I have a couple questions about your preferences for implementation:

• Would you want this to be part of debug mode, or a separate configuration option? (e.g. $config->reportUnresolvableQueries(true), perhaps along with the mixed-type error)
• Would you want the UnresolvableQueryException::RULE_TIP to be expanded to include info about non-literal strings, or would you rather have separate exceptions for each type of failure (deriving from UnresolvableQueryException) with their own customized tips?

Otherwise, I can leave it to you. Thanks again!

[staabm]

In fact, I thought debug mode was just going to be more verbose, but now that I know what it does I can't imagine using phpstan-dba without it!

thanks for the feedback. looking forward to more feedback as you use the tool even more.

• Would you want this to be part of debug mode, or a separate configuration option? (e.g. $config->reportUnresolvableQueries(true), perhaps along with the mixed-type error)

atm the debug mode is meant todo that for now. it might make sense to move this to a separate flag as suggested in the future though.

• Would you want the UnresolvableQueryException::RULE_TIP to be expanded to include info about non-literal strings, or would you rather have separate exceptions for each type of failure (deriving from UnresolvableQueryException) with their own customized tips?

I think we should make UnresolvableQueryException abstract and create one subclass per case.

thank you