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

Fixes #3351: site-install Drupal 7 with PHP 7.2 #3353

Merged
merged 2 commits into from
Feb 5, 2018

Conversation

greg-1-anderson
Copy link
Member

Always use 'fwrite' instead of 'print' in 'drush_print' to avoid undesired interaction with PHP's handling of the php headers.

…to avoid undesired interaction with PHP's handling of the php headers.
@greg-1-anderson
Copy link
Member Author

A couple of tests fail because this change interferes with the way they were collecting output

@ryanaslett
Copy link

I'll give this a whirl tomorrow morning

@greg-1-anderson
Copy link
Member Author

The test failures were happening because output emitted via fwrite(STDOUT) is not captured by ob_start() / ob_end(). I adjusted for this by explicitly capturing output in drush_print().

The side-effect here is that if anyone is emitting output via print instead of drush_print, it has the potential to do the wrong thing. It has always been deprecated to call print instead of drush_print, though, so this is not a serious issue.

We have two options:

  1. Leave this PR as-is.
  2. Conditionalize this change on the php version, so that PHP 7.1 and lower continue to use the current implementation, and PHP 7.2 and onward use the implementation in this PR.

The later strategy would be better for strict maintenance of backwards compatibility; however, since we are only breaking non-conforming code here, I think it is probably okay to just make the change. I'm willing to go either way, though, depending on what folks think.

@weitzman
Copy link
Member

weitzman commented Feb 5, 2018

I'm fine with the PR as is.

@greg-1-anderson greg-1-anderson merged commit a97c1d1 into 8.x Feb 5, 2018
@weitzman weitzman deleted the avoid-header-interaction branch December 13, 2019 15:45
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.

3 participants