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

Add WP_DEBUG_LOG #499

Merged
merged 1 commit into from
Mar 17, 2020
Merged

Add WP_DEBUG_LOG #499

merged 1 commit into from
Mar 17, 2020

Conversation

tangrufus
Copy link
Member

@swalkinshaw
Copy link
Member

Maybe we should set a good default one in the development config? Is app/uploads the only guaranteed writeable place?

@tangrufus
Copy link
Member Author

Is app/uploads the only guaranteed writeable place?

I believe so.

Updated default to $webroot_dir . Config::get('CONTENT_DIR') . '/uploads/debug.log'.

@@ -106,6 +106,7 @@
* Debugging Settings
*/
Config::define('WP_DEBUG_DISPLAY', false);
Config::define('WP_DEBUG_LOG', env('WP_DEBUG_LOG') ?? $webroot_dir . Config::get('CONTENT_DIR') . '/uploads/debug.log');
Copy link
Member

Choose a reason for hiding this comment

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

WP_CONTENT_DIR . '/uploads/debug.log'

Copy link
Member Author

Choose a reason for hiding this comment

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

Config::apply is not yet applied at this point.
WP_CONTENT_DIR is most likely undefined.

Copy link
Member

Choose a reason for hiding this comment

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

Even with Config::get('WP_CONTENT_DIR')? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Config::define('WP_DEBUG_LOG', env('WP_DEBUG_LOG') ?? "${WP_CONTENT_DIR}/uploads/debug.log"); 

Config::apply();
==> PHP Notice:  Undefined variable: WP_CONTENT_DIR in ......./config/application.php on line 129

echo WP_DEBUG_LOG;
==> /uploads/debug.log

Copy link
Contributor

@austinpray austinpray Mar 16, 2020

Choose a reason for hiding this comment

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

Can we avoid setting a default here? This could cause unintended consequences on high-availability setups. In the case WP_DEBUG is set to true on purpose or on accident this is a fail-open behavior rather than fail-safe

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we I recommend avoiding setting a default here. This could cause unintended consequences on high-availability setups. In the case WP_DEBUG is set to true on purpose or on accident this is a fail-open behavior rather than fail-safe.

Copy link
Member Author

@tangrufus tangrufus Mar 16, 2020

Choose a reason for hiding this comment

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

How about we settle for:

Config::define('WP_DEBUG_LOG', env('WP_DEBUG_LOG') ?? false); 

Reason: https://github.com/WordPress/WordPress/blob/7004afe4f4bac1fd17a142051832bdf6be8e6fcf/wp-includes/default-constants.php#L87-L89

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup that LGTM. fail-safe default.

Copy link
Member Author

@tangrufus tangrufus Mar 16, 2020

Choose a reason for hiding this comment

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

And, what do we do on development?

When using valet, it seems difficult to get the valet logs directory path.

When using trellis, roots/trellis#1160 handles it.


I think my brain has bugs. I kept missing details in the PR comments.....

Copy link
Contributor

Choose a reason for hiding this comment

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

In development the developer just does whatever works best for their development setup.

@swalkinshaw swalkinshaw requested a review from austinpray March 16, 2020 17:33
@@ -106,6 +106,7 @@
* Debugging Settings
*/
Config::define('WP_DEBUG_DISPLAY', false);
Config::define('WP_DEBUG_LOG', env('WP_DEBUG_LOG') ?? $webroot_dir . Config::get('CONTENT_DIR') . '/uploads/debug.log');
Copy link
Contributor

@austinpray austinpray Mar 16, 2020

Choose a reason for hiding this comment

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

Can we avoid setting a default here? This could cause unintended consequences on high-availability setups. In the case WP_DEBUG is set to true on purpose or on accident this is a fail-open behavior rather than fail-safe

Copy link
Contributor

@austinpray austinpray left a comment

Choose a reason for hiding this comment

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

:shipit:

@tangrufus tangrufus merged commit 919ece1 into master Mar 17, 2020
@tangrufus tangrufus deleted the wp-debug-log branch March 17, 2020 09:45
@tangrufus
Copy link
Member Author

Thank you both.
Sorry my brain not functioning yesterday.

@@ -13,6 +13,7 @@ DB_PASSWORD='database_password'
WP_ENV='development'
WP_HOME='http://example.com'
WP_SITEURL="${WP_HOME}/wp"
WP_DEBUG_LOG=/path/to/debug.log
Copy link
Member

Choose a reason for hiding this comment

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

Default value should be true imo so that it goes with default WordPress behavior.

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