Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-1071

CASE expressions do not work as documented in EBNF

    Details

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

      Description

      The EBNF definition of DQL on the website (http://www.doctrine-project.org/docs/orm/2.0/en/reference/dql-doctrine-query-language.html) defines the SELECT expression as follows:

      SelectExpression ::= IdentificationVariable | PartialObjectExpression | (AggregateExpression | "(" Subselect ")" | FunctionDeclaration | ScalarExpression) [["AS"] AliasResultVariable]

      A ScalarExpression, in turn, is defined as follows:

      ScalarExpression ::= SimpleArithmeticExpression | StringPrimary | DateTimePrimary | StateFieldPathExpression BooleanPrimary | CaseExpression | EntityTypeExpression

      There is already a missing ''|" between "StateFieldPathExpression" and "BooleanPrimary" here.

      But ignoring that detail, this definition states that case expressions can show up anywhere in the list of values to be selected in a query, which, of course, would come in handy.

      Contrary to this definition, queries like this one will not work:

      SELECT someEntity.a, CASE someEntity.b WHEN 1 THEN 'A' ELSE 'B' END FROM \someEntity;

      Whereas queries like this one will:

      SELECT CASE someEntity.b WHEN 1 THEN 'A' ELSE 'B' END FROM \someEntity;

      Both queries should be legitimate, according to the EBNF definition.

      In addition, CASE expressions are not allowed in subselects either, although the EBNF definition of the SimpleSelectExpression states that they should be.

        Issue Links

          Activity

          Show
          Guilherme Blanco added a comment - In this commit: https://github.com/doctrine/doctrine2/commit/816ce41f638d28934c79a12ef27f954124b2639e And documented in this commit: https://github.com/doctrine/orm-documentation/commit/189c729f15d2fafecf92662cad9553c2ec3dccd7 This support was FINALLY included. =)
          Hide
          Daniel Alvarez Arribas added a comment -

          Fixed typo ("EBDF" instead of "EBNF")

          Show
          Daniel Alvarez Arribas added a comment - Fixed typo ("EBDF" instead of "EBNF")
          Hide
          Benjamin Eberlei added a comment -

          There is no parser generator for PHP that generates a top-down recursive descent walker as the one we are using. There is a parser generator for bottom-up, but that is targeting php4 and produces ugly code.

          We have a convention to always update EBNF and code in conjunction, but given the large EBNF this can just be forgotten sometimes.

          Show
          Benjamin Eberlei added a comment - There is no parser generator for PHP that generates a top-down recursive descent walker as the one we are using. There is a parser generator for bottom-up, but that is targeting php4 and produces ugly code. We have a convention to always update EBNF and code in conjunction, but given the large EBNF this can just be forgotten sometimes.
          Hide
          Daniel Alvarez Arribas added a comment - - edited

          Thanks for taking care.

          An option would be to use a parser generator and generate the parser from an EBNF syntax spec, which saves you the process of deriving the EBNF manually from implementation code. As you already opted for maintaining a formal syntax definition, you could as well reap some consequential benefits. Then again, it would probably be a completely different approach. I also do not know a parser gerator capable of generating PHP 5 code.

          Another viable option would be making it a habit to update the EBNF definition first, then using it as a starting point for the actual implementation. Then, with each release, simply publish both code and documentation as you normally would. This way the EBNF definition does not run any risk of ever becoming outdated. Unless you find other ways of screwing it, of course

          Show
          Daniel Alvarez Arribas added a comment - - edited Thanks for taking care. An option would be to use a parser generator and generate the parser from an EBNF syntax spec, which saves you the process of deriving the EBNF manually from implementation code. As you already opted for maintaining a formal syntax definition, you could as well reap some consequential benefits. Then again, it would probably be a completely different approach. I also do not know a parser gerator capable of generating PHP 5 code. Another viable option would be making it a habit to update the EBNF definition first, then using it as a starting point for the actual implementation. Then, with each release, simply publish both code and documentation as you normally would. This way the EBNF definition does not run any risk of ever becoming outdated. Unless you find other ways of screwing it, of course
          Hide
          Benjamin Eberlei added a comment -

          The problem here is that it was planned for 2.0 and its really hard to keep the EBNF and the actual working stuff in sync. I know its not perfect, i will add a note to the docs soonish, but CASE and path expression were the only things that changed and/or didnt get finished for 2.0

          Show
          Benjamin Eberlei added a comment - The problem here is that it was planned for 2.0 and its really hard to keep the EBNF and the actual working stuff in sync. I know its not perfect, i will add a note to the docs soonish, but CASE and path expression were the only things that changed and/or didnt get finished for 2.0
          Hide
          Daniel Alvarez Arribas added a comment -

          I reopen this as a doc issue, as long as the feature is not yet implemented.

          At the moment, the Doctrine 2 online documentation is patently misleading with respect to the case statement., It documents the case expression as being supported, when in fact it is not even implemented yet. How is a user supposed to know, by reading the docs, that the feature is not yet implemented? I mean, if something in the EBNF definitions, the normal thing is to expect that it is actually supported. People will choose Doctrine 2, because they believe it does what it is documented to do, and instead end up with something that does not. I know Doctrine 2 comes with no warranty. But this is almost deliberate misdocumentation. This was the case with path expressions, and now with the case statement. It is becoming a bad habit.

          Anyway, I am looking forward to full support of case expressions. Until then, I will have to patch my code.

          But, PLEASE, PLEASE, fix the docs, to save prospective users some serious pain. It is just unfair to document features that are not there.

          Show
          Daniel Alvarez Arribas added a comment - I reopen this as a doc issue, as long as the feature is not yet implemented. At the moment, the Doctrine 2 online documentation is patently misleading with respect to the case statement., It documents the case expression as being supported, when in fact it is not even implemented yet. How is a user supposed to know, by reading the docs, that the feature is not yet implemented? I mean, if something in the EBNF definitions, the normal thing is to expect that it is actually supported. People will choose Doctrine 2, because they believe it does what it is documented to do, and instead end up with something that does not. I know Doctrine 2 comes with no warranty. But this is almost deliberate misdocumentation. This was the case with path expressions, and now with the case statement. It is becoming a bad habit. Anyway, I am looking forward to full support of case expressions. Until then, I will have to patch my code. But, PLEASE, PLEASE, fix the docs, to save prospective users some serious pain. It is just unfair to document features that are not there.
          Hide
          Benjamin Eberlei added a comment -

          Case is not fully implemented yet. Its on the roaodmap for 2.1

          Show
          Benjamin Eberlei added a comment - Case is not fully implemented yet. Its on the roaodmap for 2.1

            People

            • Assignee:
              Guilherme Blanco
              Reporter:
              Daniel Alvarez Arribas
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: