Skip to content

Commit 3a1af9f

Browse files
Bugfix: Autoloaders require source, not the file
1 parent b9125ee commit 3a1af9f

17 files changed

Lines changed: 378 additions & 185 deletions

‎.github/workflows/ci.yml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,11 @@ jobs:
5656
- name: Static code analysis
5757
run: composer analyze
5858

59+
- name: Autoloader smoke test
60+
env:
61+
REQUIRE_TURBO: '1'
62+
run: composer smoke-test
63+
5964
- name: Unittests
6065
if: matrix.coveralls == false
6166
run: composer test

‎CHANGELOG.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,24 @@
22

33
All notable changes to this project will be documented in this file, in reverse chronological order by release.
44

5+
## Unreleased
6+
7+
### Added
8+
9+
- A CI smoke test (`composer smoke-test`) that runs a real analysis over a generated fixture project and fails when the generated-class autoloaders report an internal error.
10+
11+
### Deprecated
12+
13+
- Nothing.
14+
15+
### Removed
16+
17+
- `bitExpert\PHPStan\Magento\Autoload\Cache\FileCacheStorage`, replaced by `bitExpert\PHPStan\Magento\Autoload\Cache\GeneratedFileCache`. The autoloaders now take that class in place of `PHPStan\Cache\Cache`, and the generated classes move from `%tmpDir%/cache/PHPStan` to `%tmpDir%/cache/bitExpert/PHPStanMagento` so that they no longer share a directory with PHPStan's own cache.
18+
19+
### Fixed
20+
21+
- Fix "Internal error: Failed opening required '<?php ...'" for every generated Factory, Proxy, Extension and ExtensionInterface class on PHPStan 2.2.10 and PHP 8.3 or later. `FileCacheStorage` stored the generated source but returned a file path from `load()`, which a `PHPStan\Cache\CacheStorage` must not do. Once PHPStan served cache entries from its shared memory arena, `Cache::load()` returned the source code and the autoloaders required that in place of the file.
22+
523
## 0.43.0
624

725
### Added

‎bin/ci_autoloader_smoke‎

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
#!/usr/bin/env php
2+
<?php
3+
4+
/*
5+
* This file is part of the phpstan-magento package.
6+
*
7+
* (c) bitExpert AG
8+
*
9+
* For the full copyright and license information, please view the LICENSE
10+
* file that was distributed with this source code.
11+
*/
12+
declare(strict_types=1);
13+
14+
/*
15+
* Runs a real PHPStan analysis against a generated fixture project and fails when the autoloaders
16+
* report an internal error.
17+
*
18+
* The fixture only asks for factories, which is enough: all four generated-class autoloaders
19+
* share the store-then-require mechanism this guards.
20+
*
21+
* The autoloaders generate a class, store it and then require the stored file. Whenever PHPStan
22+
* changes how it hands cached values back, that round trip can start to return the source code in
23+
* place of the path, which turns every generated class into
24+
* "Internal error: Failed opening required '<?php ...'". Unit tests cannot see this: it only shows
25+
* with a cold cache, more than one analysis process, and PHPStan's turbo extension active.
26+
*
27+
* Set REQUIRE_TURBO=1 (the CI job does) to also fail when PHPStan's shared memory arena is not
28+
* usable, so that the guard cannot pass without exercising the code path it protects.
29+
*/
30+
31+
$repoRoot = dirname(__DIR__);
32+
$fixtureRoot = sys_get_temp_dir() . '/phpstan-magento-smoke-' . uniqid();
33+
$numberOfFiles = 100;
34+
35+
function removeDirectory(string $directory): void
36+
{
37+
if (!is_dir($directory)) {
38+
return;
39+
}
40+
41+
$entries = new RecursiveIteratorIterator(
42+
new RecursiveDirectoryIterator($directory, FilesystemIterator::SKIP_DOTS),
43+
RecursiveIteratorIterator::CHILD_FIRST
44+
);
45+
46+
foreach ($entries as $entry) {
47+
/** @var SplFileInfo $entry */
48+
$entry->isDir() ? rmdir($entry->getPathname()) : unlink($entry->getPathname());
49+
}
50+
51+
rmdir($directory);
52+
}
53+
54+
function run(string $repoRoot, string $command, string $config): array
55+
{
56+
$commandLine = sprintf(
57+
'%s %s %s --configuration=%s 2>&1',
58+
escapeshellarg(PHP_BINARY),
59+
escapeshellarg($repoRoot . '/vendor/bin/phpstan'),
60+
$command,
61+
escapeshellarg($config)
62+
);
63+
64+
$output = [];
65+
$exitCode = 0;
66+
exec($commandLine, $output, $exitCode);
67+
68+
return [implode(PHP_EOL, $output), $exitCode];
69+
}
70+
71+
/*
72+
* The arena is what makes the cache round trip observable, and the extension being loaded does not
73+
* mean the arena works: on macOS the extension loads but never hands out an arena. Probe it the way
74+
* PHPStan does, with the binary PHPStan itself reports.
75+
*/
76+
function arenaIsUsable(string $diagnoseOutput): bool
77+
{
78+
if (preg_match('#^Turbo worker binary: (\S+)#m', $diagnoseOutput, $matches) !== 1) {
79+
return false;
80+
}
81+
82+
$probe = sprintf(
83+
'%s -d %s -r %s 2>&1',
84+
escapeshellarg(PHP_BINARY),
85+
escapeshellarg('extension=' . $matches[1]),
86+
escapeshellarg('echo \PHPStanTurbo\ArenaCache::create("probe") === null ? "no" : "yes";')
87+
);
88+
89+
$output = [];
90+
$exitCode = 0;
91+
exec($probe, $output, $exitCode);
92+
93+
return $exitCode === 0 && in_array('yes', $output, true);
94+
}
95+
96+
mkdir($fixtureRoot . '/src', 0777, true);
97+
98+
for ($i = 1; $i <= $numberOfFiles; $i++) {
99+
file_put_contents(
100+
sprintf('%s/src/Consumer%d.php', $fixtureRoot, $i),
101+
<<<PHP
102+
<?php declare(strict_types=1);
103+
104+
namespace Smoke;
105+
106+
class Consumer{$i}
107+
{
108+
public function useFactory(\\Smoke\\Thing{$i}Factory \$factory): void
109+
{
110+
\$factory->create();
111+
}
112+
}
113+
114+
PHP
115+
);
116+
}
117+
118+
// A cold tmpDir matters: with a warm cache the autoloaders find the generated file before they
119+
// ever ask PHPStan to store anything, which hides the very problem this guard looks for.
120+
file_put_contents($fixtureRoot . '/phpstan.neon', <<<NEON
121+
includes:
122+
- {$repoRoot}/extension.neon
123+
124+
parameters:
125+
tmpDir: {$fixtureRoot}/tmp
126+
level: 2
127+
paths:
128+
- {$fixtureRoot}/src
129+
magento:
130+
magentoRoot: {$fixtureRoot}
131+
parallel:
132+
jobSize: 5
133+
minimumNumberOfJobsPerProcess: 1
134+
maximumNumberOfProcesses: 4
135+
136+
NEON);
137+
138+
[$diagnoseOutput] = run($repoRoot, 'diagnose', $fixtureRoot . '/phpstan.neon');
139+
$arenaIsUsable = strpos($diagnoseOutput, 'Turbo extension: enabled (') !== false
140+
&& arenaIsUsable($diagnoseOutput);
141+
142+
echo 'PHPStan shared memory arena usable: ' . ($arenaIsUsable ? 'yes' : 'no') . PHP_EOL;
143+
144+
if (!$arenaIsUsable && getenv('REQUIRE_TURBO') === '1') {
145+
fwrite(STDERR, PHP_EOL . 'The shared memory arena is not usable, thus this guard would pass' . PHP_EOL);
146+
fwrite(STDERR, 'without exercising the code path it protects. PHPStan reported:' . PHP_EOL . PHP_EOL);
147+
fwrite(STDERR, $diagnoseOutput . PHP_EOL);
148+
removeDirectory($fixtureRoot);
149+
exit(1);
150+
}
151+
152+
[$analyseOutput, $exitCode] = run($repoRoot, 'analyse --no-progress', $fixtureRoot . '/phpstan.neon');
153+
removeDirectory($fixtureRoot);
154+
155+
if (strpos($analyseOutput, 'Internal error') !== false || $exitCode !== 0) {
156+
fwrite(STDERR, PHP_EOL . 'The generated-class autoloaders did not survive a real analysis run:' . PHP_EOL . PHP_EOL);
157+
fwrite(STDERR, $analyseOutput . PHP_EOL);
158+
exit(1);
159+
}
160+
161+
echo 'Autoloader smoke test passed.' . PHP_EOL;

‎composer.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@
6363
"cs-fix": "vendor/bin/phpcbf",
6464
"check-license": "vendor/bin/license-checker check",
6565
"analyze": "vendor/bin/phpstan analyze",
66+
"smoke-test": "@php bin/ci_autoloader_smoke",
6667
"test": "vendor/bin/phpunit",
6768
"coverage": "vendor/bin/phpunit --coverage-clover clover.xml"
6869
},

