get_entities returns false on invalid subtype

[elpaso66]

When calling with an array of subtypes, if just one of them is not valid (get_subtype_id() returns false) the function will return false as in line 1534 of entities.php

if (!$subtypeval = (int) get_subtype_id($typekey, $subtypeval))

return false;

I believe that skipping the invalid subtype or throwing an exception would be a better behaviour. Silently failing to return any entity can be a difficult error to track.

[brettp]

The reason for the change was two-fold: First, previously if you sent an a single invalid subtype or an array of invalid subtypes it would return all entities--This is completely wrong behavior for this function. Secondly, requiring real subtypes encourages tighter, tidier code in core and in the plugins.

From #1262:
"This is different to 1.6 and results in a serious problem as it should be possible to mention subtypes which are not defined e.g. where a module has been disabled there may be a reference to a subtype which doesn't exist but all other subtypes do false is returned instead of a result set"

1.6 shipped with this version of get_entities(). If you mean 1.5, though, you're right--it's a change.

I'm up for discussion about how to handle invalid subtypes when passed an array. One thing I don't want to do is allow people to get lazy about their code. The more tolerant the core functions are, the greater the chance of bugs being introduced.

[sammykanan]

The modification makes sense, the fact that the function returned everything would be very bad. Regarding being lazy in the code, the problem is that I've got the following code which populates the subtype array...

if ($object_types = get_registered_entity_types ()) {

foreach ( $object_types as $object_type => $subtype_array ) {

if (is_array ( $subtype_array ) && sizeof ( $subtype_array ) && (in_array ( $object_type, $allowedtypes )

$allowedtypes === true))

foreach ( $subtype_array as $object_subtype ) {

if (strpos ( $object_subtype, "news" ) === false) {

echo ". $object_subtype .?";
$typearray [$object_type] [] = $object_subtype;

}

}

}

}

What this does is basically just read the registered types and makes sure that they don't contain any news. This should be fine as it is actually using the methods provided to get the registered types ... my initial expectation was in fact wrong. What is actually happening is that in our test system there are no objects of type object/bookmarks, it is a registered type according to the get_registered_entity_types but when the get_entities method comes to try and resolve this into an id it can't because there is no entry in the subtypes table ...

I assume that the problem previously would only occur if there was a subtype array passed to get_entities and none of the items were found then there was no where statement added and all results were returned.

If that is so what I suggest is the following modifiction:

if (!empty($tempwhere)) $where[] = "({$tempwhere})";

should read:

if ( empty($tempwhere) ) {

return false; I prefer "return array();" but that isn't the pattern

} else {

$where[] = "({$tempwhere})";

}

That doesn't solve the lazy code issue, but it makes the function more sturdy. I'll try it out and update the ticket.

I recommend increasing the priority of this as in the case of the code above which uses only core methods to build the array the results are incorrect and this code could be proliferated throughout someones system.

[sammykanan]

My c2w:

The stanza should read:

....
if (is_array($subtype)) {

$tempwhere = "";
if (sizeof($subtype))
foreach($subtype as $typekey => $subtypearray) {

foreach($subtypearray as $subtypeval) {

$typekey = sanitise_string( $typekey );
if (!empty($subtypeval)) {

$subtypeval = get_subtype_id( $typekey, $subtypeval );

} else {

@todo: Setting subtype to 0 when $subtype = returns entities with
no subtype. This is different to the non-array behavior
but may be required in some cases.
$subtypeval = 0;

}
if ( $subtypeval !== false ) {

if (!empty($tempwhere)) {

$tempwhere .= " or ";

}
$tempwhere .= "(type = '{$typekey}' and subtype=" . (int) $subtypeval . ") ";

}

}

}
if ( empty( $tempwhere ) ) {
There must be some type conditions as a subtype array was provided

return false;

} else {

$where[] = "({$tempwhere})";

}

}
....

[brettp]

As of 1.7 the elgg_get_entities() function returns FALSE on invalid type when using {{{
$options = array(

'type' => 'object',
'subtype' => 'invalid_subtype'

);
}}}
But it returns all entities when using {{{
$options = array(

'type_subtype_pairs' => array('object' => 'invalid_subtype')

);
}}}

This needs to be standardized before 1.7 beta is released. I opt to make it return false on invalid subtypes.

[cash]

Brett, are you saying return false for the case when there is only one pair of type, subtype and it is invalid or if any pair is invalid out of many pairs?

If false is returned with the case that one of out many is invalid, that means there will be code that will not work until the first entity is created for a type/subtype pair. For example, I think the Latest Activity content on the default front page might stop working every time a new plugin is enabled that registers a new subtype until a new entity is created. (Or maybe this is just a symptom of a problem with how Elgg handles subtypes...)

[dr4g]

I think another issue here is that it has to perform the computation to generate the query, and then run it and get a COUNT(*).
This means that list_entities() has to call it twice..

If someone comes up with a solution that allows us to make one call to get_entities() and get the count of that result.
I'd personally do.

$entities = get_entities($type, $subtype, $owner_guid, "", $limit, $offset);
$count = count($entities);

instead of

$count = get_entities($type, $subtype, $owner_guid, "", $limit, $offset, true);
$entities = get_entities($type, $subtype, $owner_guid, "", $limit, $offset);

But as i'm the new kid on the block theres probably a good reason you're making two calls to his from list_entities().

Let me know this seems like a juicy core piece of code i can get my teeth into.

Cheers.
Paul Dragoonis.

[dr4g]

formatting wierdnes.

$entities = get_entities($type, $subtype, $owner_guid, "", $limit, $offset);

$count = count($entities);

instead of

$count = get_entities($type, $subtype, $owner_guid, "", $limit, $offset, true);

$entities = get_entities($type, $subtype, $owner_guid, "", $limit, $offset);

[cash]

Paul - the Trac formatting gets me, too. If you following the WikiFormatting link above the text box, it shows how to insert code like this:

$entities = get_entities($type, $subtype, $owner_guid, "", $limit, $offset);
$count = count($entities);

Also, did you change the milestone from 1.7 to 1.8 or was that a Trac oddity?

[brettp]

(In [svn:3901]) Refs #1200: Changed logic in elgg_get_entity_type_subtype_where_sql() to return FALSE if there are no valid subtypes passed. Ignores all invalid subtypes. Added (partial) tests for elgg_get_entities() types and subtypes.

[brettp]

The logic now only returns FALSE if all the types or subtypes passed are invalid. I had lost 75% of this work in a hard drive crash but believe I have duplicated it, though I don't want to close this ticket until a few others have tested. The unit test was most of what I lost so I'm hesitant to trust it completely.

• FALSE is returned if:
  • All types passed are invalid
  • All subtypes passed are invalid for all types passed (passing types and subtypes separately or using type_subtype_pairs).
• Entities are returned if:
  • No type/subtypes are passed.
  • At least one valid type is passed.
  • At least one valid type and one valid subtype for that type are passed (using separate type and subtypes).
  • At least one valid type and one valid subtype for that type are passed (using type_subtype_pairs).

[brettp]

(In [svn:3914]) Refs #1200: Added tests to test for false being returned with elgg_get_entities(). Disabled execution time during unit tests.

[brettp]

I am reasonably certain these functions now operate as expected. Closing this, but feel free to re-open for any unexpected behavior (ie, all entities are returned if any restricting options are passed, false is returned on valid types/subtypes).