Proxy copies default values of inherited methods as written: class names resolve against the child file's imports

Aperta
  • Qualcosa non va?
#836 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
1-2 giorni
Idoneità per principianti
56/100
Tipo di issue
Bug
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
php
Ambito
backend

Direzione di ricerca

Read src/Proxy/Generator/ParameterGenerator.php and AbstractInterceptedPropertyGenerator.php, then inspect AttributeGroupsGenerator::$argumentCloner and ResolvedNameQualifyingVisitor for the existing name-qualification pattern. Add coverage for inherited defaults from another file, including colliding imports, self::CONST, and promoted or intercepted properties; run the listed composer checks with opcache.jit=disable. Done means the parent defaults are preserved and existing proxy snapshots remain unchanged.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

Bug

Found during #826 (the fix went out with that declined change, see PR #831). Reproduced on master dc64ef7.

Problem

When a woven class gets an intercepted method from a parent class (or a trait) declared in another file, the proxy re-declares that method. ParameterGenerator builds the parameter type from the resolved AST names, which is correct and fully qualified. The default value, however, is copied as written (ValueGenerator::fromExprNode($astParam->default), src/Proxy/Generator/ParameterGenerator.php).

The proxy lives in the child's file and carries the child's use imports. A short class name in an inherited default (Level::LEVEL, Foo::class, new Foo()) therefore resolves against the child's imports, not the parent's.

Reproduction

// ImportedDefaultsBase.php
namespace Go\Instrument\Transformer\Stubs;
use Go\Stubs\Collision\The as Level;
class ImportedDefaultsBase
{
    public function log(string $level = Level::LEVEL, ?Level $target = null): string { return $level; }
}

// ImportedDefaultsChild.php
namespace Go\Instrument\Transformer\Stubs;
use Go\Stubs\Collision\Interceptor as Level;   // same short name, another class
class ImportedDefaultsChild extends ImportedDefaultsBase { /* … */ }

With a before advice on ImportedDefaultsChild->log, the generated proxy of the child contains:

use Go\Stubs\Collision\Interceptor as Level;
…
public function log(string $level = Level::LEVEL, ?\Go\Stubs\Collision\The $target = null): string

The type is right (\Go\Stubs\Collision\The), but the default now means Go\Stubs\Collision\Interceptor::LEVEL. Both stubs happen to define LEVEL = 'debug', which hides it in the test fixtures.

In real code, depending on the classes involved, the intercepted method then:

  • silently gets a different default (another class's constant);
  • fails on the call with "Undefined constant";
  • fails with "Class not found" when the child file has no such import.

All of this happens only when the argument is omitted, so the bug is easy to miss.

Scope

  • Affected: default values of parameters, and of promoted and intercepted properties (AbstractInterceptedPropertyGenerator copies the AST default the same way), declared in a parent class or trait from another file.
  • Not affected: types, which are already qualified from resolvedName, and attribute arguments, which AttributeGroupsGenerator already clones and qualifies with ResolvedNameQualifyingVisitor.
  • Unaffected case: members declared in the woven file itself, because the proxy shares that file's imports.
  • Related edge case: self::CONST in an inherited default refers to the declaring class in PHP. In the re-declared proxy method, self is the proxy (child) class, which matters when the child overrides the constant.

Proposed decision for the implementing agent

Keep it small and in line with how master already treats types and attribute arguments. Do not reintroduce the declined #826 alias machinery.

  1. When copying a default AST node (parameters, promoted properties, intercepted properties), clone it and qualify its names with the existing ResolvedNameQualifyingVisitor (same traverser pattern as AttributeGroupsGenerator::$argumentCloner). Do this only when the member's declaring file differs from the file being woven. Compare the declaring class's getFileName() with the woven file, which also covers trait members. Proxies of a class's own members stay byte-identical, so no existing snapshot changes.
  2. In the same foreign-file case, replace self in the default with the declaring class name (rooted). This preserves PHP's meaning of an inherited self::CONST. static can't appear in a constant expression, and parent should resolve to the declaring class's parent.
  3. Make sure the qualifying visitor does not turn self/parent into \self/\parent. A fix for that was part of the reverted #826; re-apply it if needed.

Acceptance

  • A new stub pair (the reproduction above, with different constant values in the two colliding classes). The child's proxy emits \Go\Stubs\Collision\The::LEVEL, and a functional or unit test calling log() without arguments gets the parent's value.
  • A case with self::CONST in an inherited default, with the child overriding the constant: the parent's value is kept.
  • Promoted/intercepted property defaults from a parent in another file are covered.
  • Existing proxy snapshots under tests/Instrument/Transformer/_files are unchanged (own-file members are untouched).
  • composer test, composer analyze, composer cs, and composer test:performance:transformation (generator change). Run with opcache.jit=disable: the PHP 8.5.12 tracing-JIT bug breaks the functional subprocess tests.

Scoring

Score Why
Effort Low One reused visitor on the default-copy path of the parameter and property generators, a declaring-file check, plus stubs and tests.
Impact Medium It needs an inherited, intercepted method with a class-referencing default and a colliding short name across files, so it is rare. When it hits, it is a silent wrong value or a call-time fatal.
Lingua principale
PHP
Stelle
1.7k
Fork
166
Merge medio
1g 1h
PR unite (30g)
43

Preparare l'ambiente

Questo progetto non fornisce container di sviluppo, Dockerfile né guida per i contributori, quindi l'ambiente è a tuo carico: parti dal suo README e consulta la nostra guida al primo contributo per i passaggi generali.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di goaop/framework

Tutte le issue di goaop/framework

Issue simili

Altre issue su PHP

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.