‎extension.neon‎

Lines changed: 7 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -61,16 +61,11 @@ services:
6161
class: bitExpert\PHPStan\Magento\Rules\ResourceModelsShouldBeUsedDirectlyRule
6262
-
6363
class: bitExpert\PHPStan\Magento\Rules\SetTemplateDisallowedForBlockRule
64-
fileCacheStorage:
65-
class: bitExpert\PHPStan\Magento\Autoload\Cache\FileCacheStorage
64+
generatedFileCache:
65+
class: bitExpert\PHPStan\Magento\Autoload\Cache\GeneratedFileCache
6666
arguments:
67-
directory: %tmpDir%/cache/PHPStan
67+
directory: %tmpDir%/cache/bitExpert/PHPStanMagento
6868
magentoRoot: %magento.magentoRoot%
69-
autoloaderCache:
70-
autowired: false
71-
class: PHPStan\Cache\Cache
72-
arguments:
73-
storage: @fileCacheStorage
7469
extensionAttributeDataProvider:
7570
class: bitExpert\PHPStan\Magento\Autoload\DataProvider\ExtensionAttributeDataProvider
7671
arguments:
@@ -92,29 +87,29 @@ services:
9287
factoryAutoloader:
9388
class: bitExpert\PHPStan\Magento\Autoload\FactoryAutoloader
9489
arguments:
95-
cache: @autoloaderCache
90+
cache: @generatedFileCache
9691
classLoaderProvider: @classLoaderProvider
9792
tags:
9893
- phpstan.magento.autoloader
9994
proxyAutoloader:
10095
class: bitExpert\PHPStan\Magento\Autoload\ProxyAutoloader
10196
arguments:
102-
cache: @autoloaderCache
97+
cache: @generatedFileCache
10398
classLoaderProvider: @classLoaderProvider
10499
tags:
105100
- phpstan.magento.autoloader
106101
extensionInterfaceAutoloader:
107102
class: bitExpert\PHPStan\Magento\Autoload\ExtensionInterfaceAutoloader
108103
arguments:
109-
cache: @autoloaderCache
104+
cache: @generatedFileCache
110105
classLoaderProvider: @classLoaderProvider
111106
attributeDataProvider: @extensionAttributeDataProvider
112107
tags:
113108
- phpstan.magento.autoloader
114109
extensionAutoloader:
115110
class: bitExpert\PHPStan\Magento\Autoload\ExtensionAutoloader
116111
arguments:
117-
cache: @autoloaderCache
112+
cache: @generatedFileCache
118113
classLoaderProvider: @classLoaderProvider
119114
attributeDataProvider: @extensionAttributeDataProvider
120115
tags:

‎phpstan.neon‎

Lines changed: 0 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -7,21 +7,6 @@ parameters:
77
- src/bitExpert
88
- tests/bitExpert
99
ignoreErrors:
10-
-
11-
message: '~is not covered by backward compatibility promise.~'
12-
path: src/bitExpert/PHPStan/Magento/Autoload/Cache/FileCacheStorage.php
13-
-
14-
message: '~is not covered by backward compatibility promise.~'
15-
path: src/bitExpert/PHPStan/Magento/Autoload/FactoryAutoloader.php
16-
-
17-
message: '~is not covered by backward compatibility promise.~'
18-
path: src/bitExpert/PHPStan/Magento/Autoload/ProxyAutoloader.php
19-
-
20-
message: '~is not covered by backward compatibility promise.~'
21-
path: src/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloader.php
22-
-
23-
message: '~is not covered by backward compatibility promise.~'
24-
path: src/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloader.php
2510
-
2611
message: '~is not covered by backward compatibility promise.~'
2712
path: src/bitExpert/PHPStan/Magento/Reflection/Framework/Session/SessionManagerMagicMethodReflectionExtension.php
@@ -37,24 +22,9 @@ parameters:
3722
-
3823
message: '~Function is_subclass_of\(\) is a runtime reflection concept that might not work in PHPStan~'
3924
path: 'src/bitExpert/PHPStan/Magento/Type/TestFrameworkObjectManagerDynamicReturnTypeExtension.php'
40-
-
41-
message: '~is not covered by backward compatibility promise.~'
42-
path: tests/bitExpert/PHPStan/Magento/Autoload/FactoryAutoloaderUnitTest.php
43-
-
44-
message: '~is not covered by backward compatibility promise.~'
45-
path: tests/bitExpert/PHPStan/Magento/Autoload/ProxyAutoloaderUnitTest.php
46-
-
47-
message: '~is not covered by backward compatibility promise.~'
48-
path: tests/bitExpert/PHPStan/Magento/Autoload/RegistrationUnitTest.php
49-
-
50-
message: '~is not covered by backward compatibility promise.~'
51-
path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloaderUnitTest.php
5225
-
5326
message: '~Parameter #1 \$argument of class ReflectionClass constructor expects class-string<MyUncachedExtension>|MyUncachedExtension, string given~'
5427
path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionAutoloaderUnitTest.php
55-
-
56-
message: '~is not covered by backward compatibility promise.~'
57-
path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloaderUnitTest.php
5828
-
5929
message: '~Parameter #1 \$argument of class ReflectionClass constructor expects class-string<UncachedExtensionInterface>|UncachedExtensionInterface, string given~'
6030
path: tests/bitExpert/PHPStan/Magento/Autoload/ExtensionInterfaceAutoloaderUnitTest.php

