Skip to content

test: use phan major version 6 code analysis#41650

Open
phil-davis wants to merge 2 commits into
masterfrom
phan-6.0.7
Open

test: use phan major version 6 code analysis#41650
phil-davis wants to merge 2 commits into
masterfrom
phan-6.0.7

Conversation

@phil-davis

Copy link
Copy Markdown
Contributor

Now that PHP 7 support has been dropped, we can use the latest release of the phan code analyser.

Patch releases before 6.0.7 had a problem with the function signatures for some Redis methods. That was corrected in phan/phan#5546 and released in phan version 6.0.7. So that version is required as the minimum here.

phan v6 reported:

php -d zend.enable_gc=0 vendor-bin/phan/vendor/bin/phan --config-file .phan/config.php --require-config-exists -p
   analyze ████████████████████████████████████████████████████████████ 100.0% 1017MB/1017MB
lib/private/User/DeletedUser.php:119 PhanUndeclaredMethod Call to undeclared method \OC\Hooks\Emitter::emit
lib/private/User/DeletedUser.php:155 PhanUndeclaredMethod Call to undeclared method \OC\Hooks\Emitter::emit

Actually the object is a Manager not just an Emitter. Manager implements Emitter (through a few layers of inheritance), and Manager has an emit method. I have changed the declaration of $emitter and that makes phan happy. It also looks correct, only a Manager object is ever written to $emitter

Now that PHP 7 support has been dropped, we can use the latest release of the
phan code analyser.

Patch releases before 6.0.7 had a problem with the function signatures for some
Redis methods. That was corrected in phan/phan#5546 and
released in phan version 6.0.7. So that version is required as the minimum here.

#41650
class DeletedUser implements IUser {
/** @var Emitter */
private $emitter;
private Manager $emitter;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a change to source code here, so I made a changelog file.

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review by Claude Code review agent.

LGTM. Clean, well-scoped tooling bump.

Tool bumpvendor-bin/phan/composer.json phan/phan: ^5.5 → ^6.0.7. The ^6.0.7 floor is correctly justified in the description (the Redis signature fix in phan#5546 landed in 6.0.7). No .phan/config.php change needed — target_php_version is null (auto-detect), so v6 picks up the post-PHP-7 baseline without manual edits.

v6 finding handling — the single new finding (PhanUndeclaredMethod on Emitter::emit in DeletedUser.php:119/155) is fixed, not suppressed. Narrowing $emitter from @var Emitter to Manager $emitter is correct: the constructor already accepts Manager $userManager and only ever assigns that, and Manager (same OC\User namespace — no import needed) implements Emitter with a concrete emit(). Good real fix.

CI — all required checks green; phan runs inside the passing "PHP Code Style (8.3)" job (make test-php-phan), which is the key signal that v6 analyses clean.

Changelogchangelog/unreleased/41650 present and well-formed per TEMPLATE (Change: type, present-tense summary, past-tense body, trailing PR URL).

Nit (non-blocking): the use OC\Hooks\Emitter; import on line 25 now appears unused after the type narrowing. phan didn't flag it, so harmless — could be dropped for tidiness.

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review by Claude Code review agent.

Re-review after HEAD changed (f9221fc60fcf8f). The single nit from the previous review — a now-unused use OC\Hooks\Emitter; import left behind by the phan fix — is now resolved: that import has been removed in this revision.

What changed since last review: the leftover use OC\Hooks\Emitter; is dropped, and DeletedUser::$emitter is now declared private Manager $emitter;. Manager lives in the same OC\User namespace (lib/private/User/Manager.php), so the bare type hint resolves without an import. The constructor assigns $this->emitter = $userManager where $userManager is typed Manager, so the narrowed property type is accurate, and ->emit(...) is a real method on Manager — which is exactly what satisfies the PhanUndeclaredMethod finding. Correct fix.

Tooling bump: bumping vendor-bin/phan/composer.json from ^5.5 to ^6.0.7 is correct and well-justified — the >= 6.0.7 floor avoids the pre-6.0.7 Redis signature bug (phan/phan#5546), and PHP 7 support has been dropped so v6 is appropriate.

Changelog: changelog/unreleased/41650 is present and well-formed.

Minor (non-blocking, pre-existing): the if ($this->emitter) guards at lines ~116/152 are now always-true since $emitter is a non-nullable typed property set in the constructor — harmless dead guards, not introduced or worsened by this PR.

Substance is a clean approve. Required checks: all required green; one non-required matrix leg (PHP Unit (8.3, mariadb:10.11)) was still pending at review time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants