Revise enqueue style locations for theme compat

[hnla]

As we can create '/community', '/buddypress' folders in theme root as well as custom primary template file in those folders root e.g 'buddypress.php' it makes sense that we also can move the folder'/css/buddypress.css' into this position as well, rather than have to have it live in theme root seeming a little orphaned?

Attached patch re-writes the file_exists checks to include the two possible alternative locations for stylesheets and attempts to condense code lines using is_child_theme() checks for switching stylesheet/template_directory paths.

This patch doesn't take into account the js enqueueing, which is perhaps lees of a concern for end user to be moving about if they should be at all, but this can be matched to stylesheet block if proposal thought acceptable.

[hnla]

re-factor file_exist stylesheet checks

[boonebgorges]

Moving to 1.8 for review.

[boonebgorges]

I like the idea of this ticket very much - we should be allowing the same fallbacks for assets like JS and CSS that we do for PHP template files. But I think we need to put a bit more thought into it than we have for 1.8.

Since 1.7, BP has used the notion of nested "template stacks" when looking for the appropriate template to serve. BP (and, optionally, other plugins) register certain potential template locations (such as child themes, parent themes, and bp-legacy) using bp_register_template_stack, and then in bp_locate_template() we loop through the registered locations to find the first matched template. This is a slick system, and is quite flexible.

However, in order to do the very same logic for assets like JS and CSS files, we need to have two different template stacks: one for file paths (already in place for templates) and one for URIs. I've attached a very, very rough patch to see what that second stack could look like. But my patch is incomplete and won't work, because you need to have matched pairs of dir paths + URI paths in order to do the right kinds of checks. Something like:

foreach ( $template_locations as $template_location ) {
    if ( file_exists( $template_location['dir'] ) . $filename ) {
        $location = $template_location['url'];
        $handle = $template_location['asset-handle'];
    }
}

So, we would need another layer of abstraction for registering template locations that includes the filesystem path *and* the URI path *and* some identifier for what kind of stack it is (parent theme, child theme, etc).

I'm going to put this ticket in the 1.9 milestone so we can review possible solutions early on in that dev cycle.

[hnla]

Agreed, as described the template_stacks is a slick system and tying assets into it a great idea.

Given we agree in the principle outcome i.e assets located in a given location and that in theory the only issue is how that is achieved and that the outcome will be an unchanging factor - we know the possible locations - could we not put in place an interim solution that achieves the outcome but that has the method improved at a second pass?

We are using the approach in the patch currently, and essentially all I have done is take that same approach and code and extend it to be a little more flexible in allowing us to re-locate these assets.

[boonebgorges]

could we not put in place an interim solution that achieves the outcome but that has the method improved at a second pass

Yeah, this sounds reasonable to me. My biggest concern is that we don't want to put in any limited logic that might be used by a plugin or theme dev during the 1.8.x cycle, thus harming our ability to tear out the interim solution later on. But the architecture of theme compat means that I can just make it a private method, and we shouldn't have any problems. See 4949.02.patch.

I did have to change your logic a little bit. Your is_child_theme() check means that the parent theme is not consulted if the child theme doesn't have the assets. This is not how theme compat works for templates - we first check the child, then check the parent, not one or the other.

The only real limitation of 4949.02.patch, other than the fact that it duplicates some of the _template_stack() stuff elsewhere in BP and thus is a bit inelegant, is this. The template_stack() architecture allows third parties to register their own template locations. For example, I do this in CollabPress and BuddyPress Docs so that I can ship template with the plugins that can be overridden easily in a theme. Those custom locations will *not* carry over to CSS/JS assets - the checks in 4949.02.patch are hardcoded. This can't be worked around at the moment. That said, this is a real edge case that I doubt anyone would ever have noticed if I hadn't just written it down :)

I'm going to ask johnjamesjacoby for feedback on this before committing it. I don't expect him to love it, but I think he'll agree that (a) the goal is a very good one, and (b) 4949.02.patch is a good-enough interim solution, which can easily be rethought in BP 1.9 or whenever. hnla, can I ask you to test the patch to make sure it's acting the way you'd expect?

[hnla]

I did have to change your logic a little bit. Your is_child_theme() check means that the parent theme is not consulted if the child theme doesn't have the assets. This is not how theme compat works for templates - we first check the child, then check the parent, not one or the other.

I thought my use in this context was valid my tests returned my expected results, if is_child_theme() returned false I used get_template_directory() to check the parent path.

I'll run your patch, test & report back.

[hnla]

Removed comments re pita issues between git/svn patches/diff as managed to run merge on git diff.

[hnla]

correct malformed uri - add trailingslashit()

[hnla]

Updated patch just corrects the trailing slashed for directory structure.

Not convinced of this patch validity even though I re-applied .02 to a clean copy of functions file from trunk (lord knows why when I only changed two lines the whole block shows as being added/replaced - green)
Changes simply:

$retval['location'] = trailingslashit( $location['uri'] ) . trailingslashit( $subdir ) . $file;

Running tests on revised 02 patch look fine, file located correctly in parent or child themes in root level /my-theme/css/ or in /my-theme/community/css/ complete removal and we correctly fall back to plugin bp-legacy dir.

Only glitch I could find was that the 'handle' is not correctly able to check if we are running in a child theme or simply in a parent or theme not set as a child, we always show 'bp-child-css-css'. I hacked things around a bit but couldn't find a suitable fix that didn't involve hardcoding those somewhere which didn't feel right, however if we are running from bp-legacy then we do correctly show 'bp-legacy-css-css' just not able to show bp-parent-css-css'

$location_type is simply an array of the three locations so isn't in itself a check, in previous version we had to hardcode those strings.

[boonebgorges]

'handle' is not correctly able to check if we are running in a child theme or simply in a parent or theme not set as a child, we always show 'bp-child-css-css'.

Ah, good catch. When template == stylesheet (ie, a theme that's not a child theme), the 'bp-child' check would be true. That seems misleading to me. Give 4949.04 a try - it doesn't even check in the redundant stylesheet directories when you're not running a child theme.

[hnla]

Huh! there you go - is_child_theme() - & you derided my use of it :)

Had to manually add those changes, git laughed in my face and bailed out on me :(

Yep that works fine - think for the moment as a first pass that crosses all the dots then and means method can be updated while retaining the result.

[boonebgorges]

In 7205:

Allow JS and CSS assets to be stored in folder stacks, akin to template stacks

Since BP 1.7 and theme compatibility, it's been possible to override BP's
fallback templates by providing templates in a number of different
subdirectories of your theme: 'buddypress' and 'community', by default. This
functionality was not, however, extended to static theme assets like JS and
CSS files, meaning that theme authors were forced to have orphan 'css' and 'js'
files in their theme roots if they wanted to override bp-legacy's assets.

This changeset mirrors the template_stack() functionality for assets, by
looking inside of 'buddypress' and 'community' theme subdirectories before
looking in root 'css' and 'js' directories (which still work, for backpat).

The implementation adopted in this changset is probably temporary for 1.8.
Future iterations may use an abstracted version of the core template_stack
functionality.

Props hnla for early patches and feedback.

Fixes #4949