8000 Problem with cache generation when PHP tokenizer extension is not available · Issue #14871 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Problem with cache generation when PHP tokenizer extension is not available #14871

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.

Sign up for GitHub

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

Jump to bottom
Closed
mageekguy opened this issue Jun 5, 2015 · 9 comments
Closed

Problem with cache generation when PHP tokenizer extension is not available #14871

mageekguy opened this issue Jun 5, 2015 · 9 comments
Labels
Bug

Comments

@mageekguy
Copy link

If PHP is not compiled with tokenizer extension, there is a bug in the classCollectionLoader class with this kind of code.
The root cause is that the regex use to normalize the code is too generic and is used on all namespace string, even this string is prefixed with (for example) a $ sign.
In this context, a solution is to use a look-behind assertion in the regex to exclude all namespace string which is not prefixed by $, ie. replacing '/namespace(.*?)\s*;/ by '/(?<!\$)namespace(.*?)\s*;/' (see PCRE PHP documentation for more information about assertion).
However, this fix does not handle correctly self::namespace, 'namespace', "namespace" and so on, and it seems to me very difficult to handle all cases in an efficient manner using regex.
In my opinion, the best solution is to transform the PHP tokenizer extension as a mandatory requierement to use Symfony and throwing an exception if it is not available.
Any through?
@cedvan
Copy link
cedvan commented Jun 5, 2015

👍

@jakzal
Copy link
Contributor
jakzal commented Jun 6, 2015

If PHP is not compiled with tokenizer extension, there is a bug in the classCollectionLoader class with this kind of code.

What's the issue exactly? Could you give an example of a namespace handled incorrectly?

@mageekguy
Copy link
Author

Just follow "this kind of code" in my first post…
However, maybe /^|\s+namespace\s(.*?)\s*;/ is the right solution.

@xabbuh
Copy link
Member
xabbuh commented Jun 9, 2015

Just follow "this kind of code" in my first post…

Your issue would be easier to follow if you could provide us example code of a class that would not be parsed properly (which should then be used for creating a test case btw.).

@mageekguy
Copy link
Author

I don't understand why it's so hard to follow an hypertext link… but ok, go ahead…
So this is an example:

    public function parseClass(\ReflectionClass $class)
    {
        if (method_exists($class, 'getUseStatements')) {
            return $class->getUseStatements();
        }
        if (false === $filename = $class->getFilename()) {
            return array();
        }
        $content = $this->getFileContent($filename, $class->getStartLine());
        if (null === $content) {
            return array();
        }
        $namespace = preg_quote($class->getNamespaceName());
        $content = preg_replace('/^.*?(\bnamespace\s+' . $namespace . '\s*[;{].*)$/s', '\\1', $content);
        $tokenizer = new TokenParser('<?php ' . $content);
        $statements = $tokenizer->parseUseStatements($class->getNamespaceName());
        return $statements;
    }

When tokenizer extension is not available, this code become:

    public function parseClass(\ReflectionClass $class)
    {
        if (method_exists($class, 'getUseStatements')) {
            return $class->getUseStatements();
        }

        if (false === $filename = $class->getFilename()) {
            return array();
        }

        $content = $this->getFileContent($filename, $class->getStartLine());

        if (null === $content) {
            return array();
        }

        $namespace = preg_quote($class->getNamespaceName())
{
        $content = preg_replace('/^.*?(\bamespace\s+' . $namespace . '\s*[
{{].*)$/s', '\\1', $content);
        $tokenizer = new TokenParser('<?php ' . $content);

        $statements = $tokenizer->parseUseStatements($class->getNamespaceName());

        return $statements;
    }

@xabbuh
Copy link
Member
xabbuh commented Jun 16, 2015

@mageekguy I just read your description again. So, what you mean is that you cannot load the Doctrine\Common\Annotations\PhpParser class using the ClassCollectionLoader (if tokenizer support is disabled)? Did I understand that correctly?

@mageekguy
Copy link
Author

I'm meaning that when the tokenizer PHP extension is not available, the class Doctrine\Common\Annotations\PhpParser is correctly loaded by the ClassCollectionLoader the first time BUT the version of the class Doctrine\Common\Annotations\PhpParser put in the autoload cache by ClassCollectionLoader is corrupted because the regex used by its in replacement of tokenizer PHP extension is too generic and match not only the namespace token but also things like $namespace.
So, when the ClassCollectionLoader try to load Doctrine\Common\Annotations\PhpParser from the autoload cache, PHP generate a (fatal) parse error.
This is a simple working script to illustrate my statement:

<?php

// extract from https://github.com/doctrine/annotations/blob/master/lib/Doctrine/Common/Annotations/PhpParser.php#L55
$source = '$namespace = preg_quote($class->getNamespaceName());';

function fixNamespaceDeclaration($source)
{
    // extract from https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/ClassLoader/ClassCollectionLoader.php#L140
    if (preg_match('/namespace(.*?)\s*;/', $source)) {
         $source = preg_replace('/namespace(.*?)\s*;/', "namespace$1\n{", $source)."}\n";
    }

    return $source;
}

var_dump($source == fixNamespaceDeclaration($source)); // should be TRUE, is currently FALSE
// because preg_replace generate :
// $namespace = preg_quote($class->getNamespaceName())
// {} => PARSE ERROR!

@xabbuh xabbuh added the Bug label Jun 17, 2015
@xabbuh
Copy link
Member
xabbuh commented Jun 17, 2015

@mageekguy Thanks, it's more clear now. :)

@nicolas-grekas nicolas-grekas mentioned this issue Nov 25, 2015
Merged
@nicolas-grekas
Copy link
Member

See #16668

Tobion added a commit that referenced this issue Nov 25, 2015
@Tobion
bug #16668 [ClassLoader] Fix parsing namespace when token_get_all() i…
27327d3 
…s missing (nicolas-grekas)

This PR was merged into the 2.3 branch.

Discussion
----------

[ClassLoader] Fix parsing namespace when token_get_all() is missing

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #14871
| License       | MIT
| Doc PR        | -

Commits
-------

4a17c9e [ClassLoader] Fix parsing namespace when token_get_all() is missing
@Tobion Tobion closed this as completed Nov 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jakzal @nicolas-grekas @mageekguy @Tobion @xabbuh @cedvan
0