51423: wp-admin/setup-config.php fixes fwrite handle mismatch

[hellofromtonya]

Trac ticket: https://core.trac.wordpress.org/ticket/51423

• Fixes a fwrite handle type mismatch which happens if fopen fails, i.e. returns false
• Adds error messaging for it

Note:
The new conditional uses the absence of false instead of checking for a resource. Why? PHP has plans to change fopen to return object instead of current resource in a future version. This not falsey check though opposite ensures the code is future-proof when the new version is released.

An inline comment (thanks @peterwilsoncc) is added to explain and provides a learning opportunity for those who read the code.

@props xknown, hellofromTonya, jrf, peterwilsoncc

TODO

• Review new error messaging with the Polyglots team
• Review chmod failed error messaging

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[hellofromtonya]

Force push to reset the file back to original state (except for the indentation). Why? To first show the problem exists in PHP 8, but not in the other PHP versions. Doing this step shows the problem in action for crafting the right fix without causing a breaking change.

[hellofromtonya]

Manual Testing BEFORE the Fix

Environment:

• Using Local as the localhost app
• Nginx
• MySQL 8.0.16
• OS: Mac Big Sur
• Browser: Firefox

To make it testable, needed to create an environment where fopen fails to create the file:

• Changed the permissions of the existing wp-config.php to 000

----------  1 ****  staff  3181 Mar 15 13:25 wp-config.php

• Skipped file_exists check by commenting the check on lines 58 to 69
• Flushed the realpath cache for the file

clearstatcache( ABSPATH . 'wp-config.php' );

Test 1: with PHP 7.3.5

Results: Works, but PHP Warning

From the error.log

PHP Warning:  fopen(.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied in ...setupconfig/app/public/wp-admin/setup-config.php on line 442
PHP Warning:  fwrite() expects parameter 1 to be resource, bool given in .../setupconfig/app/public/wp-admin/setup-config.php on line 443

Test 2: with PHP 8.0

Results: Does not work due to TypeError fatal error

From the error.log

PHP Warning:  fopen(.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied in ...setupconfig/app/public/wp-admin/setup-config.php on line 442
PHP Fatal error:  Uncaught TypeError: fwrite(): Argument #1 ($stream) must be of type resource, bool given in .../setupconfig/app/public/wp-admin/setup-config.php:443
Stack trace:
#0 .../setupconfig/app/public/wp-admin/setup-config.php(443): fwrite(false, '<?php\r\n')
#1 {main}
  thrown in .../setupconfig/app/public/wp-admin/setup-config.php on line 443

[hellofromtonya]

Manual Testing AFTER the Fix

Same environment and test conditions.

Test 1: with PHP 7.3.5

Results: Works with no breaking change.

• wp-config.php file permissions is changed
• Error message displays as expected
• Error Log:
  • No fwrite PHP Warning
  • PHP Warning for fopen as expected: PHP Warning: fopen(/.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied

Test 2: with PHP 8.0

Results: Works.

• wp-config.php file permissions is changed
• Error message displays as expected
• Error log:
  • No PHP TypeError Fatal Error
  • PHP Warning for fopen as expected: PHP Warning: fopen(/.../setupconfig/app/public/wp-config.php): Failed to open stream: Permission denied

[hellofromtonya]

Review in slack by polyglots ✅

[jrfnl]

LGTM. Approved. Let's wait for a second opinion from the security team before submitting it for commit as we're touching the messaging to the end-user with potentially security sensitive information.

[peterwilsoncc]

Reviewed this with whitespace only changes suppressed.

These changes look good to me, a couple of comments/questions inline but nothing I consider a blocker.

[peterwilsoncc]

Sorry, a couple more things.

Re: the string 'Unable to write to %s file.', no need to change if it's from another file. I must have done a silly when searching, probably had regex mode enabled or some such.

[peterwilsoncc]

Lovely! I'll commit these changes today.

Reviewed with whitespace changes suppressed.

[peterwilsoncc]

Committed in https://core.trac.wordpress.org/changeset/50775