Audit all DB fetch methods to return integers where appropriate

[r-a-y]

When we populate an item from the database, sometimes the incorrect data type is returned. For example, integers are returned as strings.

As a result, some functions that rely on strict type comparison can fail. #6969 is an example of this.

For 2.6, it might be a good idea to audit all our DB fetch methods so numeric strings are cast to integers.

Attached patch is an example that fixes up the BP_Friends_Friendship class to do this.

Let me know what you think and whether you see any issues with this approach.

[boonebgorges]

Approach looks fine to me. A couple general notes -

• Where possible, we should enforce on the way out in addition to when we set the value. (that is, where we have getters)
• We should catalog all changes - maybe in this ticket - so that we can publicize the backward compatibility breaks.

[boonebgorges]

Also:

• We should be cognizant of null vs 0 vs '0' as much as possible. 6977.BP_Friends_Friendship.patch​ will mean that a failed lookup results in a BP_Friends_Friendship object where, eg, initiator_user_id is null. I think this is good - better than 0, which would be misleading - but either way, we should try to be consistent.

[r-a-y]

Thanks for the feedback, @boonebgorges!

6977.BP_Friends_Friendship.patch​ will mean that a failed lookup results in a BP_Friends_Friendship object where, eg, initiator_user_id is null.

How are you checking for a failed lookup? $f = new BP_Friends_Friendship( 0 )?

If so, $f->initiator_user_id will always return null with or without the patch.

[boonebgorges]

How are you checking for a failed lookup? $f = new BP_Friends_Friendship( 0 )?

If so, $f->initiator_user_id will always return null with or without the patch.

Yeah, or $f = new BP_Friends_Friendship( $foo ) for some $foo corresponding to a non-existent friendship. In this specific case, because the get_row() query will return nothing, fields like initiator_user_id never get set. So, as you note, it's null both before and after the patch. I think that's the correct behavior. My point was that we should be sensitive to that as we continue to patch these things - don't go casting empty results to 0 unless 0 is the semantically correct value.

[DJPaul]

I fear this is going to turn into one of those "let's do nothing because this involves changing everything" tickets, but it has my support!

[r-a-y]

In 10710:

Friends: Cast BP_Friends_Friendship class properties as integers where appropriate.

See #6977.

[r-a-y]

01.patch should handle the rest of the components. Hopefully, I didn't miss anything!

[boonebgorges]

// Omitting 'group_id' due to unit test failures. 
// Omitting 'parent_id' for safety. 

This set off warning bells for me. If casting group_id to an int makes unit tests fail, that doesn't mean we should skip it, it means we should fix the unit tests. If we had 100% coverage for return types, each one of the changes proposed in this patch would cause a unit test to fail. This demonstrates how the change we are making has the potential to break backward compatibility in some cases. I still think we can go ahead with the changes, but this is the reason why, after it's done, we need to put a post on bpdevel - there are probably plugins that will fail in the same way that our unit tests are.

What does "omitting 'parent_id' for safety" mean? It sounds scary :)

[r-a-y]

If casting group_id to an int makes unit tests fail, that doesn't mean we should skip it, it means we should fix the unit tests.

When I uploaded 01.patch, it was late in the day and I couldn't for the life of me figure out whereabouts the xprofile unit tests were failing :)

02.patch fixes this up. I forgot to cast some other properties in BP_XProfile_Group::get().

What does "omitting 'parent_id' for safety" mean? It sounds scary :)

Disregard my idiocy :)

[DJPaul]

Wait, wait? I've just looked at this properly. Why the heck are we casting strings to integers EVERYWHERE instead of just fixing the relevant properties when we fetch stuff from the database? I think the work done so far, while well-intentioned, is the wrong approach.

For an example, see the new patch I've put up for the BP_Activity_Activity which typecasts integer properties after we fetch results from the database cache (done it this way instead of before being cached because otherwise we have to force a cache flush after upgrade, but that's a trivial change to make if we wanted to do it that way.

[r-a-y]

Why the heck are we casting strings to integers EVERYWHERE instead of just fixing the relevant properties when we fetch stuff from the database?

That's exactly what I'm doing. Please take a closer look at my patch.

Edit - I see a spot in the BP_Messages_Thread and BP_XProfile_Group class where I can do the casting better. Will fix this up. Thanks for looking into the patch!

For an example, see the new patch I've put up for the BP_Activity_Activity which typecasts integer properties after we fetch results from the database cache

I missed the BP_Activity_Activity class in my patch so thanks for adding a patch for that.

[r-a-y]

Work in progress. About 90% done. WIll finish up in the next few days.

[r-a-y]

In 10853:

Activity: Cast activity properties as integers where appropriate.

Props DJPaul, r-a-y.

See #6977.

[r-a-y]

In 10907:

Activity: Fix issue with activity permalink access when the legacy activity query is enabled.

In #7113, we fixed a bug where anybody's activity item could be displayed
under a member profile. The fix (r10880) does a strict type check on
$activity->user_id.

We expect $activity->user_id to be an integer, however it can be a
numeric string if the legacy activity query is enabled with:
add_filter( 'bp_use_legacy_activity_query', '__return_true' );

Since the check will always fail in this instance, members will not be able
to access activity permalinks.

We unfortunately missed this specific instance in r10853.

This commit addresses this problem.

See #6977. Fixes #7146 (2.6-branch).

[r-a-y]

In 10908:

Activity: Fix issue with activity permalink access when the legacy activity query is enabled.

In #7113, we fixed a bug where anybody's activity item could be displayed
under a member profile. The fix (r10880) does a strict type check on
$activity->user_id.

We expect $activity->user_id to be an integer, however it can be a
numeric string if the legacy activity query is enabled with:
add_filter( 'bp_use_legacy_activity_query', '__return_true' );

Since the check will always fail in this instance, members will not be able
to access activity permalinks.

We unfortunately missed this specific instance in r10853.

This commit addresses this problem.

See #6977. Fixes #7146 (trunk).

[DJPaul]

See related effort on #7155 including an outstanding issue involving clear object cache.

[r-a-y]

In 11023:

Blogs: Cast blog properties as integers where appropriate.

See #6977.

[r-a-y]

In 11024:

Core: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

In 11025:

Friends: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

In 11026:

In BP_Core_User::get_last_activity(), check if array key is set before integer casting.

Prevent PHP notices, which broke unit tests as of r11024. Boo Ray!

See #6977.

[r-a-y]

In 11027:

Groups: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

In 11028:

Messages: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

In 11029:

Notifications: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

In 11030:

XProfile: Cast properties as integers where appropriate.

See #6977.

[r-a-y]

Going to close this one!

[r-a-y]

In 11065:

Groups: Cast properties as integers in BP_Groups_Group::get().

See #6977.