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

[FEATURE] QueryBuilder::whereIn #9

Closed
MaartenStaa opened this issue Jun 22, 2016 · 15 comments
Closed

[FEATURE] QueryBuilder::whereIn #9

MaartenStaa opened this issue Jun 22, 2016 · 15 comments

Comments

@MaartenStaa
Copy link

It would be nice if the query builder supported a whereIn function, along with orWhereIn. This is something I miss quite often also in other frameworks.

Example usage:

use Opulence\QueryBuilders\PostgreSql\QueryBuilder;

$query = (new QueryBuilder)->select("id", "name", "email")
    ->from("users")
    ->whereIn("id", [1, 2, 3])
    ->orWhereIn("email", ["foo@example.com", "bar@example.com"]);
echo $query->getSql();

This would output:

SELECT id, name, email FROM users WHERE id IN (?, ?, ?) OR email IN (?, ?)

I wouldn't mind giving implementing this a shot, the only question is whether this should default to using named or unnamed parameters. Perhaps it could default to unnamed, with an optional parameter to use named parameters? I'd like to hear what you think.

@davidbyoung
Copy link
Member

Interesting idea. I am trying to decide if it makes sense to add new methods for each condition, eg whereIn() or if it should be something like where(Condition::in("id", [1, 2, 3])). If we were to support more and more conditions, going the where{conditionName}() route might get bloated. If we go the where(Condition::{conditionName}()) route, the where() method remains cleaner.

@MaartenStaa
Copy link
Author

What other where methods do you envision being added in the future? Right now already all basic conditions can be expressed easily, only whereIn requires trickiness (formatting the placeholders etcetera).

I did not yet dive into the code so I don't know exactly how the queries are represented internally, but I think there are two main cases.

  1. where(condition), where condition is something like x operator ?
  2. where(multi-condition), e.g. a callback that is able to define something like this:
...->where(function (SubWhere $sub) {
    $sub->where("x = ?")
        ->orWhere(function (SubWhere $sub) {
            $sub->where("y = ?")
                ->where("z = ?");
        });
    });

This would produce something like this:

... WHERE (x = ? OR (y = ? AND z = ?))

Number one is now well covered except for whereIn, I think, number two is I think not yet supported but could be something like whereSub. Combined with their "or" variants, this makes for a total of six where functions. That would be pretty okay, I think.

@naroga
Copy link

naroga commented Jun 22, 2016

I'd rather have ->where(Condition::in('id', [...])), or something to that effect.

@davidbyoung
Copy link
Member

@MaartenStaa I am not sure what other conditions we'd support in the future, but I cannot rule adding more out in the future. If PHP had short closures, something like number two might be ok. For the time being, though I feel like it's awfully verbose.

@naroga The issue with my solution is that I hate static methods. Almost every time I have used them, it's because I didn't spend enough time contemplating how I could've made them instance methods. Static methods are harder to mock and unit test, so I try to steer clear of them whenever I can. I'll have to think about this for a little bit.

@MaartenStaa
Copy link
Author

Agreed about static methods, as well as about the function syntax. For what it's worth though, this is the same format Laravel uses, so it is something people are used to.

@Garethp
Copy link

Garethp commented Jun 23, 2016

I personally think ->where(Condition::in()) would be a better choice. It'd be more extensible, and allow for custom conditions if you wanted to create them yourself (Such as having a ->where(MyDomainCondition::CustomerHasThing($customer, $thing)). It's also rather expressive as I read it.

@MaartenStaa
Copy link
Author

@Garethp the same could be achieved by sub-classing the query builder though, and then you could do ->whereCustomerHasThing($customer, $thing).

@Garethp
Copy link

Garethp commented Jun 23, 2016

True, but with Condition:in() you could code against an interface, so that rather than people extending the query builder, they can just have the results of MyDomainCondition::CustomerHasThing implement an interface, which is a cleaner solution imo

@davidbyoung
Copy link
Member

davidbyoung commented Jun 23, 2016

@MaartenStaa @Garethp I'm leaning more towards leaving where() alone and introducing a new helper to create IN, NOT IN, BETWEEN, etc condition builders. Otherwise, there will have to be permutations for where(), orWhere(), and andWhere() for each condition and its opposite. I'd prefer to steer clear of static methods, too.

How about something like the following?

(new QueryBuilder)->select("name")
    ->from("users")
    ->where((new ConditionFactory)->in("id", [1, 2, 3]));

If the user plans on generating multiple conditions, they could create the factory beforehand:

$conditions = new ConditionFactory();
(new QueryBuilder)->select("name")
    ->from("users")
    ->where($conditions->in("id", [1, 2, 3]))
    ->orWhere($conditions->between("age", 18, 21));

Thoughts?

@Thijs-Riezebeek
Copy link

@opulencephp I just had the exact same Idea of a ConditionFactory. Seems like a good way to isolate the creation of all the different conditions into a non-static class.

@Garethp
Copy link

Garethp commented Jun 24, 2016

What would ConditionFactory::in return? If where accepts interfaces, then while you can use ConditionFactory, you can literally just pass in anything that implements a ConditionInterface for example

@davidbyoung
Copy link
Member

I'm thinking it'd return an interface with methods to get the generated SQL as well as the parameters bound by the condition. So, where(), orWhere(), and andWhere() would be changed to accept strings and these interfaces.

@Garethp
Copy link

Garethp commented Jun 24, 2016

My knowledge on the inner workings of ORM's isn't up to scratch, but from an outside perspective I would assume that actually generating the SQL's would be the responsibilities of the adapters, no? Or am I off base there?

@davidbyoung
Copy link
Member

To permit the most flexibility, Opulence's SqlDataMapper requires you to write the SQL to add/delete/getBy*/update entities from your database. That being said, you can use Opulence's QueryBuilder library to help you write these queries.

@davidbyoung
Copy link
Member

I've implemented this, and documented it.

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

No branches or pull requests

5 participants