PHP Notice: Undefined offset: 1 in /var/www/html/wp-includes/vars.php on line 31

[miya0001]

Related: #4290

[miya0001]

I realized that this problem will be seen when WP_DEBUG is true.

[gitlost]

Hi @miya0001, yeah it's awkward to force PHP Notices to stderr at the moment in the tests. I think looking at wp_debug_mode() https://github.com/wp-cli/wp-cli/blob/master/php/utils-wp.php#L15 the magic formula is

<?php define( 'WP_DEBUG', true ); define( 'WP_DEBUG_DISPLAY', null ); define( 'WP_DEBUG_LOG', false ); ini_set( "error_log", '' ); ini_set( 'display_errors', 'STDERR' );

I'll push this change against this branch.

The error was introduced by me in #4285 in defining WP_ADMIN true at https://github.com/wp-cli/wp-cli/blob/master/php/WP_CLI/Runner.php#L1143. I have an additional hack to fix this which I'll push here once the tests fail.

[miya0001]

@gitlost
OK, thanks! 😄

[gitlost]

I think this hack is fairly reasonable given that it's only executed on help and that it's really a bug in core not to check that the preg_match() could fail at "wp-includes/vars.php:31".

[danielbachhuber]

it's really a bug in core not to check that the preg_match() could fail at "wp-includes/vars.php:31".

If it's a core bug, can we fix it in core instead of adopting a hack we have to maintain for the long term?

[gitlost]

Well I say it's a core bug, but opinions could differ ("Why would it ever fail?"). Also it could take months.

The original hack of defining WP_ADMIN is also an attempt to get around what I would consider the poor core behaviour of using die() instead of wp_die() (die() should only be used in a very few places I think), which makes portions of the code base untestable, apart from anything else.

Anyway I'll see if I can come up with a better hack than defining WP_ADMIN...

[danielbachhuber]

Well I say it's a core bug, but opinions could differ ("Why would it ever fail?").

Worth opening the issue at least.

The original hack of defining WP_ADMIN is also an attempt to get around what I would consider the poor core behaviour of using die()

I missed this in the original review. I don't think we should define WP_ADMIN in either case. Doing so is a not-insignificant change with the potential for unexpected side-effects (see this issue).

Can we remove the WP_ADMIN definition?

[danielbachhuber]

I don't think we should define WP_ADMIN in either case. Doing so is a not-insignificant change with the potential for unexpected side-effects (see this issue).

See also #674 for additional background.

[miya0001]

I reverted the php/WP_CLI/Runner.php then removed WP_ADMIN definition.

[diggy]

loosely related trac ticket: https://core.trac.wordpress.org/ticket/41171#comment:4

[gitlost]

@diggy - thanks, interesting, and confirms really that the answer to "Why would it ever fail?" is "It shouldn't".

@danielbachhuber @miya0001 Howabout defining WP_DEBUG instead, which seems to work...?

[danielbachhuber]

Howabout defining WP_DEBUG instead, which seems to work...?

It's also another hack that's likely to have unintended consequences.

I'm still not convinced that all of the effort in refactoring this code path (and discovering what breaks when we do) is worth solving the problem presented in #4265

[miya0001]

I guess we should add a new context a WP install with debug mode to the features/steps/given.php.

Because this error is a different error than what I reported, so I want to reproduce this error in a actual WordPress environment.

a WP install with debug mode will install WordPress with WP_DEBUG.
It looks reasonable and we don't need to handle another problem.

How about my idea?

But I am sorry I will work for it tomorrow. 😄

[miya0001]

@gitlost @danielbachhuber

I remove the WP_ADMIN definition and add a new context a WP install with debug mode.
Then I reproduced this problem with a new context and there is a problem which is related on #4265 .

I am thinking we should leave this problem for now and look into more about #4265, #4285.

[danielbachhuber]

I am thinking we should leave this problem for now and look into more about #4265, #4285.

Reverting the original two PRs in #4293

[gitlost]

I'll close this in view of #4293 ... to be continued (edit: in #4265)!!