Build just one Docker image with PHP 8 and without scoping

[mpdude]

Since #5665, we might get away with the PHP 8.0 image variant only, and also no longer need to scope the Rector source code.

Also see this comment.

[TomasVotruba]

Scoping is still needed. There are some rare cases, like annotation parsing or native method reflection, that require native class to be loaded.

But that's only guess... It would be better to have this covered by test case right here, e.g. installing Symfony Console 2.8 on PHP 5.6 with one simple Command implementation and running PHP 8 Docker image on it.

[mpdude]

There are some rare cases, like annotation parsing or native method reflection, that require native class to be loaded.

Do you have a more concrete example? Maybe an old issue, a test case or so?

It would be better to have this covered by test case right here, e.g. installing Symfony Console 2.8 on PHP 5.6 with one simple Command implementation and running PHP 8 Docker image on it.

As an ad hoc demo or as a permanent test case?

If that works, would you say that scoping is not necessary after all?

[TomasVotruba]

I don't know about any specific issue.

As an ad hoc demo or as a permanent test case?

Part of our permanent CI test battery. The rector-prefixed is tested in similary way here, installed allong Symfony 2.8: https://github.com/rectorphp/rector-prefixed/blob/master/.github/workflows/composer_dependency.yaml

If that works, would you say that scoping is not necessary after all?

We'll see

[mpdude]

So let’s add these tests via #5784 and come back here afterwards.

[mpdude]

@TomasVotruba Here is another argument why it would be helpful if we could build the Docker image without scoping:

When I add a custom rule as described in https://github.com/rectorphp/rector/blob/main/docs/create_own_rule.md, and I need to run Docker because I don't have PHP 7.3+ locally, then I can use

docker run --init --rm -v $(pwd):/project rector/rector process --autoload-file /project/vendor/autoload.php src

When Rector in Docker is scoped, this fails because classes (for example, Nette\Utils\Strings) cannot be found. They have been moved into the "scoped" namespace.

When Rector-in-Docker is not scoped, it works.

So, what do you (or @JanMikes) think about this change here?

I've tried in #5784 to add tests that show what can be done without scoping, but of course, merging that is not necessary for a decision here.

[TomasVotruba]

As I don't use Docker in active way with Rector, I can only give theoretical suggestions. They might not be as useful as someone who is using Docker daily.
I believe it would be better leave further process of this and #5784 on @JanMikes descissions.

[TomasVotruba]

There's been lot of refactoring last 2 weeks and Rector 0.10 is out now 🥳

Could you update/rebase your PR to match current main? Thanks

[JanMikes]

Honestly, i do not know.

I feel like it is useful to run php version that is as close to the project php version as possible (but it is only my feeling, not an argument why have multiple php versions in docker).

It would need some testing - etc run rector in 0.9 in php 8.0 on 5.4 code that will fail. Then run it with 0.10 and it will not fail - in that case we are completely okay to have single bundled Docker image.

[JanMikes]

without scoping

I think this is not an option. Because there might be different versions of class with different api.

So the desired state would be to keep scoped rector in docker and only decide (based on tests) if we can use always php 8.0 thanks to static reflection or we need multiple php versions.

[TomasVotruba]

As explained above by @JanMikes , the scoping is neccessary in Docker too to avoid same-package of different version conflicts. It is not related to PHP version. Closing then.

@mpdude Thank you for your work though.

[mpdude]

Sorry for the delay, I had a few days off.

@JanMikes IMO, #5784 shows that Rector (thanks to static reflection) can now handle PHP 7.x and 8.x code, regardless of which PHP version is running it.

Also, it contains a fixture "project" (here) that brings along a faked Symfony Console OutputInterface. Before static reflection, this would have been loaded and messed up Rector's autoloader: The "wrong" version would have been used, leading to API problems or, in the case of my test case, to an unhandled exception.

But all this now works 🎉, so I think removing the scoping would be safe.

When Rector is scoped, how could I use my own rules (like described in https://github.com/rectorphp/rector/blob/main/docs/create_own_rule.md)? Isn't that though, since all Rector classes have been moved to a (random) namespace?

[TomasVotruba]

But all this now works tada, so I think removing the scoping would be safe.

That's just a chance, not a rule. It depends on your project autoloader and race conditions. Static reflection is a chain of sources providers. See: https://phpstan.org/blog/zero-config-analysis-with-static-reflection#hybrids-are-better-for-the-environment

With all that said, PHPStan can still use runtime reflection where appropriate. So it’s hybrid reflection, not entirely static. Static reflection is hungrier for CPU time and RAM allocation, so if PHPStan concludes that a class can be autoloaded, it will use runtime reflection for that one, and static reflection for the rest.

That means your project used static reflection to autoload OutputInterface. But it can also use projects' autoload which would load 2 classes - one in your project and one in Rector and create the fatal error.

[mpdude]

Do you have an example for that, or can you tell me which rules/conditions I would need to follow to create one?

[TomasVotruba]

There are older issues here, just search the conflict error message