Doctrine 2 - ORM
  1. Doctrine 2 - ORM
  2. DDC-2052

Custom tree walkers are not allowed to add new components to the query

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.5
    • Component/s: DQL
    • Labels:

      Description

      Custom tree walkers have freedom in modifying the AST but when you try to add a new query component (i.e. new join in walkSelectStatement() ) to the AST then the SqlWalker throws an exception because it does not has the new component in its _queryComponents array. I see two possible ways to resolve this:
      1. Modify the Parser class in order to allow tree walkers to modify queryComponents and pass changed queryComponents to the SqlWalker
      2. Improve SqlWalker so it can extract and prepare needed information about queryComponent based on AST when it does not have them.

        Activity

        Hide
        Benjamin Eberlei added a comment -
        Show
        Benjamin Eberlei added a comment - Partial fix in https://github.com/doctrine/doctrine2/pull/934
        Hide
        Andreas H added a comment -

        My workaround is to overwrite the TreeWalkerChain class (using Composer) and pulling the query components from the last iteration in walkSelectStatement to the next one.

        Show
        Andreas H added a comment - My workaround is to overwrite the TreeWalkerChain class (using Composer) and pulling the query components from the last iteration in walkSelectStatement to the next one.
        Hide
        Marco Pivetta added a comment - - edited

        As a VERY UGLY workaround, I used a static variable and a custom sql walker in combination with my AST walker.

        
        namespace Comcom\Versioning\ORM\Query;
        
        
        use Doctrine\ORM\Query\SqlWalker;
        
        class WorkaroundSqlWalker extends SqlWalker
        {
            public function __construct($query, $parserResult, array $queryComponents)
            {
                parent::__construct($query, $parserResult, $queryComponents);
        
                foreach (VersionWalker::$additionalAliases as $alias => $value) {
                    $this->setQueryComponent($alias, $value);
                }
            }
        }
        
        Show
        Marco Pivetta added a comment - - edited As a VERY UGLY workaround, I used a static variable and a custom sql walker in combination with my AST walker. namespace Comcom\Versioning\ORM\Query; use Doctrine\ORM\Query\SqlWalker; class WorkaroundSqlWalker extends SqlWalker { public function __construct($query, $parserResult, array $queryComponents) { parent::__construct($query, $parserResult, $queryComponents); foreach (VersionWalker::$additionalAliases as $alias => $value) { $ this ->setQueryComponent($alias, $value); } } }
        Hide
        Marco Pivetta added a comment -

        Just hit this while developing an ast walker... Will look into it too since I need it more than soon.

        Show
        Marco Pivetta added a comment - Just hit this while developing an ast walker... Will look into it too since I need it more than soon.
        Hide
        Benjamin Eberlei added a comment -

        Marked as improvement as its not a bug.

        A solution might probably implement an object holding all the QueryComponent, implementing ArrayAccess. So that way the state can be shared.

        Show
        Benjamin Eberlei added a comment - Marked as improvement as its not a bug. A solution might probably implement an object holding all the QueryComponent, implementing ArrayAccess. So that way the state can be shared.
        Hide
        Łukasz Cybula added a comment -

        I've tried to implement the solution mentioned in previous comment but it's also not so clean and easy as I thought. Each tree walker (including TreeWalkerChain) would have to implement getQueryComponents() and setQueryComponent($alias, array $component) methods. The same with SqlWalker, so the TreeWalker interface should have these methods, which would break BC in some way (walkers that do not inherit from SqlWalker or TreeWalkerAdapter will fail to compile). So maybe my first solution (PR #464) is not so bad for now? In the future queryComponents could be replaced by a special object or could be passed by a reference to allow modifications.

        Show
        Łukasz Cybula added a comment - I've tried to implement the solution mentioned in previous comment but it's also not so clean and easy as I thought. Each tree walker (including TreeWalkerChain) would have to implement getQueryComponents() and setQueryComponent($alias, array $component) methods. The same with SqlWalker, so the TreeWalker interface should have these methods, which would break BC in some way (walkers that do not inherit from SqlWalker or TreeWalkerAdapter will fail to compile). So maybe my first solution (PR #464) is not so bad for now? In the future queryComponents could be replaced by a special object or could be passed by a reference to allow modifications.
        Hide
        Łukasz Cybula added a comment -

        I'm afraid that this doesn't solve the initial problem at all. I'll try to describe it in more details to show what I mean. Suppose we have two doctrine extensions each of which contain its own tree walker. Each of these tree walkers need to modify AST and add new component to it (joined with some component already existing in the query). The first problem is that each tree walker has its own queryComponents array which is not passed between them, although they not necessary need to use queryComponents - they could use only AST. The second, bigger problem is that the Parser class does not know anything about modifications of queryComponents in tree walkers and cannot pass modified version to the OutputWalker. The goal of submitting this issue was to allow adding new components to the query in tree walkers which is not achievable by your fix. I think it may be the first step in the right direction. Maybe TreeWalkerAdapter should have public method getQueryComponents() which would be used by the Parser to pass modified queryComponents between different tree walkers and finally to the OutputWalker ? This would not break backward compatibility and solve this issue. What do you think about it?

        Show
        Łukasz Cybula added a comment - I'm afraid that this doesn't solve the initial problem at all. I'll try to describe it in more details to show what I mean. Suppose we have two doctrine extensions each of which contain its own tree walker. Each of these tree walkers need to modify AST and add new component to it (joined with some component already existing in the query). The first problem is that each tree walker has its own queryComponents array which is not passed between them, although they not necessary need to use queryComponents - they could use only AST. The second, bigger problem is that the Parser class does not know anything about modifications of queryComponents in tree walkers and cannot pass modified version to the OutputWalker. The goal of submitting this issue was to allow adding new components to the query in tree walkers which is not achievable by your fix. I think it may be the first step in the right direction. Maybe TreeWalkerAdapter should have public method getQueryComponents() which would be used by the Parser to pass modified queryComponents between different tree walkers and finally to the OutputWalker ? This would not break backward compatibility and solve this issue. What do you think about it?
        Hide
        Benjamin Eberlei added a comment -

        Added setQueryComponent() in SQL Walker to allow modification in output walker.

        Show
        Benjamin Eberlei added a comment - Added setQueryComponent() in SQL Walker to allow modification in output walker.
        Hide
        Benjamin Eberlei added a comment -

        Ok this is much more complicated to allow then i thought. The problem is that the QueryComponents are passed by value, as an array, not by reference. That prevents changing them because this change wouldn't be visible in the output walker.

        I can add a method to allow this in the OutputWalker for now, but generally this requires a bigger refactoring on the Query Components.

        Show
        Benjamin Eberlei added a comment - Ok this is much more complicated to allow then i thought. The problem is that the QueryComponents are passed by value, as an array, not by reference. That prevents changing them because this change wouldn't be visible in the output walker. I can add a method to allow this in the OutputWalker for now, but generally this requires a bigger refactoring on the Query Components.

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Łukasz Cybula
          • Votes:
            2 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: