Do not load both preload files when both conditions hold

[SanderMuller]

bin/rector.php picks a preload file from two conditions, written as two separate if statements:

if (file_exists(__DIR__ . '/../preload.php') && is_dir(__DIR__ . '/../vendor')) {
    require_once __DIR__ . '/../preload.php';
}

// require rector-src on split packages
if (file_exists(__DIR__ . '/../preload-split-package.php') && is_dir(__DIR__ . '/../../../../vendor')) {
    require_once __DIR__ . '/../preload-split-package.php';
}

Each condition describes a layout, and they read as alternatives. The first is "I have my own vendor/", so
a monorepo checkout. The second is "I sit inside a project's vendor/". Because they are separate if
statements, a checkout that answers yes to both loads both files. Both declare
isPHPStanTestPreloaded(), so that is a fatal error.

Neither file's own return guard can stop it. A top-level function is declared when the file compiles,
before any of its statements run.

Both intended layouts are unaffected, which I checked by installing rector/rector-src as a dependency:

	is_dir(../vendor)	is_dir(../../../../vendor)
vendor/rector/rector-src install	false	true
monorepo checkout with local deps	true	false, normally

The overlap needs a monorepo checkout that has its own vendor/ and sits three levels below another
vendor/. phpstan/phpstan clones this repository into e2e/integration/repo for an integration test, so
four levels above bin/ is phpstan's own vendor/. Every spawned bin/rector there writes nothing, and
tests/Bin/RectorTest fails with empty output. See phpstan/phpstan#15156, which works around it for now by
deleting the file after cloning.

elseif says what the two conditions already mean.

tests/Bin/PreloadTest.php builds the overlapping layout in a temp directory. It uses the real
bin/rector.php and both real preload files, and asserts the spawned run does not report
Cannot redeclare. It fails before the change and passes after. vendor/ is symlinked twice so both preload files find the php-parser
paths they require, otherwise the run stops earlier for an unrelated reason.

Full suite: OK (5273 tests, 6319 assertions). ECS is clean.

[staabm]

Nice. In addition its one less IO operation in the hot path

[TomasVotruba]

TBH we might not need preload file anymore.
We're not patching anything and php-parser with PHPStan has been identical for a while now.

Could you give it a go?

[SanderMuller]

Gave it a go. Inside this repository you are right, and I found one thing that argues the other way for the
released package.

The suite does not need it. I removed both require_once calls, from bin/rector.php and
tests/bootstrap.php, and ran everything: OK (5273 tests, 6319 assertions).

On php-parser being identical. PHPStan's phar still carries unprefixed PhpParser\ classes, so the two
copies do coexist. They are the same version though. I diffed
PhpParser/Node/Stmt/Class_.php between the phar and vendor/nikic/php-parser. The only differences are
the phar build's formatting: declare(strict_types=1) on its own line, expanded class names, brace style. With the preload gone, PhpParser\Node\Stmt\Class_ still resolves to Rector's own copy.

What argues against removing it. build/target-repository/bootstrap.php, the bootstrap that ships,
gives a different reason than php-parser:

They need to be loaded early to avoid conflict version between rector prefixed vendor and Project vendor.
For example, a project may use phpstan/phpdoc-parser v1, while rector uses phpstan/phpdoc-parser uses v2

That is about a consuming project's vendor against the prefixed one, and rector/rector does ship
vendor/phpstan/phpdoc-parser. My run cannot reach that case, because the monorepo has no prefixed vendor
and no third project in the middle. So the suite passing says the tests do not need the preload. It does not
say a user's project does not.

The full surface, in case you want it gone. Three of these I missed on a first pass, so it is wider than
it looks:

• preload.php, preload-split-package.php, build/build-preload.php
• bin/rector.php, tests/bootstrap.php, src/Testing/PHPUnit/AbstractLazyTestCase.php
• build/target-repository/bootstrap.php
• the preload script in composer.json, skips in ecs.php and rector.php
• code_analysis.yaml runs php preload.php as a step, weekly_pull_requests.yaml regenerates it,
  build_scoped_rector.yaml copies it into the scoped build.

If the phpdoc-parser case is obsolete too, say so and I will push the removal here and drop the elseif
along with the test. If you would rather keep the guard for downstream projects, this PR stays a 2 line fix.

[TomasVotruba]

And what's your human take on it?

[SanderMuller]

And what's your human take on it?

The current PR is an easy small fix, stripping it out completely could have more impact than I can oversee.

[TomasVotruba]

Is there any case the preload.php is not there but preload-split-package.php is needed?

[SanderMuller]

No, and the elseif would still cope if there were.

Where each file ships:

	preload.php	preload-split-package.php
rector-src, git	yes	yes
rector/rector-src as a dependency	yes	yes
released rector/rector	yes	no

So the split file never appears on its own. build/build-preload.php writes both, and
build_scoped_rector.yaml copies only preload.php into the scoped build.

The second half matters more, because it does not depend on that table. elseif skips the split branch only
when the first branch actually ran. If preload.php is missing, the first condition is false and the split
branch is still evaluated.

I checked that rather than assume it. In a temp layout with preload-split-package.php and no
preload.php, and a vendor/ four levels above bin/:

files present: preload-split-package.php
[MARKER] split branch taken
Rector @package_version@

The marker is a temporary fwrite inside the branch, so it is the branch running and not just the command
succeeding.

[TomasVotruba]

Sorry, I'll need a human replies, this feels like talking to chatbot 👎