Shortcode improvements

[hvianna]

This is something else I've been working on this week :) and implements basic funcionality for selecting and excluding categories from the directory listing via shortcode, using the terms and exclude attributes. Both attributes are space-separated lists of category slugs.

The terms implementation is pretty straightforward and uses the slug parameter of get_terms(). For the exclusion, I also added rules to the tax_query parameter of get_posts(), so sites with multiple categories get excluded from the list too.

[fabacab]

Hmm, I think maybe there's some confusion happening here.

My understanding of what you want this patch to do is to make it so users can write the [site-directory] shortcode with attributes that allow them to tweak which Directory Entry posts are shown. Since this is already something that the underlying functions can do (i.e., the $args argument to the underlying functions such as get_posts() and get_terms() and so on), the easiest way to do this is to pass these arguments directly down as arguments to those underlying functions where the shortcode's logic uses them.

The existing implementation of, for instance, get_sites_in_directory_by_term() provide a limited subset of those capabilities for ease of theme authoring, such as by defining a specific term. But the shortcode is not a theme-level thing. That is, theme authors don't interact with it, so it shouldn't need to be constrained by this limitation.

Therefore, if you're trying to make the shortcode more functional, why rely on the theme functions where they don't fit your purpose?

[fabacab]

@hvianna So, have a look at 42e22b8b22e8242d9a7d8247573b71cbfff02c19 please. Can you tell me if this commit fails to provide the functionality you're hoping to add?

In that commit, the $args parameter for the underlying functions is directly passed into those underlying WordPress core functions to retrieve posts or terms from the shortcode. The interface for doing this from the shortcode is the already-existing query_args attribute. This means that you can exclude Directory Entry posts with a shortcode like this, for instance:

[site-directory query_args='{"exclude": 2}']

In this case, you are excluding from the shortcode's map display any Directory Entry whose associated Site is categorized with term_id equal to 2.

All of the other tax_query parameters are supported as well, so you could do something like the following to display a list of all Directory Entries categorized with with a term whose slug is equal to foo or bar:

[site-directory display="list" query_args='{"slug":"foo,bar"}']

I realize this isn't the most intuitive shortcode value, but the benefit is that this mechanism transparently upgrades itself whenever WordPress core adds support for new arguments in the underlying functions.

[hvianna]

Hello! Thank you for elaborating on your previous reply and for providing example code.

The issues I see with this implementation:

• The exclude doesn't work for sites assigned multiple categories (unless all of them are in the exclude list, of course);
• It doesn't solve the blogname sorting;
• The JSON-encoded array inside the shortcode looks really clumsy. I'd even say it smells bad :P

But since you were nice enough to provide conditional functions in your code, I can always tweak what I need in a separate plugin, so I'm closing this PR.

Cheers!

[fabacab]

The exclude doesn't work for sites assigned multiple categories (unless all of them are in the exclude list, of course)

That's true, but it's because that's the way the exclude parameter in the WordPress core function works. This is the expected behavior. That said, if what you're trying to do is create an easy interface for a shortcode author to use a tax_query with the NOT IN operator like in 778169b3be7d264d03b96b67427e4cc1efea8502, then the same thing I said before regarding passing the parameters down into the underlying core functions is the better route to take.

Here's what I mean. With a minor change to the parseJsonAttribute method (shown in 0f4348d089b5ea4cdaec642b2304fb28c280f5a2), we can have full support for arbitrary WP_Query-like queries. This lets us write a query_args value like the following in order to get the functionality you're looking for, where we can exclude specific entries that have any of a given term rather than having to use the simpler exclude element of the $args argument I showed earlier:

[site-directory display="list" query_args='{"tax_query":%5B{"taxonomy":"subsite_category","field":"slug","terms":"EXAMPLECATEGORY","operator":"NOT IN"}%5D}']

Of course, there's some ugliness here:

1. We have to explicitly re-declare the subsite_category taxonomy (although we wouldn't if we used term_taxonomy_id instead of slug in the field key's value).
2. We have to pass URL-encoded values for [ and ] (by using %5B and %5D, respectively) in order to pass an explicit array without confusing the WordPress shortcode parser.

But those ugly characteristics can be very easily mitigated by handling the ugliness in a shortcode attribute, rather than by rewriting the implementation of the core functions. So, for example, let's make it possible for a shortcode author to write the same thing as shown above, but with a simpler shortcode:

[site-directory display="list" site_category__not_in="EXAMPLECATEGORY"]

