Opened 9 months ago

Last modified 7 months ago

#21870 new defect (bug)

@ error control operator hides fatal error on mysql_fetch_object

Reported by: ericlewis Owned by:
Priority: normal Milestone: Future Release
Component: Database Version: 1.5
Severity: normal Keywords: has-patch 3.6-early
Cc:

Description

I ran a get_posts() query that took me a while to track down to the @ error control operator on the mysql_fetch_object call (see wp-db.php:1219 in trunk), therefore PHP was silently die'ing on me because of a memory limit error.

I'm curious as to what use cases there are for the error control operator to be used here. In my case, I want to see these errors and fix my code accordingly, or tailor my query to the memory constraints, or rethink my data relations in the database.

Attachments (2)

21870.diff (1.1 KB) - added by nacin 9 months ago.
21870.2.diff (1.3 KB) - added by ericlewis 7 months ago.
As per the previous talk, forking open the discussed can of worms and removing the error control operator from mysql_query.

Download all attachments as: .zip

Change History (12)

Related: #9158

  • Keywords needs-patch added; 2nd-opinion removed

I assume error suppression is used because mysql_fetch_object() throws a notice when there is no object left to fetch.

Should be a way around that, though.

#11151 as a duplicate?

I don't see any notes in the documentation that mysql_fetch_object() can throw a notice. It returns false if there's nothing else to fetch. However, if the resource isn't a resource, you get a warning. We eliminated similar error suppression in [21511].

nacin9 months ago

comment:4 follow-up: ↓ 6   nacin9 months ago

21870.diff avoids error suppression on mysql_fetch_object(), mysql_num_fields(), mysql_fetch_field().

mysql_query() should additionally be good, because $this->dbh is guaranteed to be a resource, I guess unless someone unset it (if the connection fails, we bail in db_connect()). But touching that seems more dangerous than these.

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Awaiting Review to 3.5

comment:6 in reply to: ↑ 4 ; follow-up: ↓ 7   ericlewis9 months ago

Not to veer to much in the direction of #9158, but what about the @ operator on the mysql_query call a bit earlier in the function?

$this->result = @mysql_query( $query, $this->dbh );

This seems related and perhaps ripe for removal as well, as any error raised by this call is probably developer error.

comment:7 in reply to: ↑ 6   ericlewis8 months ago

Replying to ericlewis:

Not to veer to much in the direction of #9158, but what about the @ operator on the mysql_query call a bit earlier in the function?

$this->result = @mysql_query( $query, $this->dbh );

This seems related and perhaps ripe for removal as well, as any error raised by this call is probably developer error.

Since we'll be taking care of support for PDO and the mysqli_* class in #21663, I'm gonna retract what I said about perhaps removing the error construct for mysql_query(), because that could open up a can of worms that really doesn't need to be opened at all.

Thumbs up on nacin's patch.

comment:8 follow-up: ↓ 9   nacin8 months ago

There are no plans for PDO or mysqli. Not saying it's never going to happen, but *certainly* not for 3.5. Opening the can of worms is fine. I talked about why I avoided mysql_query() above.

comment:9 in reply to: ↑ 8   ericlewis8 months ago

Replying to nacin:

Opening the can of worms is fine. I talked about why I avoided mysql_query() above.

Ah whoops, I thought you meant it was good to leave as is.

As per the previous talk, forking open the discussed can of worms and removing the error control operator from mysql_query.

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release
Note: See TracTickets for help on using tickets.