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

Extract static variable $regex into property #13

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Extract static variable $regex into property #13

merged 2 commits into from
Jul 30, 2019

Conversation

kstkn
Copy link

@kstkn kstkn commented Feb 28, 2018

Method \Doctrine\Common\Lexer\AbstractLexer::scan contains static variable $regex, which shares state between class instances. Consider following example, in which second instance ($lexer2) has different catchable pattern ([a-z]+), but still behaves like the first one:

class Lexer extends \Doctrine\Common\Lexer\AbstractLexer
{
    private $catchablePatterns = [];

    public function addCatchablePattern($pattern)
    {
        $this->catchablePatterns[] = $pattern;
    }

    protected function getCatchablePatterns()
    {
        return $this->catchablePatterns;
    }

    protected function getNonCatchablePatterns()
    {
        return ['[\s,]+'];
    }

    protected function getType(&$value)
    {
        return 1;
    }
}

$lexer1 = new Lexer();
$lexer1->addCatchablePattern('[a-z]');
$lexer1->setInput('one');
$token = $lexer1->glimpse();
var_dump($token['value']);
// string(1) "o"

$lexer2 = new Lexer();
$lexer2->addCatchablePattern('[a-z]+');
$lexer2->setInput('one');
$token = $lexer2->glimpse();
var_dump($token['value']);
// string(1) "o"

@jwage
Copy link
Member

jwage commented Apr 11, 2018

This makes sense. Unless there is some valid reason why it was done this way. For performance or something, but I doubt it. Can you add a unit test?

@jwage jwage added this to the v1.1.0 milestone Apr 16, 2018
@kstkn
Copy link
Author

kstkn commented May 14, 2018

@jwage test added

@jwage jwage requested a review from guilhermeblanco May 14, 2018 14:33
@jwage
Copy link
Member

jwage commented May 14, 2018

@guilhermeblanco can you take a look at this?

@guilhermeblanco
Copy link
Member

The static was added exactly for performance reasons, but can be addressed by using the private variable, for sure. We did that before because we could b using the same Lexer for multiple queries and didn't want to rebuild the regex every time (as Lexer in ORM is not shared).

@guilhermeblanco
Copy link
Member

Anyway, LGTM

@kstkn
Copy link
Author

kstkn commented Jun 5, 2018

@jwage

@alcaeus alcaeus self-assigned this Jul 30, 2019
@alcaeus alcaeus removed the request for review from guilhermeblanco July 30, 2019 19:23
@alcaeus alcaeus merged commit e17f069 into doctrine:master Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants