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

Implementing subqueries in FROM section #4476

Closed

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented Mar 23, 2021

Description
I am waiting feedback of the community, so I see no reason to add tests and descriptions to UG right now.

Added BaseConnection::raw() method.
It expects a raw SQL subquery as an argument and returns an instance of the SqlExpression class.
This is to prevent processing subqueries as a simple string.

BaseConnection::table() and BaseBuilder::from() methods.
Now takes as an argument Closure, BaseBuilder, SqlExpression, array, string, null.

//Closure 
$this->db->table(function(BaseBuilder $builder) { /* subquery */  });

// BaseBuilder 
$subquery = $this->db->table('user');
$this->db->table($subquery);

// SqlExpression
$this->db->table($this->db->raw('SELECT * FROM user'));

// new instance of BaseBuilder without dafeult table 
// Thanks to the designers. Because it is impossible to correctly create a clean instance of the class without BCs. 
$this->db->table(null);

This solution makes it possible to use a wrapper for the main query.

SELECT * FROM (main query)

and will allow better implementation of support for UNION queries.

Query builder methods that previously took Closure as an argument now take BaseBuilder and SqlExpression.

$builder->where('id', $this->db->raw('SELECT id FROM user'));

Help wanted
I also want to ask about this "problem".
The problem starts with tests (SQLSRV). We get an instance of the query builder.

$this->db = new MockConnection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);
$builder = new SQLSRVBuilder('user', $this->db);

If $this->db->table(null) is called inside the query builder, then the MockBuilder instance will be retrieved, not the SQLSRVBuilder instance.
And the subquery will not be generated correctly.

There are several solutions.
Use new static(null, $this->db) instead of $this->db->table(null) , but phpstan says: Unsafe usage of new static().
Or use the SQLSRV driver's Coonnection class instead of the MockConnection class for testing.

$this->db = new Connection(['DBDriver' => 'SQLSRV', 'database' => 'test', 'schema' => 'dbo']);

But I'm not sure if replacing the connection class is correct.

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@iRedds iRedds marked this pull request as ready for review March 23, 2021 17:08
@kenjis
Copy link
Member

kenjis commented Mar 24, 2021

If $this->db->table(null) is called inside the query builder, then the MockBuilder instance will be retrieved, not the SQLSRVBuilder instance.

It seems the $this->db should be an object to return SQLSRVBuilder.
And MockConnection does not behave like that. It is not a correct mock.

If you can write tests with the real SQLSRV Connection without a real SQL Server, it is no problem.
If not, it is better to create a new MockConnection class like SQLSRV\MockConnection.
Or you could use PHPUnit mock functionality to create a mock:
https://phpunit.readthedocs.io/en/9.5/test-doubles.html

$store = $this->getMockBuilder(CookieStore::class)
->setConstructorArgs([$cookies])
->onlyMethods(['setRawCookie', 'setCookie'])
->getMock();

@kenjis kenjis added the stale Pull requests with conflicts label Nov 2, 2021
@lonnieezell
Copy link
Member

Seems to be overridden by #5510

Closing.

@lonnieezell lonnieezell closed this Jan 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants