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

BUG: Command Line Undefined Variable Warning Notice Flood #2240

Closed
notjustcode-sp opened this issue Jun 25, 2021 · 4 comments · Fixed by #2243
Closed

BUG: Command Line Undefined Variable Warning Notice Flood #2240

notjustcode-sp opened this issue Jun 25, 2021 · 4 comments · Fixed by #2243
Assignees
Labels
bug Something isn't working
Milestone

Comments

@notjustcode-sp
Copy link

When performing a command line sync in blocks of 350 we get the following notice three times:

Warning: Request Entity Too Large

Followed by a warning flood:

PHP Notice: Undefined variable: item in elasticpress/includes/classes/Command.php on line 960

Followed by a further flood:

PHP Notice: Undefined index: type in elasticpress/includes/classes/Command.php on line 1021
PHP Notice: Undefined index: reason in elasticpress/includes/classes/Command.php on line 1022
PHP Notice: Undefined index: type in elasticpress/includes/classes/Command.php on line 1027

We can see that the $attempts for loop is hitting the continue statement on line 941 meaning that we never get to the if/else statement on line 944.

This means that when we get to line 959 $index_objects hasn't been reset (line 951) and $item does not exist (line 945) hence the warning flood.

Whilst the foreach of line 959 could be guarded with a check to see if $item is set, we're not sure whether this is appropriate as we don't understand enough about the code to know what this foreach is doing and why it's using $item.

Steps to Reproduce

  1. Visit top level WP directory.

  2. Run the command:
    wp elasticpress index --setup

  3. Observe the warning flood.

Expected behavior
Warning that the Request entity is too large without the flood of "undefined variable" warning messages.

Environment information

  • Device: Desktop PC
  • OS: Debian 10 buster
  • WordPress version: 5.7.2
  • ElasticPress version: 3.5.6
  • Elasticsearch version: 7.13.2
  • Where do you host your Elasticsearch server? localhost
@notjustcode-sp notjustcode-sp added the bug Something isn't working label Jun 25, 2021
@felipeelia
Copy link
Member

Hi @notjustcode-sp, actually, the warning flood happens because Elasticsearch doesn't send that info for the error you are having (it does for some others). The if/else is there to remove errored objects from the full list and works as expected.

You do have a point though. The plugin shouldn't assume $error has a type or a reason inside output_index_errors. Can you please check if #2243 fixes the error for you? Thanks

@felipeelia felipeelia self-assigned this Jun 28, 2021
@notjustcode-sp
Copy link
Author

notjustcode-sp commented Jun 28, 2021

Hi @felipeelia, unfortunately #2243 does not fix the issue:

$ wp elasticpress index --setup                        
Adding post mapping...
Success: Mapping sent
Indexing posts...
Processed 350/1450...
Warning: Request Entity Too Large
Warning: Request Entity Too Large
Warning: Request Entity Too Large
PHP Notice:  Undefined variable: item in /wp-content/plugins/elasticpress/includes/classes/Command.php on line 960
Notice: Undefined variable: item in //wp-content/plugins/elasticpress/includes/classes/Command.php on line 960
... x350 warnings!

The if/else that removes errored objects is never reached because of the continue statement on line 941.

This means that when we get to line 959 $index_objects does exist, but $item does not exist. It's the foreach on line 959 that's causing the flood we're reporting here.

As previously stated, we're not sure why this foreach statement is using $item on line 960 as we don't know what the purpose of this foreach statement is.

@felipeelia
Copy link
Member

You are absolutely right, @notjustcode-sp, sorry. That $item there was really wrong. Can you please give #2243 another try? I've pushed a new commit there that (1) should address the warning flood problem and (2) give correct details for each failed document.

Thanks!

@notjustcode-sp
Copy link
Author

Success!

No PHP warning flood:

Adding post mapping...
Success: Mapping sent
Indexing posts...
Processed 350/1450. Last Object ID: 14221
Warning: Request Entity Too Large
Warning: Request Entity Too Large
Warning: Request Entity Too Large
Processed 700/1450. Last Object ID: 12861
Warning: Request Entity Too Large
...

We thought $item looked wrong, but didn't know enough about the code to fix it!

Thanks!

@felipeelia felipeelia added this to the 3.6.0 milestone Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants