wp-shortcake / shortcake-bakery

A fine selection of shortcodes for WordPress
42 stars 16 forks source link

Add a filter for Facebook URL regex validation #191

Closed montchr closed 6 years ago

montchr commented 8 years ago

Few of the other shortcode classes use this sort of regex URL validation in the callback methods. These patterns may change unpredictably at any time.

goldenapples commented 8 years ago

I think there was a reason why we had this explicit post URL structure validation in place, so that we could give our editors a heads-up when trying to embed a structure that wasn't supported yet.

But this works in our newsroom because we have (or had) a quick feedback loop, where we'll hear in a few minutes that an editor is trying to post something that doesn't work through the existing post element, we can test it out and see if it'd work, and either update the validation rules or find an alternative way of embedding that post. When there's a post format that doesn't work with embedding, as happened recently with the 360 video posts that Facebook introduced, it was useful to have an error message on our end that we don't support that kind of embed rather than let the post element be inserted and then have it fail on the front end.

But this plugin is less-actively updated now than it was when we introduced this validation, and without being able to be quickly responsive to new formats, it's not that helpful of a check.

I'm wondering if it makes more sense to add a filter there, so that sites can override the validation if they find new acceptable patterns that aren't included in the plugin's validation rules, or just to remove the validation rules altogether. Anyone else have opinions one way or the other on this?

montchr commented 8 years ago

I'm wondering if it makes more sense to add a filter there, so that sites can override the validation if they find new acceptable patterns that aren't included in the plugin's validation rules, or just to remove the validation rules altogether.

That solution sounds like a good compromise. I can update my PR if you like, or open a new one.

davisshaver commented 7 years ago

Filter/patch loop seems fine. @montchr if you have any examples of bad structures we can get those added.

montchr commented 7 years ago

@goldenapples I've reverted my first commit and instead added a filter.

if you have any examples of bad structures we can get those added.

@davisshaver I don't recall which pattern was giving us trouble, so let's just say it's all good. My approach to using this with a filter:

add_filter( 'shortcake_bakery_facebook_url_patterns', function( $patterns ) {
    return array_merge( $patterns, [
        '#https?:\/\/www?\.facebook\.com\/?#',
    ] );
} );

So if some Facebook URL doesn't work with that — a public event, for example — I get the following when rendered:

montchr commented 7 years ago

It'd be nice to have phpdoc comments for all the filters in the plugin. Would it make sense to extract this list of whitelisted regexes - and the filter - into a separate method the way that it's done on the script and iframe shortcodes?

Good call, I can do that.

montchr commented 7 years ago

@goldenapples updated