Skip to content

Ensure headers have not been sent before attempting to set a cookie#14256

Merged
brbrr merged 1 commit intomasterfrom
prevent/php-notice
Dec 25, 2019
Merged

Ensure headers have not been sent before attempting to set a cookie#14256
brbrr merged 1 commit intomasterfrom
prevent/php-notice

Conversation

@kraftbj
Copy link
Copy Markdown
Contributor

@kraftbj kraftbj commented Dec 17, 2019

Changes proposed in this Pull Request:

  • Checks for headers_sent before setting a cookie.

This can be seen via wp jetpack module activate minileven which results a notice in the logs:

[17-Dec-2019 23:07:31 UTC] PHP Warning:  Cannot modify header information - headers already sent by (output started at /usr/local/bin/psysh:1) in /var/www/html/wp-content/plugins/jetpack/class.jetpack.php on line 5617
[17-Dec-2019 23:07:31 UTC] PHP Stack trace:
[17-Dec-2019 23:07:31 UTC] PHP   1. {main}() /usr/local/bin/wp:0
[17-Dec-2019 23:07:31 UTC] PHP   2. include() /usr/local/bin/wp:4
[17-Dec-2019 23:07:31 UTC] PHP   3. include() phar:///usr/local/bin/wp/php/boot-phar.php:11
[17-Dec-2019 23:07:31 UTC] PHP   4. WP_CLI\bootstrap() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/wp-cli.php:27
[17-Dec-2019 23:07:31 UTC] PHP   5. WP_CLI\Bootstrap\LaunchRunner->process() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/bootstrap.php:74
[17-Dec-2019 23:07:31 UTC] PHP   6. WP_CLI\Runner->start() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Bootstrap/LaunchRunner.php:23
[17-Dec-2019 23:07:31 UTC] PHP   7. WP_CLI\Runner->run_command_and_exit() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:1160
[17-Dec-2019 23:07:31 UTC] PHP   8. WP_CLI\Runner->run_command() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:394
[17-Dec-2019 23:07:31 UTC] PHP   9. WP_CLI\Dispatcher\Subcommand->invoke() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Runner.php:371
[17-Dec-2019 23:07:31 UTC] PHP  10. call_user_func:{phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/Subcommand.php:451}() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/Subcommand.php:451
[17-Dec-2019 23:07:31 UTC] PHP  11. WP_CLI\Dispatcher\CommandFactory::WP_CLI\Dispatcher\{closure:phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php:95-102}() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/Subcommand.php:451
[17-Dec-2019 23:07:31 UTC] PHP  12. call_user_func:{phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php:98}() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php:98
[17-Dec-2019 23:07:31 UTC] PHP  13. Jetpack_CLI->module() phar:///usr/local/bin/wp/vendor/wp-cli/wp-cli/php/WP_CLI/Dispatcher/CommandFactory.php:98
[17-Dec-2019 23:07:31 UTC] PHP  14. Jetpack::activate_module() /var/www/html/wp-content/plugins/jetpack/class.jetpack-cli.php:531
[17-Dec-2019 23:07:31 UTC] PHP  15. Jetpack::state() /var/www/html/wp-content/plugins/jetpack/class.jetpack.php:3041
[17-Dec-2019 23:07:31 UTC] PHP  16. setcookie() /var/www/html/wp-content/plugins/jetpack/class.jetpack.php:5617

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • n/a

Testing instructions:

  • See above.

Proposed changelog entry for your changes:

  • CLI: Prevents a PHP notice when running some Jetpack CLI commands.

@kraftbj kraftbj added Bug When a feature is broken and / or not performing as intended General [Status] Needs Review This PR is ready for review. [Feature] Jetpack CLI labels Dec 17, 2019
@kraftbj kraftbj added this to the 8.1 milestone Dec 17, 2019
@kraftbj kraftbj requested a review from a team December 17, 2019 23:21
@jetpackbot
Copy link
Copy Markdown
Collaborator

jetpackbot commented Dec 17, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: January 14, 2020.
Scheduled code freeze: January 7, 2020

Generated by 🚫 dangerJS against 328b173

jeherve
jeherve approved these changes Dec 18, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This makes sense, however I can't seem to be able to reproduce the original issue. Was this happening in a specific PR?

@kraftbj
Copy link
Copy Markdown
Contributor Author

kraftbj commented Dec 18, 2019

@jeherve It was in my logs after testing the Mobile Theme deprecation PR via yarn docker:wp...

brbrr
brbrr approved these changes Dec 25, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Tests well 🚢

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Dec 25, 2019
@brbrr brbrr merged commit baf13c5 into master Dec 25, 2019
@brbrr brbrr deleted the prevent/php-notice branch December 25, 2019 11:18
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 25, 2019
zinigor added a commit that referenced this pull request Dec 27, 2019
@zinigor
Changelog: add #14256
65656de
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
fff089c 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <zinigor@gmail.com>
zinigor added a commit that referenced this pull request Dec 30, 2019
@jeherve @zinigor
Changelog: 8.1 additions (#14166)
d4a7565 
* Changelog: 8.1 additions

* Changelog: add #13858

* Changelog: add #13963

* Changelog: add #14174

* Changelog: add #14178

* Changelog: add #14175

* Changelog: add #14192

* Changelog: add #14196

* Changelog: add #14182

* Changelog: add #14218

* Changelog: add #14214

* Changelog: add #13757

* Changelog: add #14190

* Changelog: add #14131

* Changelog: add #14101

* Changelog: add #14203

* Changelog: add #14211

* Changelog: add #14224

* Changelog: add #14230

* Changelog: add #14241

* Changelog: add #14249

* Changelog: add #14264

* Changelog: add #14263

* Changelog: add #14256

* Changelog: add #10189

* Changelog: add #14240

* Changelog: add #14239

Also added some new entries to the testing file.

Co-authored-by: Igor Zinovyev <zinigor@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve approved these changes

+1 more reviewer

@brbrr brbrr brbrr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@kraftbj kraftbj

Labels

Bug When a feature is broken and / or not performing as intended [Feature] Jetpack CLI General

Projects

None yet

Milestone

8.1

Development

Successfully merging this pull request may close these issues.

6 participants

@kraftbj @jetpackbot @jeherve @brbrr @zinigor @matticbot