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

dataAvailable method backported to 2.4 #1098

Closed
wants to merge 7 commits into from
Closed

dataAvailable method backported to 2.4 #1098

wants to merge 7 commits into from

Conversation

rikvdh
Copy link

@rikvdh rikvdh commented Jan 29, 2013

I already did this for 3.0 earlier, now for 2.4

*/
public function dataAvailable($timeout = 0) {
$readFds = array($this->_input);
$readyFds = stream_select($readFds, $w = null, $e = null, $timeout);
Copy link
Member

Choose a reason for hiding this comment

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

Inline assignments in function calls always look like a mistake. You should assign these variables above the function call and pass them in.

@rikvdh
Copy link
Author

rikvdh commented Jan 30, 2013

@markstory See pull request #1020 ;-)

@dereuromark
Copy link
Member

I don't think the initialization is even necessary.
In PHP you can just do stream_select($readFds, $writeFds, $errorFds, $timeout);.
$writeFds, $errorFds will automatically be initialized here as they are merely return variables.

A (small) flaw in PHP itself one could say.

@markstory
Copy link
Member

Well I made a mistake last time, these things happen. I'll fix the code in 3.0.

@rikvdh
Copy link
Author

rikvdh commented Jan 30, 2013

I think it the initialization is necessary because these parameters are also a input to this function, by passing uninitialized variables you don't actually know what happens and PHP throws a warning.

From: http://php.net/manual/en/function.stream-select.php

e.g. write: 'The streams listed in the write array will be watched to see if a write will not block.'

which means that (even though its a reference) it still is input.

@dereuromark
Copy link
Member

Interesting, I only know if from exec() etc where you don't need to do that. But maybe this method behaves differently.

@markstory
Copy link
Member

Based on my local testing, this method like most of the gross C API's in PHP don't need the variables initialized.

<?php
error_reporting(-1);
$fds = array(fopen('php://stdin', 'r'));
$ready = stream_select($fds, $write, $error, 0);

Works fine on my machine without any notice errors.

@rikvdh
Copy link
Author

rikvdh commented Jan 30, 2013

I still don't like it, when the PHP interpreter changes this may give problems, just relying on the default of the interpreter is not the best way to go in my opinion.

@dereuromark
Copy link
Member

This behavior is quite unlikely to ever change :)

@rikvdh
Copy link
Author

rikvdh commented Jan 30, 2013

@dereuromark, Yeah I think your right. Still I think its better to init imo. BUT now we saved 2 lines of code!

@dereuromark
Copy link
Member

You still need to squash, though.

@rikvdh
Copy link
Author

rikvdh commented Jan 31, 2013

@dereuromark what do you mean?

@dereuromark
Copy link
Member

those 5 commits should be merged into a single one by rebasing it upstream and squashing them into a single commit.

@ceeram
Copy link
Contributor

ceeram commented Jan 31, 2013

@markstory except that phpcs will complain about Unused variable.

@markstory
Copy link
Member

@ceeram That's a separate problem, I can take care of that after the merge.

Rik van der Heijden added 2 commits February 2, 2013 14:34
* '2.4' of github.com:djbobke/cakephp:
  Remove variable initialization
@rikvdh
Copy link
Author

rikvdh commented Feb 2, 2013

See #1112 :)

@markstory
Copy link
Member

See #1112

@markstory markstory closed this Feb 2, 2013
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.

4 participants