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

Predictable QueryBuilder::executeQuery() and QueryBuilder::executeStatement() #4578

Merged

Conversation

PowerKiKi
Copy link
Contributor

@PowerKiKi PowerKiKi commented Apr 2, 2021

Q A
Type improvement
BC Break no
Fixed issues #4461 (comment)

Summary

This deprecates QueryBuilder::execute(), because its return type is
unpredictable and raises issues with static analysis tools such as PHPStan.

Instead you should use either QueryBuilder::executeQuery() or
QueryBuilder::executeStatement(), depending on whether the queryBuilder is a query (SELECT)
or a statement (INSERT, UPDATE, DELETE).

You might also consider the use of the new shortcut methods, such as:

  • fetchAllAssociative()
  • fetchAllAssociativeIndexed()
  • fetchAllKeyValue()
  • fetchAllNumeric()
  • fetchAssociative()
  • fetchFirstColumn()
  • fetchNumeric()
  • fetchOne()

This commit is a direct follow-up to #4461
where those shortcut methods where introduced.

@beberlei
Copy link
Member

beberlei commented Apr 2, 2021

If we do this change, then we could look to add this Statement in general maybe even in 2.x, because it would help us solve the issue with Statement::execute() returning a boolean at the moment. The API could be in general Statement::executeStatement (yeah naming) and Statement::executeQuery.

@morozov
Copy link
Member

morozov commented Apr 2, 2021

yeah naming

The naming problem is not on our end. The "statement" on the left-hand side is a prepared statement. The "statement" on the right-hand side is a SQL statement (DDL/DML/DCL) as opposed to an SQL query. Having it named as Statement::executeUpdate() would be worse IMO.

@morozov
Copy link
Member

morozov commented Apr 3, 2021

Looks good to me. @beberlei do you want to backport it to 2.x or it's not necessary for the upgrade path improvement?

@beberlei
Copy link
Member

beberlei commented Apr 4, 2021

@morozov the query builder change is ok for 3.1.x I believe, since the API for QueryBuilder::execute() does not return a boolean, it is already ok to use, would be a new migration step in the future for users. I don't believe there is pressure moving this to 2.13

@beberlei
Copy link
Member

beberlei commented Apr 4, 2021

#4580 would be the one that is more interesting for the 2.x branches

Copy link
Member

@morozov morozov left a comment

@PowerKiKi please add a runtime deprecation using Deprecation::trigger().

@PowerKiKi
Copy link
Contributor Author

PowerKiKi commented Apr 5, 2021

I added the Deprecation::trigger() call as required.

src/Query/QueryBuilder.php Outdated Show resolved Hide resolved
@PowerKiKi PowerKiKi force-pushed the semantical-execute-methods-query-builder branch from 8975f66 to c8f1a38 Compare Apr 5, 2021
Copy link
Member

@morozov morozov left a comment

Looks good overall. Please squash at the end.

src/Query/QueryBuilder.php Outdated Show resolved Hide resolved
src/Query/QueryBuilder.php Outdated Show resolved Hide resolved
@@ -300,19 +300,57 @@ public function fetchFirstColumn(): array
return $this->connection->fetchFirstColumn($this->getSQL(), $this->params, $this->paramTypes);
}

/**
* Executes an SQL query (SELECT) and returns a Result.
Copy link
Member

@morozov morozov Apr 6, 2021

Choose a reason for hiding this comment

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

This may be misleading since it works also for DESCRIBE, SHOW and so on. Let's rephrase to:

Executes an SQL query that returns a Result.

Copy link
Contributor Author

@PowerKiKi PowerKiKi Apr 6, 2021

Choose a reason for hiding this comment

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

I'd agree with that on the Connection class, but AFAIK, there is no way to build anything else than a SELECT query with a QueryBuilder. So I don't think we should document an impossible use-case.

WDYT ?

see:

/*
* The query types.
*/
public const SELECT = 0;
public const DELETE = 1;
public const UPDATE = 2;
public const INSERT = 3;

Copy link
Member

@morozov morozov Apr 6, 2021

Choose a reason for hiding this comment

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

You're right, I didn't take the scope into account. Sorry for the confusion.

/**
* Executes an SQL statement and returns the number of affected rows.
*
* Should be used for INSERT, UPDATE and DELETE
Copy link
Member

@morozov morozov Apr 6, 2021

Choose a reason for hiding this comment

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

It's not limited to DML (INSERT, etc.), it could be also DDL and other non-DQL statements. See JDBC for reference.

Copy link
Contributor Author

@PowerKiKi PowerKiKi Apr 6, 2021

Choose a reason for hiding this comment

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

Idem, the QueryBuilder cannot build anything else than DML. Should we really mention that ?

Copy link
Member

@morozov morozov Apr 6, 2021

Choose a reason for hiding this comment

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

No, you're right.

…Statement()`

This deprecates `QueryBuilder::execute()`, because its return type is
unpredictable and raises issues with static analysis tools such as PHPStan.

Instead you should use either `QueryBuilder::executeQuery()` or
`QueryBuilder::executeStatement()`, depending on whether the queryBuilder is a query (SELECT)
or a statement (INSERT, UPDATE, DELETE).

You might also consider the use of the new shortcut methods, such as:

- `fetchAllAssociative()`
- `fetchAllAssociativeIndexed()`
- `fetchAllKeyValue()`
- `fetchAllNumeric()`
- `fetchAssociative()`
- `fetchFirstColumn()`
- `fetchNumeric()`
- `fetchOne()`

This commit is a direct follow-up to doctrine#4461
where those shortcut methods where introduced.
@PowerKiKi PowerKiKi force-pushed the semantical-execute-methods-query-builder branch from 100cde2 to 7b5dd78 Compare Apr 6, 2021
morozov
morozov approved these changes Apr 6, 2021
@morozov morozov merged commit 3ea7e73 into doctrine:3.1.x Apr 6, 2021
48 checks passed
@morozov
Copy link
Member

morozov commented Apr 6, 2021

Thanks, @PowerKiKi.

@PowerKiKi PowerKiKi deleted the semantical-execute-methods-query-builder branch Apr 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants