fix: resolve ChildProcess dynamic property warnings

[KevinBatdorf]

• Implement __get magic method for settings access
• Add test verification for property hydration
• Remove direct dynamic property assignments

Addresses this warning

[warning] Creation of dynamic property Native\Laravel\ChildProcess::$iniSettings is deprecated in /Users/kevin/code/native/mcp-manager/vendor/nativephp/laravel/src/ChildProcess.php on line 150 []

For the test, I had to scope them to bypass the mock, so each line has an indentation update. The only change in that file is

describe('Hydration', function () {
    it('hydrates runtime properties from API responses', function () {
        $processData = [
            'pid' => 1234,
            'settings' => [
                'iniSettings' => ['memory_limit' => '256M'],
            ],
        ];
        Http::fake([
            'http://localhost:4000/api/child-process/start-php' => Http::response($processData, 200),
        ]);

        $childProcess = new ChildProcessImplement(resolve(Client::class));

        $result = $childProcess->php("-r 'sleep(5);'", 'some-alias');
        expect($result->pid)->toBe(1234);
        expect($result->iniSettings)->toBe(['memory_limit' => '256M']);
    });
});

[SRWieZ]

Why not simply add $iniSettings as a property?

[PeteBishwhip]

Thank you for your first contribution! 🚀

This to me is quite ambiguous. Although quite an elegant way of ensuring fatal errors wouldn't occur, it's not clear and obvious that calling $childProcess->something is meant to be a setting value only.

Could we add an $iniSettings property explicitly instead? This is far less ambiguous and keeps that separation.

[KevinBatdorf]

yeah I see whtat you mean. I don't know much about the expected values, but I think we could either set defaults like this:

protected function fromRuntimeProcess($process)
{
    $this->pid = $process['pid'] ?? 0;
    $this->alias = $process['settings']['alias'] ?? '';
    $this->cmd = $process['settings']['cmd'] ?? [];
    $this->cwd = $process['settings']['cwd'] ?? null;
    $this->env = $process['settings']['env'] ?? null;
    $this->persistent = $process['settings']['persistent'] ?? false;
    $this->iniSettings = $process['settings']['iniSettings'] ?? [];
    // Could also make all the values nullable and set them to null.

    return $this;
}

or checking if each are set

protected function fromRuntimeProcess($process)
{
    if (isset($process['pid'])) {
        $this->pid = $process['pid'];
    }

    if (isset($process['settings']['alias'])) {
        $this->alias = $process['settings']['alias'];
    }

    if (isset($process['settings']['cmd'])) {
        $this->cmd = $process['settings']['cmd'];
    }

    if (isset($process['settings']['cwd'])) {
        $this->cwd = $process['settings']['cwd'];
    }

    if (isset($process['settings']['env'])) {
        $this->env = $process['settings']['env'];
    }

    if (isset($process['settings']['persistent'])) {
        $this->persistent = $process['settings']['persistent'];
    }

    if (isset($process['settings']['iniSettings'])) {
        $this->iniSettings = $process['settings']['iniSettings'];
    }

    return $this;
}

Another probably less desirable option is to just leave the code as it was and set a class attribute allowing dynamic properties to be set
https://www.php.net/manual/en/class.allowdynamicproperties.php

What do you think?

[simonhamp]

@KevinBatdorf I agree that this could be more robust and explicit about which values are expected, but I also agree with @SRWieZ and @PeteBishwhip. Using __get() feels like overkill.

Your explicit listing of all of the properties also feels messy - in either form.

I'm going to close this in favour of #535, which I think needs a little more work too.

Looking forward to your next PR!