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 alternative - build PrefixedRector #449

Closed
wants to merge 1 commit into from

Conversation

mssimi
Copy link
Contributor

@mssimi mssimi commented Apr 29, 2018

It is not complete, but already got working proof of concept:)

I'm taking a little break now that regular expressions killing me:)

rector-prefixer

What is missing:

  1. Get tests working(php.inc is not prefixed)
  2. Fix error found by tests
  3. Git push build dir somewhere to github.

@TomasVotruba
Copy link
Member

Great job 👍 I can handle regulars, if that's pain for you.

It looks like wrong screenshot, since it replaces classes.

@mssimi
Copy link
Contributor Author

mssimi commented Apr 29, 2018

It is not wrong screen I wanted to make simple preview it is already somehow working:D Notice I run that under build directory.

@mssimi
Copy link
Contributor Author

mssimi commented Apr 29, 2018

As of regular expressions I had to use perl instead sed as I was unable to get that working with sed.

@TomasVotruba
Copy link
Member

TomasVotruba commented Apr 29, 2018

I see. Yesterday we talked about NamespaceReplacer and now you're using ClassReplacer, it does different thing :).

NamespacePrefixer is the goal and the screen doesn't do it.

@@ -0,0 +1,14 @@
-----BEGIN PUBLIC KEY-----
MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAxKi/ABhiCMn7hqH9/W0x
Copy link
Member

Choose a reason for hiding this comment

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

PHP Scoper should be a dependnecy in composer.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In docs it is not recommended.

Copy link
Member

@TomasVotruba TomasVotruba Apr 29, 2018

Choose a reason for hiding this comment

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

Docs is not a dog, nor God :)
Still, should be dependency in composer.json. Without that we have to maintain phar manually.

What advantages do you see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conflict avoidance.

Choose a reason for hiding this comment

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

bamarni/composer-bin-plugin or phive can both help out there (see this article)[https://medium.com/@tfidry/managing-your-dependencies-in-php-321d584441ab]. But I advise against relying on committing the PHAR:

  • it's really heavy on the Git history
  • it's much harder to cherry-pick a branch to try a bugfix or something

If you want to go that way nonetheless, I recommend to mark the public key as a binary file as well in .gitattributes

Copy link
Member

Choose a reason for hiding this comment

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

I just tried bamarni/composer-bin-plugin but it cannot work due to php-parser 3/4 conflict.

image

Copy link
Member

Choose a reason for hiding this comment

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

So phar seems as the best option for the time being. It doesn't evolve much and the php-parser 4 + better reflectoin is main blocker there, so missing update-ability won't be such a problem.

@mssimi
Copy link
Contributor Author

mssimi commented Apr 29, 2018

This pull request is not about NamespacePrefixerRector this is about PHAR alternative. I made working prefixed rector w/o creating custom Rector for that.

@TomasVotruba
Copy link
Member

Oh, the image it not for the PR you provided, but for the tool it generated. I get it now

@theofidry
Copy link

@mssimi what about doing that in PHP-Scoper directly? I think it would be more interesting to have it able to scope a Symfony application out of the box rather than requiring every user to meddle with regexes

@TomasVotruba TomasVotruba mentioned this pull request May 11, 2018
7 tasks
@TomasVotruba
Copy link
Member

@theofidry

@mssimi what about doing that in PHP-Scoper directly? I think it would be more interesting to have it able to scope a Symfony application out of the box rather than requiring every user to meddle with regexes

What exactly do you mean by this?

@theofidry
Copy link

I mean instead of trying to modify the YAML service files like that, a support could be added to PHP-Scoper to be able to change the YAML files directly

@TomasVotruba
Copy link
Member

TomasVotruba commented May 11, 2018

I see, that's more clear, thanks. I can look on that later.

At the moment I'd like skip Rector\* classes in prefixing, so this can be removed: https://github.com/rectorphp/rector/pull/449/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R69

How to do that?

I found only Whitelist: https://github.com/humbug/php-scoper#whitelist:

return [
    'whitelist' => [
        'PHPUnit\Framework\TestCase',
    ],
];

but it seems only accepts fully class names only. We need:

return [
    'whitelist' => [
        'Rector\*',
    ],
];

@theofidry
Copy link

theofidry commented May 11, 2018

I found only Whitelist: https://github.com/humbug/php-scoper#whitelist, but it seems only accepts fully class names only

Yes for now. We need to find an easier way to whitelist stuff (see humbug/php-scoper#192 (comment)) and extend it to constants and functions as well (potentially way more tricky).

@TomasVotruba
Copy link
Member

TomasVotruba commented May 11, 2018

How to do it now?

I used RobotLoader for now.

@TomasVotruba
Copy link
Member

TomasVotruba commented May 17, 2018

Continues in #465

@mssimi Thanks for you work, I'm using ~80 % of it in following PR!
Slowly but surelly we're getting there

TomasVotruba added a commit that referenced this pull request Jul 17, 2021
rectorphp/rector-src@e3f0952 github-actions: declare timeout for job execution (#449)
echo511 pushed a commit to echo511/rector that referenced this pull request Sep 4, 2021
the default of 6h is way to long and produced a overlong queue in case of endless loop bugs

Co-authored-by: Markus Staab <[email protected]>
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