src/bitExpert/PHPStan/Magento/Autoload/Cache/FileCacheStorage.php renamed to src/bitExpert/PHPStan/Magento/Autoload/Cache/GeneratedFileCache.php

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,15 @@
1212

1313
namespace bitExpert\PHPStan\Magento\Autoload\Cache;
1414

15-
use PHPStan\Cache\CacheStorage;
16-
17-
class FileCacheStorage implements CacheStorage
15+
/**
16+
* Stores the source code the autoloaders generate and hands back the path of the file holding it.
17+
*
18+
* This deliberately is not a PHPStan\Cache\CacheStorage: the autoloaders need the path of the
19+
* generated file to require it, while a CacheStorage has to return what was handed to save().
20+
* Mixing the two made the autoloaders require the source code itself once PHPStan started to
21+
* serve cache entries from its shared memory arena.
22+
*/
23+
class GeneratedFileCache
1824
{
1925
/**
2026
* @var string
@@ -27,9 +33,10 @@ class FileCacheStorage implements CacheStorage
2733
private $magentoRoot;
2834

2935
/**
30-
* FileCacheStorage constructor.
36+
* GeneratedFileCache constructor.
3137
*
3238
* @param string $directory
39+
* @param string $magentoRoot
3340
*/
3441
public function __construct(string $directory, string $magentoRoot)
3542
{
@@ -38,39 +45,41 @@ public function __construct(string $directory, string $magentoRoot)
3845
}
3946

4047
/**
48+
* Returns the path of the file generated for the given key, null when nothing was generated yet.
49+
*
4150
* @param string $key
42-
* @param string $variableKey
43-
* @return mixed
51+
* @return string|null
4452
*/
45-
public function load(string $key, string $variableKey)
53+
public function getFile(string $key): ?string
4654
{
47-
return (function (string $key): ?string {
48-
$cacheDir = $this->getCacheDir($key);
49-
$cacheFile = $this->getCacheFile($key);
50-
if (!is_file($cacheDir . '/' . $cacheFile)) {
51-
return null;
52-
}
55+
$cacheFile = $this->getCacheDir($key) . '/' . $this->getCacheFile($key);
56+
if (!is_file($cacheFile)) {
57+
return null;
58+
}
5359

54-
return $cacheDir . '/' . $cacheFile;
55-
})($key);
60+
return $cacheFile;
5661
}
5762

5863
/**
64+
* Writes the given source code and returns the path of the file holding it.
65+
*
5966
* @param string $key
60-
* @param string $variableKey
61-
* @param mixed $data
67+
* @param string $contents
68+
* @return string
6269
*/
63-
public function save(string $key, string $variableKey, $data): void
70+
public function putFile(string $key, string $contents): string
6471
{
6572
$cacheDir = $this->getCacheDir($key);
66-
$cacheFile = $this->getCacheFile($key);
73+
$cacheFile = $cacheDir . '/' . $this->getCacheFile($key);
6774
$this->makeDir($cacheDir);
68-
$tmpSuccess = @file_put_contents($cacheDir . '/' . $cacheFile, $data);
69-
if ($tmpSuccess === false) {
75+
$written = @file_put_contents($cacheFile, $contents);
76+
if ($written === false) {
7077
throw new \InvalidArgumentException(
71-
sprintf('Could not write data to cache file %s.', $cacheDir . '/' . $cacheFile)
78+
sprintf('Could not write data to cache file %s.', $cacheFile)
7279
);
7380
}
81+
82+
return $cacheFile;
7483
}
7584

7685
/**

0 commit comments

Comments
 (0)