PHP WARNING: "array_merge(): Argument #2 is not an array" in file engine/classes/Elgg/Database/Annotations.php (line 372) #9785

Closed
rohit1290 opened this Issue May 1, 2016 · 9 comments

Comments

Projects
None yet
3 participants
@rohit1290

I noted that the above mentioned PHP warning is appearing in my error log file. I inspected the array_merge() function at line 372 of Annotations.php file and saw that the 2nd argument is $time_wheres and _elgg_get_entity_time_where_sql is returning a "string" to $time_wheres parameter at line 368 of Annotations.php file.

This can be solved by changing line 368 of Annotations.php file from

$time_wheres = _elgg_get_entity_time_where_sql('a', $options['annotation_created_time_upper'], $options['annotation_created_time_lower']);

to

$time_wheres = array(_elgg_get_entity_time_where_sql('a', $options['annotation_created_time_upper'], $options['annotation_created_time_lower']));

I am using Elgg 1.12.x

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 2, 2016

Member

On what page does this error occur? I want to make sure this is not due to incorrect API usage.

Member

mrclay commented May 2, 2016

On what page does this error occur? I want to make sure this is not due to incorrect API usage.

@rohit1290

This comment has been minimized.

Show comment
Hide comment
@rohit1290

rohit1290 May 2, 2016

I am not sure which page is causing this error. I just got the log in my error log file.
How can I know which page is causing this error?

I am not sure which page is causing this error. I just got the log in my error log file.
How can I know which page is causing this error?

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue May 2, 2016

fix(annotations): fixes logic in time-based annotations searches
`_elgg_get_entity_time_where_sql()` returns a string, so if it's not empty
we just push it onto the wheres stack.

Fixes #9785
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 2, 2016

Member

I propose a different fix: #9787 I'm not motivated to write tests for this at this time.

Member

mrclay commented May 2, 2016

I propose a different fix: #9787 I'm not motivated to write tests for this at this time.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 4, 2016

Member

We should not accept any fix without a basic test to exercise it. As no one can locate what it affects, I don't see urgency to rush a fix.

Member

mrclay commented May 4, 2016

We should not accept any fix without a basic test to exercise it. As no one can locate what it affects, I don't see urgency to rush a fix.

@rohit1290

This comment has been minimized.

Show comment
Hide comment
@rohit1290

rohit1290 May 4, 2016

The fix that you proposed is doing the work of array_merge and that too with a string. I have done a basic testing to see the output and found the fix valid. #9787 (comment)

I have even made the correction on my website 2 days ago and have not encountered the above mentioned error till now.

The fix that you proposed is doing the work of array_merge and that too with a string. I have done a basic testing to see the output and found the fix valid. #9787 (comment)

I have even made the correction on my website 2 days ago and have not encountered the above mentioned error till now.

@rohit1290

This comment has been minimized.

Show comment
Hide comment
@iionly

This comment has been minimized.

Show comment
Hide comment
@iionly

iionly May 9, 2016

Contributor

@mrclay , @rohit1290 proposed alternative fix:

        $time_wheres = _elgg_get_entity_time_where_sql('n_table', $options['annotation_created_time_upper'],
            $options['annotation_created_time_lower']);

        if ($time_wheres) {
            $time_wheres = explode(' AND ', $time_wheres);
            $options['wheres'] = array_merge($options['wheres'], $time_wheres);
        }

I don't know how to write tests. I also don't know why the 'a' table is now expected as 'n_table'. I think it only doesn't crash more often at the moment because the $time_wheres might not contain anything in most cases. The function _elgg_get_entity_time_where_sql() calls the function getEntityTimeWhereSql(). In getEntityTimeWhereSql() the where clauses are imploded with ' AND ' and a string is returned. Therefore I think it should be okay to simply explode the string again before merging with the $options['wheres'] array.

Contributor

iionly commented May 9, 2016

@mrclay , @rohit1290 proposed alternative fix:

        $time_wheres = _elgg_get_entity_time_where_sql('n_table', $options['annotation_created_time_upper'],
            $options['annotation_created_time_lower']);

        if ($time_wheres) {
            $time_wheres = explode(' AND ', $time_wheres);
            $options['wheres'] = array_merge($options['wheres'], $time_wheres);
        }

I don't know how to write tests. I also don't know why the 'a' table is now expected as 'n_table'. I think it only doesn't crash more often at the moment because the $time_wheres might not contain anything in most cases. The function _elgg_get_entity_time_where_sql() calls the function getEntityTimeWhereSql(). In getEntityTimeWhereSql() the where clauses are imploded with ' AND ' and a string is returned. Therefore I think it should be okay to simply explode the string again before merging with the $options['wheres'] array.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay May 10, 2016

Member

New fix with test #9807

Member

mrclay commented May 10, 2016

New fix with test #9807

@rohit1290

This comment has been minimized.

Show comment
Hide comment
@rohit1290

rohit1290 May 12, 2016

Fix #9807 is not creating any further issue.
Both the array_merge warning and databaseexception error is solved from this fix.

Fix #9807 is not creating any further issue.
Both the array_merge warning and databaseexception error is solved from this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment