Skip to content

Commit cb1648c

Browse files
dereuromarkdereuromark
authored
Let --remove prune parent-superseded method overrides even when self-called (#447)
The entity-template gate stopped emitting the CRUD/find-family method overrides once the Cake ORM Table generic covers them, but did nothing about overrides already committed by an older config/version. Running "annotate models --remove" did not clean them either: methodInUse() flags any method-override the class calls on $this and excludes it from removal. That keep-heuristic is correct when the override is the only type source. It is wrong once the parent generic supersedes the override: the stale override's only remaining effect is stripping the parent throws tag, reintroducing the false catch.neverThrown. Tables whose domain method self-calls saveOrFail()/save()/newEntity() kept the regression even after upgrade plus --remove. ModelAnnotator now publishes the superseded method names via AbstractAnnotator::CONFIG_SUPERSEDED_METHODS; the in-use guard skips them so --remove can prune them. The list is gated exactly like emission, so narrowing overrides for detailed/concrete are still regenerated and matched before the removal pass. Empty for non-model annotators, no behaviour change elsewhere. Also corrects EntityTask::getEntityFields() return docblock to the type it actually returns (a map of entity class to a map of field name to StringName); the previous array<array<string>> was simply wrong and newer phpstan deps in CI rightly reject it.
1 parent ea6ca7d commit cb1648c

4 files changed

Lines changed: 154 additions & 5 deletions

File tree

‎src/Annotator/AbstractAnnotator.php‎

Lines changed: 34 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,16 @@ abstract class AbstractAnnotator {
7171
*/
7272
public const CONFIG_REMOVE = 'remove';
7373

74+
/**
75+
* Config: method names a parent generic already covers. For these the
76+
* methodInUse() keep-heuristic is skipped so --remove can prune the
77+
* redundant override even when the class self-calls the method.
78+
* Populated by ModelAnnotator; empty elsewhere.
79+
*
80+
* @var string
81+
*/
82+
public const CONFIG_SUPERSEDED_METHODS = 'supersededMethods';
83+
7484
/**
7585
* @var string
7686
*/
@@ -557,7 +567,12 @@ protected function parseExistingAnnotations(File $file, int $closeTagIndex, arra
557567
if ($this->getConfig(static::CONFIG_REMOVE) && $tag === PropertyReadAnnotation::TAG && $this->propertyInUse($tokens, $closeTagIndex, $content)) {
558568
$annotation->setInUse();
559569
}
560-
if ($this->getConfig(static::CONFIG_REMOVE) && $tag === MethodAnnotation::TAG && $this->methodInUse($tokens, $closeTagIndex, $content)) {
570+
if (
571+
$this->getConfig(static::CONFIG_REMOVE)
572+
&& $tag === MethodAnnotation::TAG
573+
&& !$this->methodSupersededByParent($content)
574+
&& $this->methodInUse($tokens, $closeTagIndex, $content)
575+
) {
561576
$annotation->setInUse();
562577
}
563578

@@ -713,6 +728,24 @@ protected function methodInUse(array $tokens, int $closeTagIndex, string $method
713728
return false;
714729
}
715730

731+
/**
732+
* Whether the method is one a parent generic already covers (see
733+
* {@see static::CONFIG_SUPERSEDED_METHODS}), making its override
734+
* redundant and removable regardless of self-calls.
735+
*
736+
* @param string $method The method annotation content, e.g. `saveOrFail(... $entity, array $options = [])`.
737+
*
738+
* @return bool
739+
*/
740+
protected function methodSupersededByParent(string $method): bool {
741+
$superseded = $this->getConfig(static::CONFIG_SUPERSEDED_METHODS);
742+
if (!$superseded || !preg_match('#^(\w+)\(#', $method, $matches)) {
743+
return false;
744+
}
745+
746+
return in_array($matches[1], (array)$superseded, true);
747+
}
748+
716749
/**
717750
* @param \PHP_CodeSniffer\Files\File $file
718751
* @param int $lastTagIndexOfPreviousLine

‎src/Annotator/ModelAnnotator.php‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,23 @@ protected function buildAnnotations(array $associations, string $entity, array $
259259
&& ($parentClass === '' || ltrim($parentClass, '\\') === 'Cake\\ORM\\Table');
260260
$entityTemplateFindFamily = $entityTemplateEmitted && $this->supportsEntityTemplateFindFamily();
261261

262+
// Methods the parent Table generic now fully covers (return type
263+
// + throws). Publishing them lets --remove prune a stale override
264+
// even when the table self-calls the method. Narrowing overrides
265+
// for detailed/concrete are still emitted and matched before the
266+
// removal pass, so this only ever affects orphaned leftovers.
267+
$supersededMethods = [];
268+
if ($entityTemplateEmitted) {
269+
$supersededMethods = [
270+
'newEmptyEntity', 'newEntity', 'newEntities', 'get',
271+
'patchEntity', 'patchEntities', 'save', 'saveOrFail',
272+
];
273+
}
274+
if ($entityTemplateFindFamily) {
275+
$supersededMethods = array_merge($supersededMethods, ['find', 'findOrCreate', 'loadInto']);
276+
}
277+
$this->setConfig(static::CONFIG_SUPERSEDED_METHODS, $supersededMethods);
278+
262279
if (!$entityTemplateEmitted) {
263280
$annotations[] = "@method {$fullClassName} newEmptyEntity()";
264281
}
@@ -601,9 +618,11 @@ protected function supportsEntityTemplate(): bool {
601618
* This is a strictly later change than {@see static::supportsEntityTemplate()} —
602619
* the class-level template was added separately from the find-family annotations.
603620
*
604-
* Requires cakephp/cakephp#19438, currently milestoned for CakePHP 5.3.6. The
605-
* version gate below MUST be set to the actual release containing that change
606-
* before this is mark-able as non-draft.
621+
* Requires cakephp/cakephp#19438 (find/findOrCreate/loadInto propagation),
622+
* #19439 (SelectQuery::first/firstOrFail typed via TSubject) and #19440
623+
* (Table::find() narrowed to SelectQuery<TEntity>). All three are merged
624+
* to cakephp 5.x under milestone 5.3.6, so the 5.3.6 gate below is the
625+
* confirmed first release that carries the complete find-family typing.
607626
*
608627
* @return bool
609628
*/

‎src/Generator/Task/EntityTask.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ public function collect(): array {
5353
}
5454

5555
/**
56-
* @return array<array<string>>
56+
* @return array<string, array<string, \IdeHelper\ValueObject\StringName>>
5757
*/
5858
protected function getEntityFields(): array {
5959
$modelFields = [];

‎tests/TestCase/Annotator/AbstractAnnotatorTest.php‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
namespace IdeHelper\Test\TestCase\Annotator;
5+
6+
use Cake\Console\ConsoleIo;
7+
use Cake\TestSuite\TestCase;
8+
use IdeHelper\Annotator\AbstractAnnotator;
9+
use IdeHelper\Annotator\ModelAnnotator;
10+
use IdeHelper\Console\Io;
11+
use ReflectionMethod;
12+
use Shim\TestSuite\ConsoleOutput;
13+
14+
class AbstractAnnotatorTest extends TestCase {
15+
16+
/**
17+
* @param array<string, mixed> $params
18+
* @return \IdeHelper\Annotator\ModelAnnotator
19+
*/
20+
protected function annotator(array $params): ModelAnnotator {
21+
$io = new Io(new ConsoleIo(new ConsoleOutput(), new ConsoleOutput()));
22+
23+
return new ModelAnnotator($io, $params);
24+
}
25+
26+
/**
27+
* @param \IdeHelper\Annotator\ModelAnnotator $annotator
28+
* @param string $content
29+
* @return bool
30+
*/
31+
protected function isSuperseded(ModelAnnotator $annotator, string $content): bool {
32+
$method = new ReflectionMethod($annotator, 'methodSupersededByParent');
33+
$method->setAccessible(true);
34+
35+
return $method->invoke($annotator, $content);
36+
}
37+
38+
/**
39+
* No list configured: nothing is superseded (non-model annotators).
40+
*
41+
* @return void
42+
*/
43+
public function testFalseWhenNoSupersededListConfigured() {
44+
$annotator = $this->annotator([]);
45+
46+
$this->assertFalse(
47+
$this->isSuperseded($annotator, 'saveOrFail(\App\Model\Entity\Foo $entity, array $options = [])'),
48+
);
49+
}
50+
51+
/**
52+
* A name in the list is superseded (in-use guard must not protect it).
53+
*
54+
* @return void
55+
*/
56+
public function testTrueForNameInSupersededList() {
57+
$annotator = $this->annotator([
58+
AbstractAnnotator::CONFIG_SUPERSEDED_METHODS => ['save', 'saveOrFail', 'patchEntity'],
59+
]);
60+
61+
$this->assertTrue(
62+
$this->isSuperseded($annotator, 'saveOrFail(\App\Model\Entity\Foo $entity, array $options = [])'),
63+
);
64+
$this->assertTrue(
65+
$this->isSuperseded($annotator, 'patchEntity(\App\Model\Entity\Foo $entity, array $data, array $options = [])'),
66+
);
67+
}
68+
69+
/**
70+
* Always-emitted batch variants stay protected (not in the list).
71+
*
72+
* @return void
73+
*/
74+
public function testFalseForNameNotInSupersededList() {
75+
$annotator = $this->annotator([
76+
AbstractAnnotator::CONFIG_SUPERSEDED_METHODS => ['save', 'saveOrFail'],
77+
]);
78+
79+
$this->assertFalse(
80+
$this->isSuperseded($annotator, 'saveManyOrFail(iterable $entities, array $options = [])'),
81+
);
82+
}
83+
84+
/**
85+
* Unparseable content never matches.
86+
*
87+
* @return void
88+
*/
89+
public function testFalseForUnparseableContent() {
90+
$annotator = $this->annotator([
91+
AbstractAnnotator::CONFIG_SUPERSEDED_METHODS => ['save'],
92+
]);
93+
94+
$this->assertFalse($this->isSuperseded($annotator, 'not a signature'));
95+
}
96+
97+
}

0 commit comments

Comments
 (0)