Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-2341

[GH-606] Don't add empty Expr to another one

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.3.3
    • Component/s: None
    • Security Level: All
    • Labels:
      None

      Description

      This issue is created automatically through a Github pull request on behalf of jean-gui:

      Url: https://github.com/doctrine/doctrine2/pull/606

      Message:

      Doctrine should not allow to add an empty Expr to another one. Current code allows that, which can lead to wrong DQL.
      Example 1:
      ```php
      $andExpr = $this->_expr->andx();
      $andExpr->add($this->_expr->andx());
      $andExpr->add($this->_expr->eq(1, 1));
      echo $andExpr;
      ```
      will output:
      ```sql
      AND 1 = 1
      ```
      instead of:
      ```sql
      1 = 1
      ```

      Example 2:
      ```php
      echo $andExpr;
      ```
      will output:
      ```sql
      AND AND
      ```
      instead of nothing.

      IRC log of the discusion on #doctrine:
      ```irc
      [jeudi 7 mars 2013] [14:36:18] <Jean-Gui> hi
      [jeudi 7 mars 2013] [14:36:35] <Jean-Gui> I have a question about Doctrine/ORM/Query/Expr/Base.php
      [jeudi 7 mars 2013] [14:37:17] <Jean-Gui> looking at the add function (https://github.com/doctrine/doctrine2/blob/master/lib/Doctrine/ORM/Query/Expr/Base.php#L89), the first if seems wrong
      [jeudi 7 mars 2013] [14:37:39] <Jean-Gui> [[ if ( $arg !== null || ($arg instanceof self && $arg->count() > 0) ) ]]
      [jeudi 7 mars 2013] [14:39:49] <Jean-Gui> if $arg is not null, then it will get added even if $arg->count === 0
      [jeudi 7 mars 2013] [14:41:03] <ocramius> yes
      [jeudi 7 mars 2013] [14:41:42] <Jean-Gui> that doesn't seem right, does it?
      [jeudi 7 mars 2013] [14:43:29] <ocramius> why not?
      [jeudi 7 mars 2013] [14:43:34] <ocramius> can you elaborate?
      [jeudi 7 mars 2013] [14:45:37] <Jean-Gui> that "if" seems to be meaning that the function shouldn't add $arg if $arg->count() equals 0
      [jeudi 7 mars 2013] [14:45:45] <Ninj> ocramius: i think Jean-Gui means that the right side of the OR will be called only if the left side is false, which means $arg is null. And if $arg is null it cannot be an object
      [jeudi 7 mars 2013] [14:46:07] <Jean-Gui> right
      [jeudi 7 mars 2013] [14:46:30] <Ninj> this condition is indeed bugged
      [jeudi 7 mars 2013] [14:47:12] <alcuadradoatwork> I see Jean-Gui's point too
      [jeudi 7 mars 2013] [14:47:18] <ocramius> write a test case then
      [jeudi 7 mars 2013] [14:47:24] <alcuadradoatwork> maybe it works right, but it's confusing
      [jeudi 7 mars 2013] [14:48:07] <Ninj> this condition will not throw any error because the "instanceof" will prevent the $arg::count function to be called on a null object, but it is still useless
      [jeudi 7 mars 2013] [14:48:20] <Jean-Gui> I think [[ if ( $arg Unable to render embedded object: File (== null && () not found.($arg instanceof self) || ($arg instanceof self && $arg->count() > 0)) ) ]] would work better
      [jeudi 7 mars 2013] [14:48:31] * Jean-Gui will look into how to submit bug reports
      [jeudi 7 mars 2013] [14:48:49] <alcuadradoatwork> isn't this enough? if ($arg instanceof self && $arg->count() > 0)
      [jeudi 7 mars 2013] [14:49:18] <Ninj> i think it is, alcuadradoatwork
      [jeudi 7 mars 2013] [14:49:28] <Jean-Gui> no, because if $arg is not an instance of self, I think we still want to add it
      [jeudi 7 mars 2013] [14:49:45] <ocramius> alcuadradoatwork: again... if it is a buggy condition, write a small test and open a PR
      [jeudi 7 mars 2013] [14:50:03] <alcuadradoatwork> ocramius, it depends on your definition of "buggy"
      [jeudi 7 mars 2013] [14:50:32] <alcuadradoatwork> it won't damage the behavior of the software, but it's quality its kind of pour
      [jeudi 7 mars 2013] [14:51:19] <ocramius> alcuadradoatwork: yes, still it needs coverage. I didn't say it needs a FAILING test
      [jeudi 7 mars 2013] [14:51:38] <alcuadradoatwork> I see your point
      [jeudi 7 mars 2013] [14:51:47] <Ninj> if the logic is : "add any non-null object, but if it's self so add it only if count>0" then it must be rewriten
      [jeudi 7 mars 2013] [14:52:46] <Jean-Gui> $args should be added if it's an instance of self or $_allowedClasses
      [jeudi 7 mars 2013] [14:53:16] <Ninj> hum
      [jeudi 7 mars 2013] [14:53:37] <Ninj> $args should be added if it's an instance of self with count > 0 or any other $_allowedClasse
      [jeudi 7 mars 2013] [14:53:55] <Jean-Gui> yes, Ninj
      [jeudi 7 mars 2013] [14:55:01] <Jean-Gui> thanks for your help guys, I'll submit a bug report with some test cases
      [jeudi 7 mars 2013] [14:55:43] <Ninj> your welcome Jean-Gui
      ```

        Activity

        Benjamin Eberlei created issue -
        Benjamin Eberlei made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.3.3 [ 10329 ]
        Resolution Fixed [ 1 ]
        Hide
        Benjamin Eberlei added a comment -

        A related Github Pull-Request [GH-606] was closed
        https://github.com/doctrine/doctrine2/pull/606

        Show
        Benjamin Eberlei added a comment - A related Github Pull-Request [GH-606] was closed https://github.com/doctrine/doctrine2/pull/606

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DDC-2341, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Benjamin Eberlei
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: