boonebgorges / buddypress-docs

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

Add filters for when to include folders in loop view/when to restrict folder query. #509

Closed dcavins closed 8 years ago

dcavins commented 8 years ago

Make sure that we account for other plugins that might be providing filtered views of the docs directory using other taxonomies.

See https://github.com/dcavins/buddypress-docs/commit/14158089a3fb6d2ec3a33c86d47e264043f922a3 for one approach.

One question: I introduced some functions to help get the possible list of filters and to determine if any were applied. This could also use bp_docs_get_current_filters() which already exists, but maybe could change (by incorporating the new bp_docs_get_directory_filter_types() function) to make it easier for other plugins to add filters. (As is, plugin authors have to hook in a few different places. We could probably save one hook for most typical uses.)

dcavins commented 8 years ago

Notes on using bp_docs_get_current_filters() vs bp_docs_is_directory_view_filtered() to check whether the directory is being filtered.

Using bp_docs_is_directory_view_filtered() reports the correct status when

It over-reports has-attachment, returning true no matter the value of ?has-attachments= (options are yes, no, empty string).

Using bp_docs_get_current_filters() returns a non-empty array when

But under-reports has-attachment, returning an empty array no matter the value of ?has-attachments= (options are yes, no, empty string).

So maybe it makes sense to use the existing function instead of adding a new one. (Or maybe use bp_docs_get_current_filters() within bp_docs_is_directory_view_filtered(). The new functions bp_docs_get_directory_filter_types() and bp_docs_get_directory_filter_query_args() may still be useful, though.)

boonebgorges commented 8 years ago

If has_attachment is causing problems because of empty() checks, we can just put special logic in place for it inside of bp_docs_is_directory_view_filtered().

If we thought that custom filters were going to be prevalent, we could come up with a proper mechanism for registering filter types, along with "null" values for each. But this is pretty unlikely, so I don't see the point at this juncture. Let's just make sure that the filters are in place.

It's up to you whether you think the bp_docs_get_directory_filter_types() abstraction is worthwhile. For the time being, I think that a filter on bp_docs_is_directory_view_filtered() is probably enough for plugin authors to do what they need to do.

Again, though, I don't feel strongly. If you are doing something in your own plugin that you feel justifies a more centralized approach, you're welcome to implement it in buddypress-docs.

dcavins commented 8 years ago

Ah. Using bp_docs_get_current_filters() internally in bp_docs_is_directory_view_filtered() gets complicated because bp_docs_get_current_filters() returns

array(1) { ["folders"]=> array(1) { [0]=> int(77) } }

when you're in a folder. (It's true that a folder is a filter, so maybe the function name is bad?) Rewriting it without relying on the unnecessary convenience functions looks like:

function bp_docs_is_directory_view_filtered() {
    $is_filtered = false;

    $types = apply_filters( 'bp_docs_filter_types', array() );
    $filters = array();
    if ( is_array( $types ) ) {
        $filters = wp_list_pluck( $types, 'query_arg' );
    }

    $intersection = array_intersect( array_keys( $_GET ), $filters );
    if ( ! empty( $intersection ) ) {
        $is_filtered = true;
    } elseif ( isset( $_GET['has-attachment'] ) && ( 'yes' == $_GET['has-attachment'] || 'no' == $_GET['has-attachment'] ) ) {
        $is_filtered = true;
    }

    return apply_filters( 'bp_docs_is_directory_view_filtered', $is_filtered );
}

and that creates the desired behavior:

Do you have any thoughts on language/function & hook names so that it's clear what's going on? At least now I see the difference between the two methods I thought were similar.

Sorry for all the back and forth on this. It seems harder than it is.

boonebgorges commented 8 years ago

The bp_docs_filter_types filter seems like overkill. We should not introduce a half-baked "API" for registering filter types, because then we'll have to support it indefinitely. Instead, if a plugin wants to announce that its custom filter should count as making the directory "filtered", it can do this:

add_filter( 'bp_docs_is_directory_view_filtered', function( $filtered ) {
    if ( ! $filtered && isset( $_GET['my-custom-filter-key'] ) && 'none' !== $_GET['my-custom-filter-key] ) {
        $filtered = true;
    }
    return $filtered;
} );

Otherwise, we end up making lots of assumptions about the $_GET payload that will only be valid in 70 or 80% of cases.

If we want to make the logic more transparent, especially the has-attachment exception, this might be more appropriate:

$filters = array( 'bpd_tag', 's', 'folder', 'has_attachment' );
$is_filtered = false;
foreach ( $filters as $filter ) {
    if ( 'has_attachment' === $filter && isset( $_GET['has-attachment'] ) && it's 'yes' or 'no' ) {
        $is_filtered = true;
    } elseif ( ! empty( $_GET[ $filter ] ) ) {
        $is_filtered = true;
    }

    if ( $is_filtered ) {
        break;
    }
}

Anyway, I don't care too much about the details of the implementation, as long as it doesn't tie us to too many backward compatibility issues.

dcavins commented 8 years ago

Hi Boone-

I've moved the logic for when a view is filtered out into the various components within BP Docs, so each component can apply whatever rule is appropriate: https://github.com/dcavins/buddypress-docs/commit/73af41011a8bc5f3f3e8b5c5478d6b438ba69704

Please take a look when you have a chance.

Also, please verify that you are good with this logic:

I thought this is what we were aiming for, but I want to be doubly sure.

Thanks for your feedback.

boonebgorges commented 8 years ago

The filter-based approach in the patch looks good to me. The $exclude trick for folder views seems fine.

The logic you've outlined looks correct. Thanks for working on this!

dcavins commented 8 years ago

Thanks for your feedback. I'm going to start pushing to 1.9.x. We can make reverts/changes anytime. I'll also make sure that the folder_id gets added to tag urls when you're in a folder.

dcavins commented 8 years ago

Mischief managed in https://github.com/boonebgorges/buddypress-docs/commit/7ea05fd1f4f5705f80ef0952a4d709711a9d1319