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

Support stream_select() #8

Closed
wants to merge 1 commit into from

Conversation

alexpott
Copy link
Contributor

Don't error on stream_select() - we can return the original resource.

@alexpott
Copy link
Contributor Author

This doesn't work :(

@alexpott
Copy link
Contributor Author

I need to write tests and find something that does.

@ohader
Copy link
Member

ohader commented Jan 18, 2019

Interesting, what was your idea and approach on using stream_select?

@ohader
Copy link
Member

ohader commented Jan 19, 2019

I read the comments at https://www.drupal.org/project/drupal/issues/3026560 but failed to see how Phar archives might be involved in stream_select. Looking forward to getting some additional feedback on how Drupal or Drush is making use of it.

@ohader
Copy link
Member

ohader commented Feb 12, 2019

Is this one still required? If yes, what would the scenario look like that shall be solved? Thx in advance for feedback!

@alexpott
Copy link
Contributor Author

Drupal users using drush have definitely experienced the error thrown by this method so we do have a way of causing it. However somewhat annyoingly I've never managed to reproduce it. Drush (Drupal's shell tool) has functionality to connect to remote servers and run commands there. I suspect that this is occurring when it is being used to do that but I've not managed to prove that.

@ohader
Copy link
Member

ohader commented Mar 2, 2019

Just for the record:

@ohader
Copy link
Member

ohader commented Mar 2, 2019

Thus, having something in source code does not feel correct in general (most probably that's triggered by Drush, caches, whatever)

    $pointer = fopen('phar://storage/bundle.phar/Resources/content.txt', 'r');
    $read = [$pointer];
    $write = null;
    $except = null;
    $events = stream_select($read, $write, $except, 3);

The following replacement for PharStreamWrapper::stream_cast would "solve" the issue, however actually selecting from a Phar stream is not expected - it's just a work-around for possible code flaws in other components

    /**
     * @param int $cast_as
     */
    public function stream_cast($cast_as)
    {
        return STDIN;
    }

@ohader
Copy link
Member

ohader commented Mar 2, 2019

In https://www.drupal.org/project/drupal/issues/3026560#comment-12931921 I found pointers to DrushInputAdapter and ClassLoader, but could not find any evidence there. Checking stream_resolve_include_path used in ClassLoader did not reveal anything new.

So... I still cannot reproduce this locally...

@hron84
Copy link

hron84 commented Mar 13, 2019

@ohader this resolved a lot of problems for me with DRD Agent module on PHP 5.6. Mostly this error caused on sites that run on PHP 5.6 and before you say: there is currently no way to switch from that version for us in those specific cases. Most of our sites run on PHP 7.1 but there are older sites that we need to support but we don't have resource for upgrade them in terms of PHP (abandoned contrib modules causes this moslty).

Please run your tests against PHP 5.6 and since Drupal 8.x still support this PHP version please provide some solution for this specific exception. Something, like checking phpversion() and returning with STDIN, would work.

@ohader
Copy link
Member

ohader commented May 6, 2019

@hron84 I was playing around with different old PHP 5.6 versions (5.6.x-cli as Docker image) but failed to reproduce the mentioned behavior (e.g. trying to setup Drush as well as process forking using symfony/process component).

https://www.drupal.org/project/drd_agent/issues/3039384 mentions

[..] The patch in that other issue is just removing the function that throws the exception [..]

In case removing the whole PharStreamWrapper::stream_cast method helps, I could include that in one of the next releases - please just let me know whether that really solves the issue.

Side note on STDIN from above - which is really just a hack. I am reluctant in putting that into the package since STDIN is actually used then - including all new side effects that may be causing. For instance using the code of #8 (comment) and focussing on the result of $events changes with a simple experiment in CLI:

  • php example.php has $events: 0
  • php example.php < example.php has $events: 1 - sure since there is real STDIN

@hron84
Copy link

hron84 commented May 6, 2019

I could be fine with it. I did not figured out what conditions trigger Drupal / DRD to use stream_cast but almost all our PHP 5.6 sites were affected in this issue. I had to manually hack them regardless if they're on Drupal 6, Drupal 7 or Drupal 8. If you say removing the method can help - I will absolute love this solution. As I said, I am not a PHP developer rather a sysadmin, from my aspect if the error will not came back then I can collect my profit.

@ohader
Copy link
Member

ohader commented May 6, 2019

Alright & thx for your feedback. I'm going to raise my question later in the DRD issue tracker as well...

@ohader
Copy link
Member

ohader commented Oct 17, 2019

Closed since it could not be directly reproduced with various PHP 5.6 Docker containers...

@ohader ohader closed this Oct 17, 2019
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

3 participants