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

Setup the fixtures for the Symfony support #271

Merged
merged 13 commits into from
Oct 31, 2018

Conversation

theofidry
Copy link
Member

@theofidry theofidry commented Oct 26, 2018

Closes #182

  • Yaml support
    • Class param
    • Class as service ID
    • Class as alias
    • Class as alias shortcut
    • Class as argument
    • Class as tag element
    • Whitelisted class
    • Class belonging to a whitelisted namespace
  • XML support
    • Class param
    • Class as service ID
    • Class as alias
    • Class as alias shortcut
    • Class as argument
    • Class as tag element
    • Whitelisted class
    • Class belonging to a whitelisted namespace
  • PHP support
    • Class param
    • Class as service ID
    • Class as alias
    • Class as alias shortcut
    • Class as argument
    • Class as tag element
    • Whitelisted class
    • Class belonging to a whitelisted namespace
  • Neon support?

@theofidry
Copy link
Member Author

/cc @TomasVotruba IIRC you were the one needing the Symfony support the most for Rector or other :)

@TomasVotruba
Copy link
Contributor

This would be great 👍 :)

Indeed, mainly for services.yml

At the moment I have to do it with regular expression like this: https://github.com/rectorphp/prefixer/blob/a10a2a005673e72b4beec4f4611b3b5651e2ca92/src/Worker/PrefixConfigsWorker.php#L29-L33

I still need it for services.neon files, because they're in /vendor as well.

@theofidry theofidry force-pushed the feature/symfony-support branch from dcc3c57 to 9e76ab8 Compare October 30, 2018 09:57
@theofidry theofidry changed the title [WIP] Setup the fixtures for the Symfony support Setup the fixtures for the Symfony support Oct 30, 2018
@theofidry theofidry merged commit e5efabc into humbug:master Oct 31, 2018
@theofidry
Copy link
Member Author

Let's go with the YAML and XML support first. The PHP one should already somewhat be there and for the Neon one I'm not sure but I'll be happy to add it afterwards.

@theofidry theofidry deleted the feature/symfony-support branch October 31, 2018 07:46
@TomasVotruba
Copy link
Contributor

TomasVotruba commented Oct 31, 2018

Agreed 👍 , I'd be to happy to test it ASAP.

As for prefixing Symfony WTFs, I've found that container dump needs to be prefixed extra, since it's string: https://github.com/rectorphp/prefixer/blob/master/src/PrefixFixer.php#L50-L61

@theofidry
Copy link
Member Author

theofidry commented Oct 31, 2018 via email

@TomasVotruba
Copy link
Contributor

TomasVotruba commented Nov 30, 2019

I'm giving this try after year and still end up with same error.

Here is the code we use: rectorphp/rector#2373

Autoconfigure configs are not prefixed:

services:
    _defaults:
        public: true
        autowire: true

    Rector\NetteToSymfony\:
        resource: '../src'
        exclude:
            - '../src/Rector/**/*Rector.php'
            - '../src/Route/RouteInfo.php'
            - '../src/Event/EventInfo.php'

They should be with phar:// prefix, right?

services:
    _defaults:
        public: true
        autowire: true

    Rector\NetteToSymfony\:
        resource: 'phar://../src'
        exclude:
            - 'phar://../src/Rector/**/*Rector.php'
            - 'phar://../src/Route/RouteInfo.php'
            - 'phar://../src/Event/EventInfo.php'

Because when I run:

../tmp/rector.phar 

I get this error:

[ERROR] The                                                                                                            
         "phar:///var/www/rector/tmp/rector.phar/src/HttpKernel/../../config/../packages/
NetteToSymfony/config/../src/Route/RouteInfo.php" directory does not exist in 
phar:///var/www/rector/tmp/rector.phar/src/HttpKernel/../../config/../packages/
NetteToSymfony/config/config.yaml (which is being imported from 
"phar:///var/www/rector/tmp/rector.phar/src/HttpKernel/../../config/config.yaml").  

It's interesting this fails only on exclude and not on resource

I vaguelly recall it was somehow fixed in Symfony, but exclude might have been missed

@TomasVotruba
Copy link
Contributor

Box version 3.8.3@43f13de 2019-11-03 17:18:32 UTC

@TomasVotruba
Copy link
Contributor

After short exploration... it seems it's another Symfony bug.
The error is caused here: https://github.com/symfony/symfony/blob/c62bb82e27974ef4e389da523f0de451b6632266/src/Symfony/Component/Finder/Finder.php#L589

This was similar fix, might be related: symfony/symfony@b61c8fc

@theofidry
Copy link
Member Author

@TomasVotruba it's been a while so I may remember wrong, but it looks weird to me. The bug you report makes sense to me in the context of the bundled application, i.e. the PHAR of Rector, compiles its containers. This should not happen, see https://github.com/humbug/box/blob/master/doc/symfony.md#symfony-support for a more detailed explanation as to why

@TomasVotruba
Copy link
Contributor

@theofidry
Copy link
Member Author

theofidry commented Nov 30, 2019

It might be but I'm point is the reason why you're hitting that bug in the first place might be the real issue here, at least for your case.

If the Symfony compiler compiles within the PHAR, it can never work because the PHAR is supposed to be read only (cf. the doc linked above)

@TomasVotruba
Copy link
Contributor

Oh. So what would be the solution?
I didn't get that from the link

@TomasVotruba
Copy link
Contributor

There are no assets, just services

@theofidry
Copy link
Member Author

theofidry commented Nov 30, 2019

There are no assets, just services

I'm not really following how that is related. I'm suggesting - I don't know if that's the real issue, that when you package Rector in the PHAR and then use it, because it has not been packaged "properly" (as a Symfony app requires a bit more tweaking up-front), it attemps to compile the container which will 200% fail and will never ever work by design. A PHAR is a ready-only environment so compiling the container which dumps various files in its cache directory won't work out.

The suggestions in the article are to prevent that:

  • make sure that your env variables are dumped if any
  • make sure your assets are dumped if any
  • make sure your cache is warmed up (which will ensure the compiler is compiled
  • make sure that all the above happens in prod mode with debug set to false
  • make sure that your app running in the PHAR remains in prod mode with debug set to false

The article is a "standard" way to do so with a regular minimal Symfony app (and is tested within Box). It might however need adjustments depending on how you use the framework

@TomasVotruba
Copy link
Contributor

Rector doesn't have any env variables. That's why I don't get it.
Also it's container is dumped in normal way to temp cache. Like any other cache, that changes based on the configuration so it can't be persisted.

There were 2 issues with Symfony config resource/exclude bugs in PHAR. Nothing more yet. I've found saint workarounds that compensate bugs.

Finally the container is build and app run, yay:

good_times

Now more testing is coming :)

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.

Add frameworks integration
2 participants