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

PHAR improvements #389

Closed
wants to merge 10 commits into from
Closed

PHAR improvements #389

wants to merge 10 commits into from

Conversation

mssimi
Copy link
Contributor

@mssimi mssimi commented Mar 29, 2018

Related to #319

Simplified and greatly improved performance of PHAR building script by using rsync.
Add composer script to build PHAR.
You can now ignore PHAR files in .pharignore.
Included an example for prefixing vendors using rector(performance is bad here so example is just for small folder).

Atm there is problem with autowring in PHAR as Symfony uses glob and it is not supported by PHAR.
At least this class will have to be re-implemented. I may add test for it later.

composer.json Outdated
"phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon"
"phpstan": "vendor/bin/phpstan.phar analyse packages src tests --level max --configuration phpstan.neon",
"compile-phar": [
"rm -f rector.phar",
Copy link
Member

Choose a reason for hiding this comment

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

For reasons we've talked about and WTF like #319 (comment)
I'd recommend to use nested directory for build, e.g. common build, so file in build/rector.phar would fail on missing vendor/autoload.php.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what about bin/rector.phar?

Copy link
Member

@TomasVotruba TomasVotruba Mar 29, 2018

Choose a reason for hiding this comment

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

In /bin only bin files should be. build is for the extra generated stuff

Don't worry, it will be usable as vendor/bin/rector.phar in the end.

services:
Rector\Rector\Dynamic\NamespaceReplacerRector:
$oldToNewNamespaces:
'Jean85': '%prefix%Jean85'
Copy link
Member

Choose a reason for hiding this comment

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

Very creative use of Rector, like it 👍

Love the param! :)
There could be dynamic Rector (via RectorProvider), that would replace every namespace node with prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every except specified would be even better:) We don't need to prefix Rector itself.

Copy link
Member

Choose a reason for hiding this comment

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

I see :) That would require new Rector, probably NamespacePrefixerRector

services:
    Rector\Rector\Dynamic\NamespaceReplacerRector:
        !Rector: Prefixed # all but "Rector" prefix by "Prefixed"

What do you need to know from me to write it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will know that, after I try it:) But I feel like performance is bigger issue here, can we somehow speed it up?

Copy link
Member

Choose a reason for hiding this comment

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

First things first. When PHAR is done, we can improve performance.

Doing both at same time improved neither of both, tried for you :D

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm with Tomas here. Let's first fix this PHAR issue, than we can join forces together and make Rector as fast as we can 💪

@mssimi
Copy link
Contributor Author

mssimi commented Mar 30, 2018

Added test for GlobResource. I could use some help here.

@mssimi
Copy link
Contributor Author

mssimi commented Apr 2, 2018

realpath() also seems buggy in PHAR.

@TomasVotruba TomasVotruba mentioned this pull request Apr 3, 2018
3 tasks
@Tuzex Tuzex self-assigned this Apr 21, 2018

$this->assertNotCount(0, $resource);
}
}
Copy link
Member

@TomasVotruba TomasVotruba Apr 21, 2018

Choose a reason for hiding this comment

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

@edobarnas Here it's needed to override getContainerLoader() in AppKernel with own PharAwaretGlobFileLoader class:

final class RectorKernel extends Kernel
{

    /**
     * @param ContainerInterface|ContainerBuilder $container
     */
    protected function getContainerLoader(ContainerInterface $container): DelegatingLoader
    {
        $kernelFileLocator = new FileLocator($this);

        $loaderResolver = new LoaderResolver([
            // @todo - create this clsas
            return PharAwareGlobFileLoader($container, $kernelFileLocator));
            // ...
        ]);

        return new DelegatingLoader($loaderResolver);
    }

}

As done in here https://github.com/symplify/packagebuilder#10-do-you-need-to-merge-parameters-in-yaml-files-instead-of-override


PharAwareGlobFileLoader might look like this:

namespace Symfony\Component\Config\Loader\GlobFileLoader;

final class PharAwareGlobFileLoader extends GlobFileLoader
{
    protected function glob(string $pattern, bool $recursive, &$resource = null, bool $ignoreErrors = false)
    {
        // override parent glob() method logic to not-use "glob" function
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

What do you think @mssimi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check GlobResource usages, I think it is not only used in loader.

@TomasVotruba
Copy link
Member

Follow up in #449

TomasVotruba added a commit that referenced this pull request Jul 6, 2021
rectorphp/rector-src@95e6a5e [CodingStyle] Handle FuncGetArgsToVariadicParamRector in Method/Static Call (#389)
echo511 pushed a commit to echo511/rector that referenced this pull request Sep 4, 2021
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.

4 participants