Make WordPress Core

Opened 13 years ago

Closed 13 years ago

#19914 closed defect (bug) (fixed)

wp-app.php assumes elements exist

Reported by: rmccue's profile rmccue Owned by: ryan's profile ryan
Milestone: 3.4 Priority: normal
Severity: normal Version: 2.3
Component: AtomPub Keywords: has-patch
Focuses: Cc:

Description

First bug found by Gorilla, yay!

So, submitting a very sparse entry gives errors.

Submitted document:

<?xml version="1.0" encoding="utf-8"?>
<entry>
    <title>From a Gorilla!</title>
    <id>tag:ryanmccue.info,2012:016540021841</id>
    <summary type="text">Summary from a &lt;strong&gt;&amp;lt;Gorilla&amp;gt;&lt;/strong&gt; at 2012-01-28T23:30:20+10:00</summary>
</entry>

Gives the following errors:

Notice:  Trying to get property of non-object in wp-app.php on line 403
Warning:  Invalid argument supplied for foreach() in wp-app.php on line 403
Notice:  Trying to get property of non-object in wp-app.php on line 426
Notice:  Trying to get property of non-object in wp-app.php on line 427
Notice:  Trying to get property of non-object in wp-app.php on line 428
Notice:  Trying to get property of non-object in wp-app.php on line 429

Attachments (3)

19914.patch (1.3 KB) - added by kurtpayne 13 years ago.
19914.2.patch (1.3 KB) - added by kurtpayne 13 years ago.
19914.3.patch (1.6 KB) - added by kurtpayne 13 years ago.

Download all attachments as: .zip

Change History (18)

#1 @rmccue
13 years ago

(To be fair though, some of those are caused by my missing XMLNS, but the errors are still valid. wp-app.php shouldn't have PHP errors with invalid input)

#2 @rmccue
13 years ago

Some extra ones from after creation:

Notice:  Undefined property: AtomEntry::$content in wp-app.php on line 427
Notice:  Undefined property: AtomEntry::$published in wp-app.php on line 429

#3 @westi
13 years ago

  • Cc westi added
  • Keywords needs-patch added

#4 follow-up: @rmccue
13 years ago

Oh, another good idea would be to do error_reporting(0) to avoid outputting any errors at all, regardless of WP_DEBUG, as it's not a HTML endpoint.

@kurtpayne
13 years ago

#5 in reply to: ↑ 4 ; follow-up: @kurtpayne
13 years ago

  • Cc kpayne@… added
  • Keywords has-patch added

Hi rmccue,

I've submitted a patch for the issues you've pointed out. I was unable to recreate the problems you originally reported using Gorilla. Can you please post the instructions on how to recreate? I'm using this command:

gorilla --uri=http://localhost/wp-app.php --user=admin --pass=******** EntryPostsTest

Replying to rmccue:

Oh, another good idea would be to do error_reporting(0) to avoid outputting any errors at all, regardless of WP_DEBUG, as it's not a HTML endpoint.

Perhaps it would be better to disable error output and send errors to an error log.

#6 in reply to: ↑ 5 @rmccue
13 years ago

Replying to kurtpayne:

I've submitted a patch for the issues you've pointed out. I was unable to recreate the problems you originally reported using Gorilla. Can you please post the instructions on how to recreate?

Apologies Kurt, I didn't realise my version of PHPUnit was so out-of-date. If you pull again now, it should spit out the errors properly. (If you're still seeing nothing, try -v as well to enable debug mode.

Perhaps it would be better to disable error output and send errors to an error log.

Agreed.

#7 @rmccue
13 years ago

By the way, patch looks good, but I think isset( $entry->categories ) should probably be !empty( $entry->categories ) instead. For the strings, I think that should check $var[1] rather than just $var.

@kurtpayne
13 years ago

#8 follow-ups: @kurtpayne
13 years ago

  • Keywords needs-patch removed

Replying to rmccue:

I think isset( $entry->categories ) should probably be !empty( $entry->categories ) instead.

Agreed.

For the strings, I think that should check $var[1] rather than just $var.

Disagree. We should definitely check for $var before trying to access any sub elements. Switched these to empty()` calls and reversed the order of the ternary to be in-line with the coding standards.

Perhaps it would be better to disable error output and send errors to an error log.

Agreed.

This is already possible using wp-config.php:

define('WP_DEBUG_LOG', true);
define('WP_DEBUG_DISPLAY', false);

Also, Gorilla worked beautifully this time!

#9 in reply to: ↑ 8 @rmccue
13 years ago

Replying to kurtpayne:

Disagree. We should definitely check for $var before trying to access `any sub elements.

I meant to check both, sorry. Otherwise $var could be an empty array and then cause it to error.

Switched these to empty() calls and reversed the order of the ternary to be in-line with the coding standards.

I think you actually want isset there, since it could be an empty string (which would give the same result of course, but less efficiently).

This is already possible using wp-config.php:

Not separately for APP/XMLRPC endpoints though. Whether that actually warrants a patch is a different story though.

Also, Gorilla worked beautifully this time!

Awesome, glad to hear it.

@kurtpayne
13 years ago

#10 @kurtpayne
13 years ago

I think 19914.3.patch combines all of the error checking we've discussed. Gorilla passes with this patch, and there are no errors in the logs.

#11 @kurtpayne
13 years ago

  • Version set to 2.3

#12 @rmccue
13 years ago

Looks good to me. (Minor style nitpick for whoever commits: =(string) should be = (string) on line 441. No real need for another patch though, IMHO.)

#13 in reply to: ↑ 8 @SergeyBiryukov
13 years ago

Replying to kurtpayne:

Switched these to empty() calls and reversed the order of the ternary to be in-line with the coding standards.

BTW, there's an exception for !empty() in the coding standards (testing for false seems more readable in this case).

#14 @ryan
13 years ago

  • Milestone changed from Awaiting Review to 3.4

#15 @ryan
13 years ago

  • Owner set to ryan
  • Resolution set to fixed
  • Status changed from new to closed

In [19926]:

Don't assume elements exist. Check to avoid notices. Props kurtpayne, rmccue. fixes #19914

Note: See TracTickets for help on using tickets.