Skip to content

Commit 9636daa

Browse files
CybotTMCybotTM
committed
fix: resolve coding standards issues from Round 7 review
- Remove unused import `array_fill_keys` from DependencyGraph - Add numeric literal separators per SlevomatCodingStandard - Add phpcs:ignore for NodeTraversalTrait naming convention All 119 tests pass.
1 parent 3ab1dad commit 9636daa

File tree

10 files changed

+84
-54
lines changed

10 files changed

+84
-54
lines changed

‎packages/guides/src/Build/IncrementalBuild/DependencyGraph.php‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -13,15 +13,15 @@
1313

1414
namespace phpDocumentor\Guides\Build\IncrementalBuild;
1515

16+
use Generator;
1617
use InvalidArgumentException;
1718
use SplQueue;
1819

19-
use Generator;
20-
21-
use function array_fill_keys;
2220
use function array_keys;
2321
use function array_map;
22+
use function assert;
2423
use function count;
24+
use function get_debug_type;
2525
use function is_array;
2626
use function is_string;
2727
use function max;
@@ -45,7 +45,7 @@
4545
final class DependencyGraph
4646
{
4747
/** Maximum number of documents allowed in the graph to prevent memory exhaustion */
48-
private const MAX_DOCUMENTS = 100000;
48+
private const MAX_DOCUMENTS = 100_000;
4949

5050
/** Maximum number of imports per document to prevent memory exhaustion */
5151
private const MAX_IMPORTS_PER_DOCUMENT = 1000;
@@ -56,7 +56,7 @@ final class DependencyGraph
5656
* This provides a global complexity limit independent of per-document limits.
5757
* With ~64 bytes per edge (key + value overhead), 2M edges ≈ 128-256MB RAM.
5858
*/
59-
private const MAX_TOTAL_EDGES = 2000000;
59+
private const MAX_TOTAL_EDGES = 2_000_000;
6060

6161
/** Current total number of edges in the graph */
6262
private int $edgeCount = 0;
@@ -181,8 +181,8 @@ public function propagateDirty(array $dirtyDocs): array
181181
}
182182

183183
while (!$queue->isEmpty()) {
184-
/** @var string $current */
185184
$current = $queue->dequeue();
185+
assert(is_string($current));
186186

187187
if (isset($visited[$current])) {
188188
continue;
@@ -225,14 +225,15 @@ public function propagateDirtyIterator(array $dirtyDocs): Generator
225225
}
226226

227227
while (!$queue->isEmpty()) {
228-
/** @var string $current */
229228
$current = $queue->dequeue();
229+
assert(is_string($current));
230230

231231
if (isset($visited[$current])) {
232232
continue;
233233
}
234234

235235
$visited[$current] = true;
236+
236237
yield $current;
237238

238239
foreach ($this->getDependents($current) as $dependent) {
@@ -255,14 +256,18 @@ public function removeDocument(string $docPath): void
255256
// The dependents index tells us which documents import this one
256257
$parents = array_keys($this->dependents[$docPath] ?? []);
257258
foreach ($parents as $from) {
258-
if (isset($this->imports[$from][$docPath])) {
259-
unset($this->imports[$from][$docPath]);
260-
$this->edgeCount--;
259+
if (!isset($this->imports[$from][$docPath])) {
260+
continue;
261+
}
261262

262-
if ($this->imports[$from] === []) {
263-
unset($this->imports[$from]);
264-
}
263+
unset($this->imports[$from][$docPath]);
264+
$this->edgeCount--;
265+
266+
if ($this->imports[$from] !== []) {
267+
continue;
265268
}
269+
270+
unset($this->imports[$from]);
266271
}
267272

268273
// 2. Remove edges originating FROM this document
@@ -272,19 +277,23 @@ public function removeDocument(string $docPath): void
272277
// Remove this document from the dependents list of its imports
273278
foreach (array_keys($children) as $to) {
274279
unset($this->dependents[$to][$docPath]);
275-
if (isset($this->dependents[$to]) && $this->dependents[$to] === []) {
276-
unset($this->dependents[$to]);
280+
if (!isset($this->dependents[$to]) || $this->dependents[$to] !== []) {
281+
continue;
277282
}
283+
284+
unset($this->dependents[$to]);
278285
}
279286

280287
// 3. Remove own entries
281288
unset($this->imports[$docPath]);
282289
unset($this->dependents[$docPath]);
283290

284291
// Safety: ensure non-negative edge count
285-
if ($this->edgeCount < 0) {
286-
$this->edgeCount = 0;
292+
if ($this->edgeCount >= 0) {
293+
return;
287294
}
295+
296+
$this->edgeCount = 0;
288297
}
289298

290299
/**
@@ -309,9 +318,11 @@ public function clearImportsFor(string $docPath): void
309318

310319
// Update edge count
311320
$this->edgeCount -= $edgesRemoved;
312-
if ($this->edgeCount < 0) {
313-
$this->edgeCount = 0; // Safety: ensure non-negative
321+
if ($this->edgeCount >= 0) {
322+
return;
314323
}
324+
325+
$this->edgeCount = 0; // Safety: ensure non-negative
315326
}
316327

317328
/**

‎packages/guides/src/Build/IncrementalBuild/DocumentExports.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,9 @@
1616
use InvalidArgumentException;
1717

1818
use function array_keys;
19+
use function array_map;
1920
use function count;
21+
use function get_debug_type;
2022
use function in_array;
2123
use function is_array;
2224
use function is_int;
@@ -44,13 +46,14 @@
4446
final class DocumentExports
4547
{
4648
/** Maximum allowed length for string fields to prevent memory exhaustion attacks */
47-
private const MAX_STRING_LENGTH = 65536;
49+
private const MAX_STRING_LENGTH = 65_536;
4850

4951
/** Maximum allowed number of items in array fields to prevent memory exhaustion */
50-
private const MAX_ARRAY_ITEMS = 10000;
52+
private const MAX_ARRAY_ITEMS = 10_000;
5153

5254
/** Maximum allowed timestamp value (year 3000 in Unix time) to catch corrupted data */
53-
private const MAX_TIMESTAMP = 32503680000;
55+
private const MAX_TIMESTAMP = 32_503_680_000;
56+
5457
/**
5558
* @param string $documentPath Source file path (relative)
5659
* @param string $contentHash Hash of the source file content

‎packages/guides/src/Build/IncrementalBuild/IncrementalBuildState.php‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use InvalidArgumentException;
1717

1818
use function count;
19+
use function get_debug_type;
1920
use function implode;
2021
use function in_array;
2122
use function is_array;
@@ -34,7 +35,7 @@ final class IncrementalBuildState
3435
* Maximum number of exports allowed to prevent memory exhaustion.
3536
* Matches DependencyGraph::MAX_DOCUMENTS for consistency.
3637
*/
37-
private const MAX_EXPORTS = 100000;
38+
private const MAX_EXPORTS = 100_000;
3839

3940
/**
4041
* Allowed hash algorithms for validation.

‎packages/guides/src/Compiler/Passes/DependencyGraphPass.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,9 +86,11 @@ public function run(array $documents, CompilerContextInterface $compilerContext)
8686

8787
// Add edges to the graph, tracking rejections
8888
foreach ($imports as $importedDocPath) {
89-
if (!$graph->addImport($filePath, $importedDocPath)) {
90-
$this->rejectedImports++;
89+
if ($graph->addImport($filePath, $importedDocPath)) {
90+
continue;
9191
}
92+
93+
$this->rejectedImports++;
9294
}
9395
}
9496

‎packages/guides/src/Compiler/Passes/NodeTraversalTrait.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
/**
2222
* Provides recursive node traversal functionality for compiler passes.
2323
*/
24+
// phpcs:ignore SlevomatCodingStandard.Classes.SuperfluousTraitNaming.SuperfluousSuffix
2425
trait NodeTraversalTrait
2526
{
2627
/**

‎packages/guides/tests/unit/Build/IncrementalBuild/ChangeDetectorTest.php‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,11 @@
1818
use function clearstatcache;
1919
use function file_exists;
2020
use function file_put_contents;
21+
use function filemtime;
22+
use function is_dir;
2123
use function is_link;
24+
use function mkdir;
25+
use function rmdir;
2226
use function symlink;
2327
use function sys_get_temp_dir;
2428
use function uniqid;
@@ -50,9 +54,11 @@ protected function tearDown(): void
5054
unlink($file);
5155
}
5256

53-
if (is_dir($this->tempDir)) {
54-
rmdir($this->tempDir);
57+
if (!is_dir($this->tempDir)) {
58+
return;
5559
}
60+
61+
rmdir($this->tempDir);
5662
}
5763

5864
private function createTempFile(string $name, string $content): string
@@ -204,7 +210,7 @@ public function testDetectChangesWithResolverUsesCustomResolver(): void
204210
$result = $this->detector->detectChangesWithResolver(
205211
['doc-path'],
206212
[],
207-
fn (string $docPath) => $filePath, // Custom resolver
213+
static fn (string $docPath) => $filePath, // Custom resolver
208214
);
209215

210216
self::assertSame(['doc-path'], $result->new);
@@ -272,13 +278,13 @@ public function testZeroMtimeForcesHashComputation(): void
272278
public function testResolverReturnsNonExistentPath(): void
273279
{
274280
$cachedExports = [
275-
'doc-path' => $this->createExports('doc-path', 'old-hash', 12345),
281+
'doc-path' => $this->createExports('doc-path', 'old-hash', 12_345),
276282
];
277283

278284
$result = $this->detector->detectChangesWithResolver(
279285
['doc-path'],
280286
$cachedExports,
281-
fn (string $docPath) => '/nonexistent/path/' . $docPath . '.rst',
287+
static fn (string $docPath) => '/nonexistent/path/' . $docPath . '.rst',
282288
);
283289

284290
// Non-existent file should be detected as dirty (mtime=0 differs from cache)
@@ -299,7 +305,7 @@ public function testMixedChangesInSingleRun(): void
299305
$cachedExports = [
300306
$cleanFile => $this->createExports($cleanFile, $cleanHash, $cleanMtime),
301307
$dirtyFile => $this->createExports($dirtyFile, 'old-hash', (int) filemtime($dirtyFile) + 1),
302-
'deleted-file' => $this->createExports('deleted-file', 'hash', 12345),
308+
'deleted-file' => $this->createExports('deleted-file', 'hash', 12_345),
303309
];
304310

305311
$result = $this->detector->detectChanges(

‎packages/guides/tests/unit/Build/IncrementalBuild/DependencyGraphTest.php‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,10 @@
1515

1616
use InvalidArgumentException;
1717
use PHPUnit\Framework\TestCase;
18+
use ReflectionClass;
19+
20+
use function iterator_to_array;
21+
use function sort;
1822

1923
final class DependencyGraphTest extends TestCase
2024
{
@@ -336,13 +340,13 @@ public function testValidateLimitsThrowsOnExcessiveDocuments(): void
336340

337341
// Create a graph that exceeds MAX_DOCUMENTS via fromArray (which allows direct setting)
338342
$imports = [];
339-
for ($i = 0; $i < 100001; $i++) {
343+
for ($i = 0; $i < 100_001; $i++) {
340344
$imports['doc' . $i] = [];
341345
}
342346

343347
// Use reflection to directly set imports to bypass addImport limits
344348
$graph = new DependencyGraph();
345-
$reflection = new \ReflectionClass($graph);
349+
$reflection = new ReflectionClass($graph);
346350
$property = $reflection->getProperty('imports');
347351
$property->setValue($graph, $imports);
348352

@@ -361,7 +365,7 @@ public function testValidateLimitsThrowsOnExcessiveImportsPerDocument(): void
361365
}
362366

363367
$graph = new DependencyGraph();
364-
$reflection = new \ReflectionClass($graph);
368+
$reflection = new ReflectionClass($graph);
365369
$property = $reflection->getProperty('imports');
366370
$property->setValue($graph, ['docA' => $toMap]);
367371

@@ -390,7 +394,7 @@ public function testFromArrayThrowsOnExcessiveDocumentCount(): void
390394

391395
// Create array with more than MAX_DOCUMENTS (100000) imports entries
392396
$imports = [];
393-
for ($i = 0; $i < 100001; $i++) {
397+
for ($i = 0; $i < 100_001; $i++) {
394398
$imports['doc' . $i] = [];
395399
}
396400

@@ -404,12 +408,12 @@ public function testValidateLimitsThrowsOnExcessiveDependentsDocumentCount(): vo
404408

405409
// Create a graph with excessive dependents via reflection
406410
$dependents = [];
407-
for ($i = 0; $i < 100001; $i++) {
411+
for ($i = 0; $i < 100_001; $i++) {
408412
$dependents['doc' . $i] = [];
409413
}
410414

411415
$graph = new DependencyGraph();
412-
$reflection = new \ReflectionClass($graph);
416+
$reflection = new ReflectionClass($graph);
413417
$property = $reflection->getProperty('dependents');
414418
$property->setValue($graph, $dependents);
415419

@@ -428,7 +432,7 @@ public function testValidateLimitsThrowsOnExcessiveDependentsPerDocument(): void
428432
}
429433

430434
$graph = new DependencyGraph();
431-
$reflection = new \ReflectionClass($graph);
435+
$reflection = new ReflectionClass($graph);
432436
$property = $reflection->getProperty('dependents');
433437
$property->setValue($graph, ['docTarget' => $fromMap]);
434438

@@ -443,9 +447,9 @@ public function testValidateLimitsThrowsOnExcessiveTotalEdges(): void
443447
// Create a graph with excessive total edges via reflection
444448
// We can't easily create 2M+ edges, so we'll set edgeCount directly
445449
$graph = new DependencyGraph();
446-
$reflection = new \ReflectionClass($graph);
450+
$reflection = new ReflectionClass($graph);
447451
$edgeProperty = $reflection->getProperty('edgeCount');
448-
$edgeProperty->setValue($graph, 2000001); // Just over MAX_TOTAL_EDGES
452+
$edgeProperty->setValue($graph, 2_000_001); // Just over MAX_TOTAL_EDGES
449453

450454
$graph->validateLimits();
451455
}
@@ -464,6 +468,7 @@ public function testFromArrayThrowsOnExcessiveTotalEdges(): void
464468
for ($target = 0; $target < 1000; $target++) {
465469
$targets[] = 'target_' . $doc . '_' . $target;
466470
}
471+
467472
$imports['doc' . $doc] = $targets;
468473
}
469474

@@ -473,9 +478,9 @@ public function testFromArrayThrowsOnExcessiveTotalEdges(): void
473478
public function testAddImportReturnsFalseWhenTotalEdgeLimitReached(): void
474479
{
475480
$graph = new DependencyGraph();
476-
$reflection = new \ReflectionClass($graph);
481+
$reflection = new ReflectionClass($graph);
477482
$edgeProperty = $reflection->getProperty('edgeCount');
478-
$edgeProperty->setValue($graph, 2000000); // At MAX_TOTAL_EDGES
483+
$edgeProperty->setValue($graph, 2_000_000); // At MAX_TOTAL_EDGES
479484

480485
// Should return false when limit is reached
481486
self::assertFalse($graph->addImport('docNew', 'docTarget'));

0 commit comments

Comments
 (0)