default/fallback grid not in allowed_grids

[kraftner]

In case you filter gc_allowed_grids does not contain 4 the code still falls back to 4 anyway if the selected grid is not allowed, for example when using 4

I see two ways to solve this:

1. Do not directly use $this->allowed_grids but instead use $defaults['grid']
2. Add a new filter gc_fallback_grid or alike.

I can create a PR, I just wanted to ask beforehand what you're preferred approach would be on this.

[kraftner]

You could work around this, but I feel it is cleaner if this is handled internally, anyway;

add_filter('gc_allowed_grids', function( $allowed_grids ) {
    return array( 2, 3 );
});

add_filter('gc_column_args', function($attr){
    if( ! in_array( $attr['grid'], array( 2, 3 ))){
        $attr['grid'] = 3;
    }
    return $attr;
});

[justintadlock]

I'm thinking a filter hook on the default is the best route.

[kraftner]

Okay I thought this through some more and I think I need to explain this in more detail.

The current behaviour just sticks with the last valid grid if an invalid one is found until a new valid one is used. Let's look at an example:

$allowed_grids = array( 2, 3 );
[column grid="2"]...[/column]
[column grid="4"]...[/column]

This actually results in two grid="2" columns.

If this is intended behaviour (or even if not, because changing this would probably cause issues with existing sites) we can not do something like this at L182

if( $this->is_first_column ){
    if( in_array( $attr['grid'], $this->allowed_grids ) ){
        $this->grid = absint( $attr['grid'] );
    }else{
        $this->grid = absint( $defaults['grid'] );
    }
 }

Actually we only need to check on the very first usage of the shortcode because from then on $grid will always have a valid value according to the filtered $allowed_grids.

So I'd just add a filter in init():

$this->grid = absint(apply_filters( 'gc_default_grid', $this->grid ));

But the problem is that we already have a filter gc_column_defaults containing grid which people will reasonably expect to be considered for the fallback.

So we probably need to use this hook in init, something like this:

/* Set up the default arguments. */
$defaults = apply_filters(
    'gc_column_defaults',
    array(
        'grid'  => $this->grid,
        'span'  => 1,
    'push'  => 0,
    'class' => ''
)

$this->grid = absint($defaults['grid']);

This still has the slight problem that people can now manually set a default grid that is incompatible with $allowed_grids but at least then it is the users fault. Also this isn't really elegant to have this filter applied at two places. This might cause issues with people who do crazy stuff that depends on the order of the filters.

I'll keep looking for a backward compatible and clean solution to this. Maybe you have an idea too.

[kraftner]

Okay another idea. What if we add this:

if ( ! in_array( $this->grid, $this->allowed_grids ) ) {
   $this->grid = $defaults['grid'];
}

right after the line that sets the grid

No need for a new filter and this only kicks in when we end up with and invalid grid. I think that is it. What do you think? Ready for a PR?

[kraftner]

As I am hitting this again on a current project - any thoughts?

[lkraav]

@kraftner what's your real world use case for this happening? People entering random grid values, not knowing the restrictions, or?

[kraftner]

Well by default you can choose between 2, 3, 4, 5 or 12 columns.

As this doesn't always make sense, say my theme only works and therefore accepts 2 and 3 column layouts. If I filter gc_allowed_grids and then someone enters a non-valid grid it falls back to 4 which isn't allowed either.

[lkraav]

Ah right, you've already upvoted #7 :) No further comments needed then.