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

Adding BaseQuery that provides concrete implementation for Query #19761

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

johnkarpn
Copy link

Q A
Is bugfix?
New feature? ✔️
Breaks BC?
Fixed issues

There is no way to extend the yii/db/Query class.
There is a Yii::$classMap, but then you will have to copy all the code to a new class and maintain it manually or use patches.
This solution solves the problem.

@bizley bizley added the pr:missing usecase It is not clear what is the use case for the pull request. label Feb 9, 2023
@yii-bot
Copy link

yii-bot commented Feb 9, 2023

Thank you for putting effort in the improvement of the Yii framework.
We have reviewed your pull request.

Unfortunately a use case is missing. It is required to get a better understanding of the pull request and helps us to determine the necessity and applicability of the suggested change to the framework.

Could you supply us with a use case please? Please be as detailed as possible and show some code!

Thanks!

This is an automated comment, triggered by adding the label pr:missing usecase.

@johnkarpn
Copy link
Author

Our company uses mysql as a database. We need to use indexes to optimize database queries.

We want to add globally additional methods to set these indexes in order to use them in all of our Query classes (ActiveQuery and not only).

We were surprised (and still are) that there is no way to extend the Query class without crutches.

Indexes are one example. We have quite a few improvements that require extending the functionality of the Query class

@DeryabinSergey
Copy link
Contributor

We want to add globally additional methods to set these indexes in order to use them in all of our Query classes (ActiveQuery and not only).

What about DI?

For example you can define your query class common\db\Query.php

namespace common\db;

class Query extends \yii\db\Query
{
}

and define in config common/config/main.php

	'container' => [
		'definitions' => [
			\yii\db\Query::class => \common\db\Query::class,
		]
	]

@johnkarpn
Copy link
Author

Простите, я так понимаю, что тут на русском могут. Проще объяснить...

Данное решение - это первое что мы попробовали, вроде очевидное действие, но нам оно не помогло.
Дело в том, что мало просто добавить какой-то функционал в yii\db\Query, нужно еще доработать QueryBuilder, а именно метод build($query, $params = [])

С доработкой QueryBuilder нет проблем. Добавил расширенный yii\db\Connection и указал свой класс CustomQueryBuilder, например:

class CustomDbConnection extends Connection {
	/**
	 * @return CustomQueryBuilder
	 */
	public function getQueryBuilder() {
		return new CustomQueryBuilder($this->getSchema()->db);
	}
}

Указал в definitions \yii\db\Query::class => \common\db\Query::class и по логике должно всё прекрасно работать. Но нет...
в build() нашего класса передается в $query базовый объект yii\db\Query, а не мой \common\db\Query::class

Изучив код yii2 досконально, приходишь к выводу, что yii\db\Query просто зашит там.
И самое простое решение - это переопределить его в $classMap.
Но в текущей реализации тебе необходимо полностью скопировать код yii\db\Query в свой класс и поддерживать его вручную.
Мы пока нашли решение, запускать патчи в composer, причем мы делаем тоже самое, что и в этом pull-request.
Но, на мой взгляд, это не очень правильно...

@samdark samdark added status:to be verified Needs to be reproduced and validated. type:enhancement and removed pr:missing usecase It is not clear what is the use case for the pull request. labels Feb 9, 2023
@rob006
Copy link
Contributor

rob006 commented Feb 23, 2023

Why not extend Query and ActiveQuery and use these new classes in your project instead of basic classes?

@johnkarpn
Copy link
Author

Because we use these methods not only in the context of ActiveQuery. I don't understand how it happened that the Query class can't be overridden in a simple way

@rob006
Copy link
Contributor

rob006 commented Feb 27, 2023

Because we use these methods not only in the context of ActiveQuery.

That does not explain what is the problem with solution I proposed. If you want to use your custom methods, then use new YourQuery() instead of new Query().

I don't understand how it happened that the Query class can't be overridden in a simple way

This BaseSomething convention used to overwrite some Yii classes is more like a hack and it is limited to helpers with static methods. For things that are actually instantiated you should use DI-like solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:to be verified Needs to be reproduced and validated. type:enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants