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

Styleci bridge #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

soullivaneuh
Copy link

As discussed on Gitter.

cc @theofidry

@webmozart
Copy link
Member

Thanks for this! :)

// PHP-CS-Fixer 2.x
if (method_exists($config, 'setRules')) {
$config
->setRiskyAllowed(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Risky fixers are not allowed by default on 2.x.

But it's possible you don't need it with your configuration. Will try and remove it if it's true.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this?

@webmozart
Copy link
Member

When I try this, I get a:

PHP Fatal error: Call to undefined method Symfony\Component\Finder\Iterator\RecursiveDirectoryIterator::isAccepted()

with php-cs-fixer 1.10.1. Is this familiar?

@soullivaneuh
Copy link
Author

Well, never get this error.

Could you please give me the complete entered command and composer info -i result?

@soullivaneuh
Copy link
Author

I reproduced it but I didn't find where is the issue yet.

You can avoid this error by setting prefer-stable to true on your composer.json.

@soullivaneuh
Copy link
Author

More strange thing, the error seems to come from your code:

PHP Fatal error:  Call to undefined method Symfony\Component\Finder\Iterator\RecursiveDirectoryIterator::isAccepted() in /home/sullivan/projects/fork/puli-cli/vendor/symfony/finder/Iterator/FilenameFilterIterator.php on line 30

@webmozart
Copy link
Member

Ah, I just realized what the problem is. You are loading the project's autoloader in .php_cs, which may conflict with the libraries bundled with php-cs-fixer (and that seems to be the case right now).

@soullivaneuh
Copy link
Author

You are loading the project's autoloader in .php_cs

Yes, this is needed to get the bridge working.

which may conflict with the libraries bundled with php-cs-fixer (and that seems to be the case right now)

Yeah, but what I don't understand is the location of the error. Regarding php trace, it seems to provide from your code. That makes no sense to me. Any idea?

@soullivaneuh
Copy link
Author

Well, I was wrong this is coming from Symfony vendor, not your code.

But it's not an error from my lib call, this is hard to debug.

@soullivaneuh
Copy link
Author

@webmozart Could you tell me if you still get this error with the following modification?

If it's solved, I'll try to make a custom loader on my project to prevent this kind of things.

Ref: composer/composer#1493 (comment)

@soullivaneuh
Copy link
Author

@webmozart Should be ok this time. :-)

I released a new patch version of the bridge with a custom loader to avoid this kind of conflicts. 👍

@webmozart
Copy link
Member

Nice, thanks :) How do you run PHP-CS-Fixer? The tool hangs for me.. there must be an infinite loop somewhere.

@webmozart
Copy link
Member

Ah, no! Apparently it just takes a lot of network bandwidth and I'm on a slow connection right now. Why is that so?

@soullivaneuh
Copy link
Author

it just takes a lot of network bandwidth and I'm on a slow connection right now. Why is that so?

The bridge does not download anything... Oo

What is your command?

Just run php-cs-fixer fix -v on your project root directory and you are good to go.

@soullivaneuh
Copy link
Author

ping @webmozart

@webmozart
Copy link
Member

Hm, seems to work now.

@soullivaneuh
Copy link
Author

Hm, seems to work now.

I ensure you that no internet connection is done from the bridge.

If you have some, this is from another library / tools.

@webmozart
Copy link
Member

I just reviewed your autoload.php, but I don't get how this is different now. What's the point of this custom class loader? You are still forwarding everything to the project's autoloader, no?

@soullivaneuh
Copy link
Author

You are still forwarding everything to the project's autoloader, no?

Yes, but I only load needed classes on runtime. So, no conflict like you got.

@webmozart
Copy link
Member

Yes, but I only load needed classes on runtime.

Can you explain this a bit more? I thought loading the classes at runtime was the purpose of Composer's autoloader?

@soullivaneuh
Copy link
Author

I thought loading the classes at runtime was the purpose of Composer's autoloader?

I'm not an expert but I don't think so. Seems all classes are registered.

That why the unregister method of the default autoloader is called on composer/composer#1493 (comment).

@soullivaneuh
Copy link
Author

BTW, this autoload is used only for php-cs-fixer. No side effect with your project.

@webmozart
Copy link
Member

I'm not an expert but I don't think so. Seems all classes are registered.

Registered, yes, but not loaded. Classes are only loaded when they are used (e.g. new <Class>()), so your class loader doesn't make any difference except for calling __static() after loading a class (why?).

@soullivaneuh
Copy link
Author

Well, you're right, I asked Seldaek for more info.

I think this is related to autoload_real.php. I think some class are loaded inside:

// autoload_real.php @generated by Composer

class ComposerAutoloaderInit065a5b652db51bcdb5d3d6134bf669ad
{
    private static $loader;

    public static function loadClassLoader($class)
    {
        if ('Composer\Autoload\ClassLoader' === $class) {
            require __DIR__ . '/ClassLoader.php';
        }
    }

    public static function getLoader()
    {
        if (null !== self::$loader) {
            return self::$loader;
        }

        spl_autoload_register(array('ComposerAutoloaderInit065a5b652db51bcdb5d3d6134bf669ad', 'loadClassLoader'), true, true);
        self::$loader = $loader = new \Composer\Autoload\ClassLoader();
        spl_autoload_unregister(array('ComposerAutoloaderInit065a5b652db51bcdb5d3d6134bf669ad', 'loadClassLoader'));

        $map = require __DIR__ . '/autoload_namespaces.php';
        foreach ($map as $namespace => $path) {
            $loader->set($namespace, $path);
        }

        $map = require __DIR__ . '/autoload_psr4.php';
        foreach ($map as $namespace => $path) {
            $loader->setPsr4($namespace, $path);
        }

        $classMap = require __DIR__ . '/autoload_classmap.php';
        if ($classMap) {
            $loader->addClassMap($classMap);
        }

        $loader->register(true);

        $includeFiles = require __DIR__ . '/autoload_files.php';
        foreach ($includeFiles as $file) {
            composerRequire065a5b652db51bcdb5d3d6134bf669ad($file);
        }

        return $loader;
    }
}

function composerRequire065a5b652db51bcdb5d3d6134bf669ad($file)
{
    require $file;
}

BTW, removing the __static() still working.

@soullivaneuh
Copy link
Author

@webmozart I asked for that. Please see composer/composer#1493 (comment).

@theofidry
Copy link

@soullivaneuh any progress?

@soullivaneuh
Copy link
Author

@theofidry for me the PR is ready.

Waiting @webmozart confirmation since the question asked to Seldaek.

@soullivaneuh
Copy link
Author

Hi @webmozart, going back to this old and tricky PR. :-)

I removed the custom autoload requirement.

Your issue mentioned in #21 (comment) looks to happen only when running php-cs-fixer from a global composer installation.

Tries with the .phar and with a local composer installation, working great actually.

I would just recommend to use the .phar and I think this is also the recommended way from PHP-CS-Fixer itself.

I already have strange errors like this with composer globally installed tools.

Recently with PHPUnit, telling me test passes when it should fail because I didn't require symfony/dependency-injection. Why? Simply because this dependency is globally installed and loaded too.

Ready to merge for my side. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants