Skip to content

[VarExporter] Fix calling scope detection inside magic accessors #51071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jul 26, 2023
Merged

[VarExporter] Fix calling scope detection inside magic accessors #51071

merged 1 commit into from
Jul 26, 2023

Conversation

vtsykun
Copy link
Contributor

@vtsykun vtsykun commented Jul 22, 2023 •
edited by nicolas-grekas
Loading

Q A
Branch? 6.2
Bug fix? yes
New feature? no
Deprecations? -
Tickets Fix #51048
License MIT
Doc PR -

This PR provides fixes related to detection of class scope for which the magic method was called.
this fixes are related to the issue described in this RFC https://wiki.php.net/rfc/access_scope_from_magic_accessors

more accurate STR for the bug:

class A1Class {
    private $prop1;
    public function __construct($prop1)
    {
        $this->prop1 = $prop1;
    }

    public function getProp1()
    {
        return $this->prop1;
    }
}

class B1Class extends A1Class
{
    protected $prop1;
    protected $prop2;

    public function __construct($prop1)
    {
        parent::__construct($prop1);
        $this->prop1 = $prop1;
    }

    public function test()
    {
        return $this->prop1;
    }

    public function test2()
    {
        return $this->prop2;
    }

    public function setProp2($prop2)
    {
        $this->prop2 = $prop2;
    }
}
    App\B1Class:
        arguments: [ 'test1' ]
        calls:
            - [ setProp2, [ 'test2' ] ]
        lazy: true

Call $this->b1Class->test2();

Actual result:
Selection_1645

@carsonbot carsonbot added this to the 6.3 milestone Jul 22, 2023
@vtsykun vtsykun force-pushed the fix/detect-scope branch from 2e302d6 to b6ee8a6 Compare July 22, 2023 18:53
@vtsykun
Copy link
Contributor Author

vtsykun commented Jul 22, 2023

I found another type of error related to this bug

class FinalPublicClass
{
    private $count = 0;

    final public function increment(): int
    {
        return $this->count += 1;
    }

    public function decrement(): int
    {
        return $this->count -= 1;
    }
}

class TestOverwritePropClass extends FinalPublicClass
{
    public function __construct(
        protected string $dep,
        protected int $count
    ) {
    }

    public function getDep(): string
    {
        return $this->dep;
    }
}
TestOverwritePropClass:
    lazy: true
    arguments: ['123', 5]

$class->increment() // return 1,
$proxy->increment() // return 6, but must be 1

Selection_1646

@vtsykun vtsykun force-pushed the fix/detect-scope branch from b6ee8a6 to e20479e Compare July 25, 2023 03:40
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.2 Jul 26, 2023
@nicolas-grekas nicolas-grekas changed the base branch from 6.3 to 6.2 July 26, 2023 17:36
@nicolas-grekas
Copy link
Member

Thank you @vtsykun.

@nicolas-grekas nicolas-grekas merged commit f65b0a4 into symfony:6.2 Jul 26, 2023
@vtsykun vtsykun deleted the fix/detect-scope branch July 26, 2023 19:31
This was referenced Jul 30, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh Awaiting requested review from xabbuh

@lyrixx lyrixx Awaiting requested review from lyrixx

@dunglas dunglas Awaiting requested review from dunglas

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@OskarStark OskarStark Awaiting requested review from OskarStark

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Bug Status: Needs Review VarExporter
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
3 participants
@vtsykun @nicolas-grekas @carsonbot