Error with tokenizer for JOINs (comments include proposed solutions)

As reported here:
http://forum.symfony-project.org/index.php/t/24441/

It seems that the problem is with the doctrine:build-all-reload in symfony and not anything deeper.

As such, I think it would make sense to disactivate that task in symfony (rather than marking it as deprecated) since it is broken and a note in it directing people to use the new tasks would be far more useful.

C

As noted above, this is not a Critical error with Doctrine, but rather a problem with a symfony task.

On further inspection, this is not an error with the symfony task, but a problem with the Tokenizer class, in particular where Doctrine_Query_From::parse() calls Doctrine_Query_Tokenizer::bracketExplode() with the delimiter set to 'JOIN'.

OK... I think I have found and solved this one but a more experienced member of the Doctrine team should review what I propose before adding it to the codebase.

The problem occurs because Doctrine_Query_Tokenizer::getSplitRegExpFromArray() is being passed a string with no word boundaries (the parameter being passed to it by Doctrine_Query_From::parse() is "JOIN"). getSplitRegExpFromArray() then treats this string as a case insensitive regex so if any of your class or field names contain this string they are treated as a split.

E.g. "JoinFooBar JOIN BanjoIndia" would be split into array ("", "FooBar", "Ban", "dia").

This is clearly wrong and would mean that no table or fieldnames could contain the letters j-o-i-n in that order.

The proposed solution is as follows:

Current version of Doctrine_Query_Tokenizer::getSplitRegExpFromArray() (Doctrine 1.2, as of r7034):

 
    private function getSplitRegExpFromArray(array $d){
        $d = array_map('preg_quote', $d);

        if (in_array(' ', $d)) {
            $d[] = '\s';
        }

        return '#(' . implode('|', $d) . ')#';
    }

Proposed modification checks each delimiter given. If that delimiter consists only of \w characters (i.e. [0-9a-zA-Z_]) then a word boundary is required before and after that delimiter if it's going to be used as a match:

 
    private function getSplitRegExpFromArray(array $d){
        foreach ($d as $key => $string) {
            $escapedString = preg_quote($string);
            if (preg_match('#^\w+$#', $string)) $escapedString = "\W$escapedString\W";
            $d[$key] = $escapedString;
        }

        if (in_array(' ', $d)) {
            $d[] = '\s';
        }

        return '#(' . implode('|', $d) . ')#';
    }

Now, this seems to work for me in brief testing for my own purposes. However, the test cases in TokenizerTestCase.php (particularly testBracketExplode()) are far from exhaustive so I'm not sure if this proposed solution would suit all cases. E.g. do you ever need the delimiter to match at the very beginning or end of a string? At the moment it would not match but you could deal with this by using this instead:

 if (preg_match('#^\w+$#', $string)) $escapedString = "(^|\W)$escapedString($|\W)";

An alternative solution, but far less robust, would be just to modify Doctrine_Query_From::parse() from

    public function parse($str, $return = false)
    {
        $str = trim($str);
        $parts = $this->_tokenizer->bracketExplode($str, 'JOIN');

        $from = $return ? array() : null;
...

to this, with spaces around the JOIN string passed to bracketExplode():

    public function parse($str, $return = false)
    {
        $str = trim($str);
        $parts = $this->_tokenizer->bracketExplode($str, ' JOIN ');

        $from = $return ? array() : null;
...

This is probably going to run marginally faster, but it leaves the risk that some change in future will bring up the same problem again, so the former (fixed regexp) solution would seem to be better for stability.

I hope you can pick this up, review it and implement the most suitable change (and update the test cases too) - at the moment this is a big bug since any schema which defines table or fieldnames that contain the consecutive letters j-o-i-n will break.

C

Matthias,

feel free to use the fix detailed above. I have been using this in my local copy of Doctrine and it seems to work well so far.

We just need a member of Doctrine's dev team to decide which of the fixes above makes most sense and then to update the code for the next Doctrine release.

The fix I am using at the moment is the first one:

          if (preg_match('#^\w+$#', $string)) $escapedString = "\W$escapedString\W";

Hi, before I can test your proposed solution. I need to see a patch with your changes. I can't very easily and reliably copy and paste your changes from the comments.

Hi Jon,

Thanks for looking into this.

I will generate a patch and attach it to this ticket for your review.

Feel free to rip it apart and change the test cases or regex used.

C

Patch with suggested fix for the tokenizer and basic test case.