[32307] does not set $wdpb->error

[nerrad]

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.

[swissspidy]

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.

[ruud@…]

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

[nerrad]

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

[nerrad]

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 :)

[ruud@…]

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

[nerrad]

First pass of context messages for process_field errors.

[nerrad]

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.

[ruud@…]

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

[mnelson4]

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

[nlpro]

Duplicate of #32315.