Skip to:
Content

Opened 22 months ago

Closed 17 months ago

Last modified 17 months ago

#6977 closed enhancement (fixed)

Audit all DB fetch methods to return integers where appropriate

Reported by: r-a-y Owned by:
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Core Keywords:
Cc:

Description

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.

Attachments (5)

6977.BP_Friends_Friendship.patch (1.2 KB) - added by r-a-y 22 months ago.
6977.01.patch (11.0 KB) - added by r-a-y 21 months ago.
6977.02.patch (11.3 KB) - added by r-a-y 21 months ago.
6977-bp-activity-activity-01.patch (3.9 KB) - added by DJPaul 21 months ago.
6977.03.patch (38.7 KB) - added by r-a-y 21 months ago.
Work in progress. About 90% done. WIll finish up in the next few days.

Download all attachments as: .zip

Change History (33)

#1 @boonebgorges
22 months ago

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.

#2 @boonebgorges
22 months ago

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.

#3 follow-up: @r-a-y
22 months ago

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.

#4 in reply to: ↑ 3 @boonebgorges
22 months ago

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.

#5 @DJPaul
22 months ago

  • Milestone changed from Awaiting Review to Future Release

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!

#6 @r-a-y
21 months ago

In 10710:

Friends: Cast BP_Friends_Friendship class properties as integers where appropriate.

See #6977.

#7 @r-a-y
21 months ago

  • Keywords has-patch added

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

Last edited 21 months ago by r-a-y (previous) (diff)

@r-a-y
21 months ago

#8 @boonebgorges
21 months ago

// 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
21 months ago

#9 @r-a-y
21 months ago

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

#10 @DJPaul
21 months ago

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.

#11 @r-a-y
21 months ago

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.

Last edited 21 months ago by r-a-y (previous) (diff)

@r-a-y
21 months ago

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

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


21 months ago

#13 @r-a-y
20 months ago

In 10853:

Activity: Cast activity properties as integers where appropriate.

Props DJPaul, r-a-y.

See #6977.

#14 @DJPaul
19 months ago

  • Component changed from Component - Any/All to Core

#15 @r-a-y
19 months ago

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

#16 @r-a-y
19 months ago

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

#17 @DJPaul
18 months ago

  • Milestone changed from Future Release to 2.7

#18 @DJPaul
18 months ago

  • Keywords needs-refresh added; dev-feedback has-patch removed

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

#19 @r-a-y
17 months ago

In 11023:

Blogs: Cast blog properties as integers where appropriate.

See #6977.

#20 @r-a-y
17 months ago

In 11024:

Core: Cast properties as integers where appropriate.

See #6977.

#21 @r-a-y
17 months ago

In 11025:

Friends: Cast properties as integers where appropriate.

See #6977.

#22 @r-a-y
17 months ago

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.

#23 @r-a-y
17 months ago

In 11027:

Groups: Cast properties as integers where appropriate.

See #6977.

#24 @r-a-y
17 months ago

In 11028:

Messages: Cast properties as integers where appropriate.

See #6977.

#25 @r-a-y
17 months ago

In 11029:

Notifications: Cast properties as integers where appropriate.

See #6977.

#26 @r-a-y
17 months ago

In 11030:

XProfile: Cast properties as integers where appropriate.

See #6977.

#27 @r-a-y
17 months ago

  • Keywords needs-refresh removed
  • Resolution set to fixed
  • Status changed from new to closed

Going to close this one!

#28 @r-a-y
17 months ago

In 11065:

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

See #6977.

Note: See TracTickets for help on using tickets.