Skip to content
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.

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

[process] provide a restart method. #5456

Closed
wants to merge 6 commits into from

Conversation

boombatower
Copy link
Contributor

Pull request for issue #5452.

Another possibility would be to allow for either run() or start() scenarios, but I am not sure that is terribly useful since restart() with a new process lends itself to restarting longer running services when they crash and you want the old process so you can inspect the logs and what not.

Otherwise, something like this might work, but doesn't allow for run() to return status code. Someone can get around that by getting manually on returned process.

<?php
public function restart($method = 'start', $callback = null)
{
    if ($this->isRunning()) {
        throw new \RuntimeException('Process is already running');
    }

    if ($method != 'start' && $method != 'run') {
        throw new \RuntimeException('Method must be start or run');
    }

    $process = clone $this;
    $process->$method();
    return $process;
}

@pborreli
Copy link
Contributor

pborreli commented Sep 7, 2012

can you add some tests please ?

@@ -143,6 +143,21 @@ public function __destruct()
$this->stop();
}

public function __clone()
{
unset($this->exitcode);
Copy link
Contributor

Choose a reason for hiding this comment

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

unset() accepts multi values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but is that preferred? and do you want them on one line or wrapped for each variable.

<?php
unset($var1, $var2);
unset(
  $var1,
  $var2
);

Or some variation.

unset($this->pipes);
unset($this->process);
unset($this->fileHandles);
unset($this->readBytes);
Copy link
Member

Choose a reason for hiding this comment

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

you should not unset the properties of the class but only reset the values in the object only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you mean. Since the default state is non-set, unset() returns them to that state. Anything else like setting equal to null would not be equivalent.

Copy link
Member

Choose a reason for hiding this comment

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

unset($this->stdout) does not unset the value of the property. It unsets the property itself. Try to do a property_exists call after that.

If you try reading one of these properties before writing in it again, you will have an E_STRICT error. And if you write first, you will end with a public properties as properties added on the fly are public.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to know and seems somewhat non-intuitive. So what is the proper solution then to "reset the value" setting to null is the closest it seems.

Copy link
Member

Choose a reason for hiding this comment

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

it is the way to do it.

fabpot added a commit that referenced this pull request Sep 10, 2012
Commits
-------

622102e [Process] change all  documentation type to callable

Discussion
----------

[Process] change all  documentation type to callable

Update documentation per #5456 instead of mixing in with restart() pull request.
@fabpot fabpot closed this in 0ba4886 Oct 4, 2012
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.

None yet

5 participants