Skip to:
Content

#4949 closed enhancement (fixed)

Revise enqueue style locations for theme compat

Reported by: hnla Owned by: boonebgorges
Milestone: 1.8 Priority: low
Severity: normal Version:
Component: Theme Keywords: has-patch 2nd-opinion needs-testing
Cc: hnla, mercijavier@…, johnjamesjacoby

Description

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.

Attachments (5)

4949-01.patch (2.3 KB) - added by hnla 12 months ago.
re-factor file_exist stylesheet checks
4949.patch (4.6 KB) - added by boonebgorges 11 months ago.
4949.02.patch (5.9 KB) - added by boonebgorges 11 months ago.
4949.03.patch (5.8 KB) - added by hnla 11 months ago.
correct malformed uri - add trailingslashit()
4949.04.patch (6.0 KB) - added by boonebgorges 11 months ago.

Download all attachments as: .zip

Change History (16)

hnla12 months ago

re-factor file_exist stylesheet checks

comment:1 mercime11 months ago

  • Cc mercijavier@… added
  • Keywords changed from legacy, style enqueueing to legacy style enqueueing

comment:2 boonebgorges11 months ago

  • Keywords has-patch added; legacy style enqueueing removed
  • Milestone changed from Awaiting Review to 1.8

Moving to 1.8 for review.

comment:3 boonebgorges11 months ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 1.8 to 1.9

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.

comment:4 hnla11 months ago

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.

boonebgorges11 months ago

comment:5 boonebgorges11 months ago

  • Cc johnjamesjacoby added
  • Keywords has-patch 2nd-opinion needs-testing added; needs-patch removed
  • Milestone changed from 1.9 to 1.8

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?

boonebgorges11 months ago

comment:6 hnla11 months ago

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.

comment:7 hnla11 months ago

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

Last edited 11 months ago by hnla (previous) (diff)

hnla11 months ago

correct malformed uri - add trailingslashit()

comment:8 hnla11 months ago

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.

Last edited 11 months ago by hnla (previous) (diff)

comment:9 boonebgorges11 months ago

'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.

boonebgorges11 months ago

comment:10 hnla11 months ago

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.

comment:11 boonebgorges11 months ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

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

Note: See TracTickets for help on using tickets.