Skip to content
This repository has been archived by the owner on Aug 22, 2023. It is now read-only.

Remove Query\Builder::__constructor overload #26

Closed
wants to merge 1 commit into from

Conversation

GromNaN
Copy link
Owner

@GromNaN GromNaN commented Aug 21, 2023

The constructor don't need to be overloaded to set the default grammar. It is read from the connection by default.

https://github.com/laravel/framework/blob/10.x/src/Illuminate/Database/Query/Builder.php#L246

@@ -758,16 +748,16 @@ public function lists($column, $key = null)
/**
* @inheritdoc
*/
public function raw($expression = null)
public function raw($value = null)
Copy link
Owner Author

Choose a reason for hiding this comment

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

Get the argument name from the parent method.

Copy link
Collaborator

@jmikola jmikola left a comment

Choose a reason for hiding this comment

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

LGTM assuming no behavioral changes (see question).

@@ -164,10 +164,10 @@ public function chunkById($count, callable $callback, $column = '_id', $alias =
/**
* @inheritdoc
*/
public function raw($expression = null)
public function raw($value = null)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -856,7 +846,7 @@ public function drop($columns)
*/
public function newQuery()
{
return new self($this->connection, $this->processor);
return new self($this->connection, null, $this->processor);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is a null second argument equivalent to constructing a new Grammar object (perhaps by way of calling Connection::getQueryGrammar()), as was done in the original overridden constructor (before the changes in this PR)?

Just want to confirm there's no behavioral change here.

@GromNaN
Copy link
Owner Author

GromNaN commented Aug 22, 2023

Moved to mongodb/laravel-mongodb#2570

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants