WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#32167 new defect (bug)

[32307] does not set $wdpb->error

Reported by: nerrad Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.2.1
Component: Database Keywords: dev-feedback has-patch
Focuses: Cc:

Description

Great work on the fix that got pushed out on [32307] but should it set $wpdb->error to a value? Right now when false is returned $wpdb->error == ''

If I get a chance later today I'll add a patch but just wanted to get this up.

Attachments (1)

32167.diff (770 bytes) - added by nerrad 2 years ago.
First pass of context messages for process_field errors.

Download all attachments as: .zip

Change History (11)

#1 @swissspidy
2 years ago

  • Keywords close added

Is this still an issue?

IMHO the function description is pretty clear: "@return false if any of the values were too long for their corresponding field."
No need for a $wpdb->error there.

#2 @ruud@…
2 years ago

  • Keywords needs-patch added

Hi @swissspidy, thnx for taking the time to think this through, I like to express my concerns in this matter.

I was following this ticket because in my case it was related to a bug where the nicename of a new user was getting too long during the initial registration process and an user record wasn't created. (patched separately for 4.4)

So I was figuring out why no records where made, but no visible or otherwise helpful errors where being made. In the end it turned out to be the DB field length for 'nicename'

I guess it would help future debuggers if a more descriptive error would be available, especially around difficult to find errors like DB-field lengths.

Thnx,
Ruud

#3 @nerrad
2 years ago

  • Keywords needs-patch removed

certainly isn't a big issue otherwise I would've raised more noise. Returning false from this particular method is NOT the issue (although it's likely something I could make clearer). It's more that when @wpdb bails on the query because of a failure here, there's no way for client code making the query to know why the bail (unless there's something that's changed since I created this ticket).

_process_field_lengths will return false if values are invalid.
which in turn is called by,
_process_fields which returns false if values are invalid.
which in turn is called by,
_insert_replace_helper will return false if values are invalid.
update will return false (and there's actually two calls to update, so what caused the error?)
delete will return false

The issue is that of consistency and context. Consistency in that other things triggering an error in a query fill the $wpdb->error field, why shouldn't this? Context, in that there is no way for client code to easily determine *what* caused the error without really digging into the code (no WP_Error object, nothing).

#4 @nerrad
2 years ago

FWIW I'll be fine with doing up a patch for this if asked, but I don't want to waste my time :). So if its a wontfix then no patch :)

#5 @ruud@…
2 years ago

Hi @nerrad,

I'm all for consistency, so if there is a strong tendency to have proper wpdb->errors set in other related cases, I would find it logical that it would also create one this case.
If on the otherhand wpdb->errors are some sort of luxury, only the most severe cases use, this may not be the best place to start adding them. I just don't know how the situation is right now.

Who to ask? perhaps opt for dev-feedback? and remove 'close' for the time-being?

Thnx,
Ruud

#6 @nerrad
2 years ago

  • Keywords needs-patch dev-feedback added

#7 @swissspidy
2 years ago

  • Keywords close removed

@nerrad
2 years ago

First pass of context messages for process_field errors.

#8 @nerrad
2 years ago

  • Keywords has-patch added; needs-patch removed

In this first pass at a patch, I kept it very simple - just added an general error message to the last_error property. Future iterations can actually go into the various granular child methods and get more granular with the error (i.e. WHAT field value caused the error) but I think initial pass at least is better than no error message at all.

Last edited 2 years ago by nerrad (previous) (diff)

#9 @ruud@…
2 years ago

Hi @nerrad,

Thnx for the quick patch, great!

I have some ideas/thoughts on the wording and such, I will try to get some free time to interact some more with this ticket. I hope some of the devs can comment on the way wpdb errors are used in other cases.

Thanks,
Ruud

#10 @mnelson4
2 years ago

+1 for this patch. I'm actually surprised this didn't happen earlier

Note: See TracTickets for help on using tickets.