WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 5 months ago

Last modified 7 days ago

#21870 closed defect (bug) (fixed)

@ error control operator hides fatal error on mysql_fetch_object

Reported by: ericlewis Owned by: ericlewis
Milestone: 4.5 Priority: normal
Severity: normal Version: 1.5
Component: Database Keywords: has-patch commit
Focuses: 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 (4)

21870.diff (1.1 KB) - added by nacin 4 years ago.
21870.2.diff (1.3 KB) - added by ericlewis 4 years ago.
As per the previous talk, forking open the discussed can of worms and removing the error control operator from mysql_query.
21870.patch (1.8 KB) - added by tyxla 16 months ago.
Refreshing the latest patch to be compatible with the current code version.
21870.3.diff (3.5 KB) - added by ericlewis 5 months ago.

Download all attachments as: .zip

Change History (31)

#1 @ericlewis
4 years ago

Related: #9158

#2 @scribu
4 years ago

  • 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.

#3 @nacin
4 years ago

#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].

@nacin
4 years ago

#4 follow-up: @nacin
4 years 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.

#5 @nacin
4 years ago

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

#6 in reply to: ↑ 4 ; follow-up: @ericlewis
4 years 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.

#7 in reply to: ↑ 6 @ericlewis
4 years 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.

#8 follow-up: @nacin
4 years 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.

#9 in reply to: ↑ 8 @ericlewis
4 years 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.

@ericlewis
4 years ago

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

#10 @nacin
3 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

#11 follow-up: @rmccue
2 years ago

Woo, 3.6-early! :) 4.0-early anyone?

#12 in reply to: ↑ 11 @pento
2 years ago

  • Keywords needs-refresh added; has-patch 3.6-early removed

Replying to rmccue:

Woo, 3.6-early! :) 4.0-early anyone?

Thanks for volunteering! ;-)

#13 @ericlewis
18 months ago

I ran into a similar problem with hyperdb today. My feeling is we should remove @ operators entirely, and rewrite anything that may produce a PHP notice properly.

#14 @wonderboymusic
16 months ago

  • Milestone changed from Future Release to 4.2

@tyxla
16 months ago

Refreshing the latest patch to be compatible with the current code version.

#15 @ericlewis
16 months ago

While we're at it, we should consider removing all 14 @mysql_* invocations.

1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14.

#16 @nacin
16 months ago

  • Owner set to pento
  • Status changed from new to assigned

There is a *lot* of history here. A few of us spelunked this years ago and did remove some. Let's leave this for pento to make any decisions and commits.

#17 @pento
16 months ago

  • Status changed from assigned to accepted

Aye, some of these can go away, with appropriate sanity checks. Hopefully there won't be too much stuff that explodes when we do.

#18 @DrewAPicture
14 months ago

  • Milestone changed from 4.2 to Future Release

A bit too late to dive down this rabbit hole for 4.2. @pento: Feel free to re-milestone if you feel like it's worth rolling the dice during beta.

#19 @pento
5 months ago

  • Milestone changed from Future Release to 4.5
  • Owner changed from pento to ericlewis
  • Status changed from accepted to assigned

This looks like fun. Let's refresh the patch and add the bits from comment #15, then see what happens.

@ericlewis
5 months ago

#20 @ericlewis
5 months ago

attachment:21870.3.diff removes all @ operators, and adds a few sanity checks to pass unit tests.

#21 @pento
5 months ago

  • Keywords has-patch commit added; needs-refresh removed

21870.3.diff looks like a good start to me. Lets commit it and see what explodes.

#22 @ericlewis
5 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 35860:

Don't suppress error messages in database function calls.

Fixes #21870.

#23 @Otto42
2 weeks ago

Just a note, in WordPress 4.5, this fix is now causing issues where hosts with PHP 5.5 and WP 4.5 are attempting to connect with mysqli, and the mysql_old_password error is not being ignored properly and output to the page.

https://wordpress.org/support/topic/authentication-failure-on-upgrading-to-wp45?replies=3

Because of the early output, cookies cannot be set, rendering users unable to log into wp-admin for their sites.

The fallback to the mysql client still appears to work okay on these sites (sometimes), however errors from mysqli_real_connect should be caught and handled somewhat better, I think. The real fix, of course, is for the database servers not to be configured to use the old_password hash mechanism, but still, that's a valid warning message to attempt to suppress.

#24 @Otto42
2 weeks ago

Now that warnings are not being suppressed anymore, a new warning is showing up for some users:

https://wordpress.org/support/topic/wp45-upgrade-mysql_connect-error

This one is:

Warning: mysql_connect(): Headers and client library minor version mismatch. Headers:xxx Library:xxx in .../wp-includes/wp-db.php on line 1515

It can be resolved by switching the installations to use the nd_mysql or mysqlnd (native driver) libraries instead of the normal mysql or mysqli ones. This eliminates the need for the mysql client libraries, and thus resolves the version mismatch between them.

Edit: An easier fix which has been working for people is to configure their host to use a newer PHP version, which generally is configured correctly unlike older versions.

Last edited 7 days ago by Otto42 (previous) (diff)

This ticket was mentioned in Slack in #core by dd32. View the logs.


7 days ago

#26 @pento
7 days ago

In 37292:

Database: Suppress connection error messages when WP_DEBUG isn't enabled.

This is a partial revert of [35860], which has been causing un-catchable warnings to be generated on some server configurations.

Fixes #36629 for trunk.
See #21870.

#27 @pento
7 days ago

In 37293:

Database: Suppress connection error messages when WP_DEBUG isn't enabled.

This is a partial revert of [35860], which has been causing un-catchable warnings to be generated on some server configurations.

Merge of [37292] to the 4.5 branch.

Fixes #36629.
See #21870.

Note: See TracTickets for help on using tickets.