The best way to implement an attribute like this is to write code that simply produces the same data structure in the query_args parameter (which now already supports what we need and a whole lot more) after being parsed. The code for this is shown in 090b94560d5e4525cdb395aeaeba6c4b07c649be. For reference, compare that commit with the alternative implementation suggested in 778169b3be7d264d03b96b67427e4cc1efea8502, which changes one branch of the prepare() method and thus only applies to some invocations of the shortcode, instead of the constructor, which applies uniformly across all invocations.

This is what I mean by suggesting that the shortcode attributes pass their values directly to arguments in the underlying core functions. This way, we simultaneously introduce more functionality to the shortcode, using less code, and save ourselves the trouble of having to re-write core implementations whenever they get updated or change. This is why it's so much more powerful to write our code to the interface rather than to copy an implementation.

It doesn't solve the blogname sorting;

True, I didn't even look at that, but the same pattern from above can be applied. But even so, I think that the sorting should be based on the Directory Entry post's name, not the underlying site's real name, because what we're outputting is the directory, not a list of the existing sites. We had a similar discussion about this when you suggested automatically redirecting to the site in question instead of viewing the site's directory entry page. (See #21.)

I think a better solution than re-sorting the user's directory behind their back is to implement an option that would automatically update the Directory Entry post's title whenever a site admin updates the title of their site.

The JSON-encoded array inside the shortcode looks really clumsy. I'd even say it smells bad :P

This final criticism is legitimate, but again, I think the better way to deal with this is to provide a convenience wrapper for an interface built on improved underlying functionality, rather than cherry-picking specific functions to rewrite at the shortcode level.

Anyway, I hope this clarifies what my original concerns were and showcases an alternative path that's useful to you. :) As always, thanks again for your time on this.

I'll merge my own alternate implementation described here into develop today so you can build on it if you prefer and so it eventually becomes available to users in future versions of the plugin.

[hvianna]

The best way to implement an attribute like this is to write code that simply produces the same data structure in the query_args parameter (which now already supports what we need and a whole lot more) after being parsed. The code for this is shown in 090b945. For reference, compare that commit with the alternative implementation suggested in 778169b, which changes one branch of the prepare() method and thus only applies to some invocations of the shortcode, instead of the constructor, which applies uniformly across all invocations.

I see now, thanks for the comprehensive explanation. I would suggest similar wrappers for the slug, orderby and order parameters, so the user won't need to resort to query_args for doing simple things.

I think a better solution than re-sorting the user's directory behind their back is to implement an option that would automatically update the Directory Entry post's title whenever a site admin updates the title of their site.

You're right, that's the real solution. Maybe add this to the to-do list? :)

[fabacab]

I would suggest similar wrappers for the slug, orderby and order parameters, so the user won't need to resort to query_args for doing simple things.

The counter argument, of course, is that simple things are simpler to do using query_args in the first place, so may not warrant such a convenience wrapper. Compare the custom tax_query with the simpler slug argument:

query_args='{"slug":"foo,bar"}'

Clearly, this is not as difficult to read as the tax_query one. Once again, it's not the prettiest thing in the world, but a convenience attribute means we'd need more PHP code and more code means more opportunity for bugs, more cognitive load on us as maintainers, more likelihood of needing to change it later if WP core changes, and so on. The cost/benefit in this case of "simpler" WP_Query arguments is not as clearly weighted towards a convenience wrapper as the other case.

The real problem you're pointing out—and it is a real problem, so thanks for pointing it out—is that to successfully use this shortcode's query_args attribute, a blog author must be familiar with JSON syntax. That's an unreasonable expectation to have of content editors. But that is a general problem that can be said to be true regardless of what this plugin or shortcode does. So instead of writing a specific solution by adding convenience attributes for each individual possible argument that query_args can now pass down to its underlying WP core functions, let's write a better interface for blog authors to use when composing the [site-directory] shortcode, an interface that accepts shortcode attributes and values and outputs those values in JSON syntax. This way, we are writing code that directly addresses the problem (the interface to query_args should not require knowledge of JSON to use) instead of writing code that works around the problem ("just use this other, more specific attribute instead").

WordPress has two mechanisms we can use for this: QuickTags for when the author is writing post or page content in HTML mode, and TinyMCE Custom Buttons for when the author is writing post or page content in Visual mode.

This is another task that should probably become its own PR or issue.

Maybe add this to the to-do list? :)

I should, and maybe I will, but that's another "thing to do." :)

Meanwhile, ¯\_(ツ)_/¯