boonebgorges / buddypress-docs

GNU General Public License v3.0
106 stars 44 forks source link

Had an issue where empty $cached array was returned. this fixed it. #617

Closed elpeyotl closed 5 years ago

elpeyotl commented 6 years ago

Docs attachment were not showing in some cases. Investigated it and realized, that event empty $cached arrays were returned. This is now working for me.

boonebgorges commented 6 years ago

If an empty array is cached, it should mean that the doc has no attachments. With this PR, the fact that a doc has no attachments will never be cached, which doesn't seem ideal.

There must be a bug somewhere else that's causing the cache not to be busted in cases where a doc goes from having no attachments to having attachments. I think we should fix this problem there instead.

@elpeyotl Did you notice any pattern to the kinds of situations where attachments weren't showing?

elpeyotl commented 6 years ago

Wow! Fast reply. I realized that when I deleted the attachment and attached it again, the attachments where showing up. So as you said, somewhere it is missing to updated the cache when updating the doc. Well my bp is in production and client updated docs. some are showing some not. I need to dig deeper to find out how he was able to insert an attachment without triggering the cache.

But you are right. this request does not make sense then. The problem is elsewhere.

elpeyotl commented 6 years ago

Ah the attachments are showing up on the BP-docs detail page. But sometimes not on the "Created docs by me" overview

elpeyotl commented 6 years ago

I got more information on this case. When I look at the bp folders in a group overview I see all attachments. If i select to a user and want so see all files which were created by the user, some are not showing up. But the entries have an attachment. You can see, I echo '1' if $chached !== false. It is going into there but no attachments showing up.

If i tried my (count($cached)) fix the opposite is happening. All attachments on the directory overview disappar but on the user list the show up.

See screenshots.

bildschirmfoto 2018-03-22 um 12 11 13 bildschirmfoto 2018-03-22 um 12 11 34
elpeyotl commented 6 years ago

This is the code:

` function bp_docs_get_doc_attachments( $doc_id = null ) {

$cache_key = 'bp_docs_attachments:' . $doc_id;
$cached = wp_cache_get( $cache_key, 'bp_docs_nonpersistent' );
if ( false !== $cached ) {
    echo 1;
    return $cached;
}

`

elpeyotl commented 6 years ago

So why are "some" attachments not showing on the users overview page? I think there is the problem.

elpeyotl commented 6 years ago

This seems to solve the problem on both views: Group Folder overview and User detail overview.

$cached = array_filter($cached); if (!empty($cached)) { return $cached; }

What u think?

boonebgorges commented 6 years ago

Thanks for sharing the additional detail.

It seems relevant that you're using the folders feature in groups. The way this feature is built means that the bp_docs_get_doc_attachments() function is called multiple times on a pageload, including the AJAX queries that are used to fetch a folder's contents. In my tests, it's being called three times per Doc. The first one hits the database, while the second and third do not. I have a feeling that something may be happening at this point that's triggering the problem.

cc @dcavins - since you worked on this AJAX loading, maybe something will jump out to you.

@elpeyotl It's interesting to me that array_filter() is fixing the problem for you. What does $cached look like before this happens? Do you have an array that has empty objects in it? eg array( array() ) or array( 0 )?

Also, are you running a persistent caching backend like Redis or Memcached? This cache group should be non-persistent, but perhaps something is preventing the items from being inserted into the memory store.

elpeyotl commented 6 years ago

@boonebgorges Thats the funny thing. When I use the old code and print_r $cached i receive all arrays on the folder view in a group.

On the users view (created document by user) there are some attachments missing which you see as empty arrays. "array ()".

see screenshots.

bildschirmfoto 2018-03-23 um 11 38 30 bildschirmfoto 2018-03-23 um 11 38 56
elpeyotl commented 6 years ago

As you see the entry "Adressliste Katecheten und Katechetinnen". On folder overview it shows array including file. On users overview array is showing but empty.

boonebgorges commented 6 years ago

If that's what $cached looks like before you run it through array_filter(), then I'm unsure how array_fliter() would fix anything. Are you sure you're showing the variable at the right place?

elpeyotl commented 6 years ago

Here you see it. clearly. It's strange because for me $cached looks the same before entering the !$cached statement.

bildschirmfoto 2018-03-23 um 14 29 45 bildschirmfoto 2018-03-23 um 14 30 16 bildschirmfoto 2018-03-23 um 14 30 35
boonebgorges commented 6 years ago

I am baffled :) If $cached is an array of post objects, and this is the check currently in Docs:

if ( false !== $cached ) {
    return $cached;
}

then how could an empty array be returned?

Is it possible that the empty array is coming from the next block, which looks like this?

if ( is_null( $doc_id ) ) {
    $doc = get_post();
    if ( ! empty( $doc->ID ) ) {
        $doc_id = $doc->ID;
    }
}

if ( empty( $doc_id ) ) {
    return array();
}
elpeyotl commented 6 years ago

I dont think so that its coming from the code below. Even if I have an empty array it goes to the first if statement (!= false). Look at my screenshot. Outputting on every if an echo. but only the first gets called. Even if array is empty it is returning with false !== $cached. Well my fix is working but don't know if thats the right behaviour.

bildschirmfoto 2018-03-23 um 15 10 26 bildschirmfoto 2018-03-23 um 15 10 54
boonebgorges commented 6 years ago

Thanks for bearing with me. I am starting to see a little bit more clearly.

Your "fix" works simply by skipping caching. This is not a viable fix for Docs.

The problem in your case must be that an empty array is being put into the cache at this line: https://github.com/boonebgorges/buddypress-docs/blob/48ea143a3ca6cee2cba04fd76f0ba70fa3b213c3/includes/templatetags.php#L2122

When a Doc has attachments, that $atts variable should not be empty. So either you have something filtering 'bp_docs_get_doc_attachments', or there's some other plugin interfering with WP_Query on your system that's causing items to be excluded improperly - perhaps a pre_get_posts callback. The next step in your debugging should be to figure out why $atts is empty.

The first place you might want to look, since it's part of Docs, is the privacy-protection code. Try commenting this out to see if it affects the issue: https://github.com/boonebgorges/buddypress-docs/blob/48ea143a3ca6cee2cba04fd76f0ba70fa3b213c3/includes/access-query.php#L361 If so, this could be a bug in Docs.

If not, start looking through your codebase for other pre_get_posts callbacks, and other things that affect the way that get_posts() and WP_Query work.

elpeyotl commented 6 years ago

Thank you for your explanation. Will dig deeper and get back to you as soon I have time...

nalonsopress commented 5 years ago

FYI, this was fixed in later versions of buddypress-docs on this line:

https://github.com/boonebgorges/buddypress-docs/blob/48ea143a3ca6cee2cba04fd76f0ba70fa3b213c3/includes/templatetags.php#L153

boonebgorges commented 5 years ago

Thanks, @nalonsopress ! See https://github.com/boonebgorges/buddypress-docs/commit/e55356fab380d872cd253f06860078bbc5ce5ad3