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

[ResourBundle] The criteria don't care about empties values #1487

Merged
merged 1 commit into from
May 14, 2014

Conversation

arnolanglade
Copy link
Contributor

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

@stloyd
Copy link
Contributor

stloyd commented May 10, 2014

This is not proper fix IMO. There are for sure cases when you want to look for false, '' (empty string, rare but still), 0 etc.

@arnolanglade
Copy link
Contributor Author

Agree for boolean or 0 but not for empty string. For a filter form you can not leave fields empties and it is annoying! I will update it soon. Thanks for feedback.

@arnolanglade
Copy link
Contributor Author

Perhaps, I should only use $value != ""

@stloyd
Copy link
Contributor

stloyd commented May 11, 2014

@Arn0d IMO this should be same as i.e. in Symfony\BlankValidator:

'' !== $value && null !== $value

@arnolanglade
Copy link
Contributor Author

@stloyd yes I will only use '' !== $value because we already check if the value is null above.

@arnolanglade
Copy link
Contributor Author

@Sylius/core-team Are you agree ?

@@ -161,12 +161,12 @@ protected function applyCriteria(QueryBuilder $queryBuilder, array $criteria = n
if (null === $value) {
$queryBuilder
->andWhere($queryBuilder->expr()->isNull($this->getPropertyName($property)));
} elseif (!is_array($value)) {
} elseif(is_array($value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

} elseif (is_array($value)) {

@arnolanglade
Copy link
Contributor Author

@umpirsky
It was :

elseif (!is_array($value)) {
      $queryBuilder
            ->andWhere($queryBuilder->expr()->eq($this->getPropertyName($property), ':' . $property))
            ->setParameter($property, $value);
 } else {
      $queryBuilder->andWhere($queryBuilder->expr()->in($this->getPropertyName($property), $value));
}

And now, it is :

} elseif(is_array($value)) {
      $queryBuilder->andWhere($queryBuilder->expr()->in($this->getPropertyName($property), $value));
} elseif ('' !== $value) {
      $queryBuilder
            ->andWhere($queryBuilder->expr()->eq($this->getPropertyName($property), ':' . $property))
            ->setParameter($property, $value);
 }

@stloyd
Copy link
Contributor

stloyd commented May 12, 2014

@Arn0d Saša is not talking about your change was wrong or not, he just points you to fix CS (note missing space between elseif & ().

@arnolanglade
Copy link
Contributor Author

OK! Done!

@umpirsky
Copy link
Contributor

:)

@arnolanglade
Copy link
Contributor Author

ping @pjedrzejewski

@arnolanglade
Copy link
Contributor Author

@pjedrzejewski Are you agree with this? I need it for a project ! Can we merge it ?

pjedrzejewski pushed a commit that referenced this pull request May 14, 2014
[ResourBundle] The criteria don't care about empties values
@pjedrzejewski pjedrzejewski merged commit 46ffc16 into Sylius:master May 14, 2014
@pjedrzejewski
Copy link
Member

Thank you Arnaud!

@arnolanglade
Copy link
Contributor Author

You're welcome!

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
[ResourBundle] The criteria don't care about empties values
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants