Skip to content

Simplify boolean filter #1905

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

Merged
merged 1 commit into from
Apr 30, 2018
Merged

Simplify boolean filter #1905

merged 1 commit into from
Apr 30, 2018

Conversation

bendavies
Copy link
Contributor

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

no need for extra query builder named parameters here. just use $expr->literal(...)

@bendavies bendavies changed the base branch from master to 2.2 April 30, 2018 09:37
@dunglas dunglas merged commit 7fb6254 into api-platform:2.2 Apr 30, 2018
@teohhanhui
Copy link
Contributor

This has implications for the DBMS prepared statement, no? I don't see why we'd want to do this, to be honest. Does it give a sizeable performance gain?

@bendavies
Copy link
Contributor Author

The reason i did this because

  1. there is no need for the parameter.
  2. the sql produced by symfonys profiler is invalid before this change, (at least for postgres - it produces where field = 1/0 which is wrong)

@teohhanhui
Copy link
Contributor

That's a Symfony profiler bug.

@soyuka
Copy link
Member

soyuka commented Apr 30, 2018

Isn't doctrine storing booleans as integers anyway?

@teohhanhui
Copy link
Contributor

teohhanhui commented Apr 30, 2018

Isn't doctrine storing booleans as integers anyway?

No. Not on PostgreSQL at least. The integers 0 and 1 are not valid boolean values: https://www.postgresql.org/docs/9.1/static/datatype-boolean.html

https://github.com/doctrine/dbal/blob/v2.7.1/lib/Doctrine/DBAL/Platforms/PostgreSqlPlatform.php#L66-L86

@bendavies
Copy link
Contributor Author

@teohhanhui

That's a Symfony profiler bug.

Well yes that's stating the obvious. I still don't understand the objection.

Cheers

@teohhanhui
Copy link
Contributor

I'm against it because a value is a value, not a literal. If there's a sizeable performance gain, I'm okay with making exceptions for the rule. But otherwise, I don't see why we'd do that.

teohhanhui pushed a commit to teohhanhui/api-platform-core that referenced this pull request May 2, 2018
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

Successfully merging this pull request may close these issues.

4 participants