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

Let Resque_Worker::updateProcLine() use PHP 5.5's cli_set_process_title() if available #131

Merged
merged 3 commits into from
Sep 7, 2013
Merged

Let Resque_Worker::updateProcLine() use PHP 5.5's cli_set_process_title() if available #131

merged 3 commits into from
Sep 7, 2013

Conversation

richardkmiller
Copy link
Contributor

updateProcLine() uses the PECL function setproctitle() if available. It should also try to use cli_set_process_title(), available in PHP 5.5.

cli_set_process_title() is preferred because setproctitle() is "incomplete and might lead to memory corruption on Linux". (See https://wiki.php.net/rfc/cli_process_title )

…is preferred over the PECL function setproctitle().
@danhunsaker
Copy link
Contributor

Seems sound.

@danhunsaker
Copy link
Contributor

On checking the code, it looks good as well, though I have a couple of comments which don't impact functionality.

Would it make sense to construct the title into a temporary variable prior to checking for either function, so we aren't duplicating code (and thereby forcing ourselves to maintain two separate lines of code identically if the title format ever changes for any reason)? Is this a valid application of DRY principles, or simply a tad overboard?

Why on earth is the else if form so common? PHP supports elseif as a single term, which inclines me to believe the latter to be more efficient, but I haven't honestly researched it. From what I can tell, though, it seems to simply be an artifact of working with other languages, particularly JavaScript - in my experience, devs who stay out of JS tend to use the single-word form.

@Rockstar04
Copy link
Contributor

Looks great, but I would like to see the title built once and passed in as a variable like @danhunsaker was saying above.

@richardkmiller
Copy link
Contributor Author

@danhunsaker @Rockstar04 I asked myself the same two questions when I wrote this code:

  1. Should I use a temporary variable for the title, or is that overkill? Seeing as you both mentioned it, I think that sways me toward using one. I'll send an update.
  2. Why "else if"? Good question. I assumed "else if" was a standard in this project because a quick global search produced 0 instances of "elseif" and ~12 instances of "else if". However, I prefer elseif and it's also in the PSR-2 standard. I could go either way for this patch -- whatever you guys want for this project.

@danhunsaker
Copy link
Contributor

Picking nits, but now the indentation isn't consistent. >_> I blame selection of spaces over tabs.

Stupid maximum-fixed-width code lines.

@richardkmiller
Copy link
Contributor Author

Ooops, my fault. Editor in the wrong mode.

@chrisboulton
Copy link
Owner

TIL cli_set_process_title is now a thing, and is now built in. : tada:

Thanks :)

chrisboulton added a commit that referenced this pull request Sep 7, 2013
Let Resque_Worker::updateProcLine() use PHP 5.5's cli_set_process_title() if available
@chrisboulton chrisboulton merged commit 7f2cba1 into chrisboulton:master Sep 7, 2013
glensc added a commit to glensc/php-resque-ex that referenced this pull request Nov 26, 2015
Let Resque_Worker::updateProcLine() use PHP 5.5's cli_set_process_title() if available

chrisboulton/php-resque#131
@richardkmiller richardkmiller deleted the updateProcLine-php5.5 branch January 29, 2020 19:17
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

4 participants