Skip to:
Content

BuddyPress.org

Opened 8 years ago

Closed 7 years ago

#6844 closed enhancement (fixed)

Extract & relocate core markup functions: Theme compat include functionality & search forms file

Reported by: hnla's profile hnla Owned by: boonebgorges's profile boonebgorges
Milestone: 2.7 Priority: normal
Severity: normal Version:
Component: Templates Keywords: has-patch
Cc:

Description

This ticket starts a process for extracting and moving core markup to theme location, and follows on from the discussion in #6556.

Attached patch is a first pass looking at the process for:

  1. Writing new functions to run require() on array of files specified in buddypress-functions.php
  1. Creation of new directory for includes in theme.
  1. Creation of a example include file to manage all BP dir search function
  1. Removal of search functions from core component template files.

The new functions in bp_theme_compat class is a modified version of the locate_assets_in_stack to enable checking for file_exists in the various theme locations and sub_dirs that are permitted. The function is extended to take an array of file names and return the paths to located files to be looped over and added to an include_once() call.

An initial example include file for search forms is included which reworks the search functions splitting out the actual markup to it's own function then running the existing function names to preserve backpat in possible overloaded templates along with their apply_filters and passing the unique params query string, markup tokens etc to the base markup function bp_get_dir_search_markup().

I propose for this to be implemented for 2.6 by which time a fair few core html markup functions should have been identified, re-factored and re-located.

N.B. This is very much a first pass, directory names, file names, and likely the methods I've used to create the includes functionality will all need tweaking and thought.

Attachments (9)

6844.01.patch (15.6 KB) - added by hnla 8 years ago.
Initial patch - buddypress-functions.php file include function & search form include file.
6844.diff (4.5 KB) - added by boonebgorges 8 years ago.
6844-02.patch (8.6 KB) - added by hnla 8 years ago.
Revert out the str_replace on the input ID token & is patch refresh
6844-03.patch (10.0 KB) - added by hnla 8 years ago.
refresh, update docblocks, add sub directory 'search'
6844-04.patch (7.3 KB) - added by hnla 8 years ago.
Update esc functions, remove duplicate search form template part file.
6844-05.patch (5.8 KB) - added by hnla 8 years ago.
Re-builds files anew on clean install ( refreshes )
6844-06.patch (10.2 KB) - added by hnla 8 years ago.
-06 replaces -05 & includes missing unversioned dir-search-form.php file.
6844.2.diff (8.1 KB) - added by boonebgorges 8 years ago.
6844.ajax.patch (1.2 KB) - added by r-a-y 8 years ago.

Download all attachments as: .zip

Change History (55)

@hnla
8 years ago

Initial patch - buddypress-functions.php file include function & search form include file.

#1 @boonebgorges
8 years ago

Thanks for the initial patch, hnla.

It seems to me that, in a quest to maintain backward compatibility, we've lost just about all the flexibility that we mean to gain by generating the markup in theme files rather than in core. Your suggested implementation means that you have to override an entire file full of functions (like bp-dir-search.php) in order to make a single change in your theme. Even if we modified this so that bits of markup were in their own functions (maybe with function_exists() wrappers), it still seems to me to fall short of our goal of moving everything into *template* files.

Here's an idea. Let's move all these search forms etc into proper template parts. We maintain the existing functions in core for generating the markup. Then, the template parts will have the following structure:

<?php if ( has_filter( 'bp_directory_members_search_form' ) ) : ?>
    <?php bp_directory_members_search_form(); ?>
<?php else : ?>

    Put our new and improved markup here

<?php endif; ?>

This way, we maintain backward compatibility with filters, while still using the well-known technique of template parts that can be overridden in a theme. All the locate_includes() stuff is then subsumed under the normal bp_get_template_part() template stack logic.

What do you think?

#2 @hnla
8 years ago

@boonebgorges Thanks for looking over this and the feedback. Mainly I do agree with all your points, and really just felt somewhat compelled to work things the way I did :(

has_filter() good point hadn't occurred to me to make use of this sort of check in some manner.

we've lost just about all the flexibility that we mean to gain by generating the markup in theme files rather than in core

This is true, however we still achieve one aim of removing the onus on core to maintain these functions/snippets of markup even if the implementation in the new file is sub optimal.

...you have to override an entire file full of functions (like bp-dir-search.php) in order to make a single change...

Again yes you do, although I don't see that as a massive issue, so one moves a file, in a sense the same could be said of copying a whole template file to make a simple class token change, but really the point here is to keep the functionality/markup under the control of the template level folders

In the ideal world and as I have done before the new file or search functions were actually reduced to one simple function (part of the point of these changes is that much of this core markup is actually replicated code) there is in reality a search function with a few very simple variable changes this I've handled by running a switch statement checking for current component and changing up my variables thus one form and one template tag call e.g bp_directory_search_form()

bp_get_template_part() Vs. file includes was something I did wrestle with and intended initially on using bp_get_temp... for it's stack logic then I worried that I was having to retain the original functions and wasn't in truth fetching templates but simply needing to call functions so settled on approach in that new file as a compromise.

Let's move all these search forms etc into proper template parts.

Ok, or at least to my mind and following your approach outlined not part(s) but part i.e still a single file freed of maintaining the original functions I now reduce this to one running some logic to determine what query to run, tokens etc, this would require we locate this 'part' somewhere e.g 'incl' or whatever thought better.

The next suggestion I have mixed feelings about, it's a solution and one that works and we could run with it:

<?php if ( has_filter( 'bp_directory_members_search_form' ) ) : ?>
    <?php bp_directory_members_search_form(); ?>
<?php else : ?>

    Put our new and improved markup here 

<?php endif; ?>

My reservations here are:

  • It defeats the object of removing code from core that shouldn't and needn't be there.
  • In the eventuality (albeit it slim) we need to change an aspect of this code we have to do so twice over.
  • It feels like we're just stuck with legacy code and uglyfying the template code looking for that legacy function if it's filtered.

If we retained the original functions could we not move them into deprecated positions?

In respect of the file location what is perceived as best approach, I still regard these functional aspects as something I would ordinarily maintain in an included file so functions were available, if we go the bp_get_temp... route where do we think we should locate these files given we will be dealing with, filter selects, bulk message actions, do we see these sorts of parts as living in say a top level dir such as 'incl' or 'template-parts' ?

@boonebgorges
8 years ago

#3 @boonebgorges
8 years ago

I think you're suggesting a couple different things, some of which I totally agree with. 6844.diff is an attempt to reconcile them with what I have in mind.

  1. First, I agree that the different components should share search-form code. This requires just a small amount of abstraction, which 6844.diff implements in the form of new template tags (bp_get_search_placeholder(), etc). We could probably use more of these template tags - eg, I don't know that we should be using the same one for 'id' and 'name' attributes, and it really stinks to concatenate 'members_search_submit' - but this gives you an idea of what I have in mind.
  1. Second, I agree that it's important for theme authors to be able to override this markup. That's the whole goal of the ticket, as I understand it.
  1. However, my point is that theme authors already have a system for overriding markup: the template stack.
  1. You're correct that we need some sort of new directory for shared template parts. On a lark, I'm suggesting 'common', but other suggestions are welcome. (incl or something like it seems wrong if the contents are template parts.)
  1. I put the has_filter() check into the index.php template. It's not beautiful, but who cares? This will go away when we have a new template pack.
  1. IMO, if we make this change, we no longer have to maintain the existing bp_directory_*_search_form() functions. We can list in the documentation that they are no longer used by default, and that they should not be used. Possibly we can move them to the deprecated folder, but this might mean crashing sites in some edge cases.

The approach I'm suggesting seems like much less of a departure from the existing paradigm of template overriding. What do you think?

#4 @hnla
8 years ago

  1. First, I agree that the different components should share search-form code

Yep and as shall other functions better re-use code as I get around to identifying and extracting (part of the exercise here was always the fact that in core we replicate html that not only serves no purpose sitting in core files but is slight bloat in being repeated.)

  1. ...theme authors to be able to override this markup. That's the whole goal of the ticket, as I understand it.

In part yes, at least that markup of this quantity should be in 'views'/templates not in mid tiers, let devs have access to it, filters are great but in the search form the filters are only there to get around the lack of access really.

Agreed on existing template stack i.e bp_get_template_part(). I do see a use for an include function along the lines that I added on that initial patch and will probably save for my own extended theme_compat if/when but that's going off at a tangent somewhat.

I'll check over your patch, which I'm sure will be the closer approach we'll use, it will be good to tie down the overall concept then I can push on and get some of the grunt work done in extracting other functions/markup.

Don't like common/ but can't think of anything better and agree incl not quite correct for what we're doing here.

#5 @hnla
8 years ago

Ok have re-patched to your approach which fundamentally we'll take as the route forward, I'll update the search form for some little aspects and update the other index template files shortly.

I had some reservations looking at the new patch - listed for the record but dismissing them generally.

  • We are tending to create more functions rather than reducing core functionality ( reducing cores hold on sitemarkup such as this was in part the exercise)
  • It's a slight sadness that we can't actually remove the original functions in the core template files
  • With the new functions e.g bp_get_search_placeholder() should we not be able to pass args through.

Given that fundamentally the dev could simply work up their own replacement placeholder text based on component checking and run this in their overloaded file nullifies my concerns.

Before going too much further I'll post a request in slack for comments from lead devs as to:

  • The new directory name currently common/ - Acceptable? Alternatives?
  • The new file name currently search-form.php to my mind too generic, perhaps better as bp-dir-search-form or dir-search-form ?
  • Any overall thoughts on this re-factoring?

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


8 years ago

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


8 years ago

#8 @hnla
8 years ago

-02 patch builds on Boone's 8884.diff to add in the dir index templates new has_filter query else fetch new template part and updates the new forms vars to ensure we use bp_current_component() for token names.

I think Boone's approach satisfies the overall requirements with one proviso as mentioned briefly in slack that the new template functions ( in bp-core-templates.php ) could be located in buddypress-functions.php if desired. I'm not sure that I have any strong opinion either way on this, in the theme functions file means they are more readily accessible, yet they don't really need to be that available?

I have a notion that tackling another one of these, possibly dir filters, might be useful in highlighting whether this approach here does work as set out or in reality needs further refinement so I shall look at filters or message actions to explore further in a new ticket.

#9 @boonebgorges
8 years ago

Thanks for seeing this through, @hnla. The general approach looks pretty good to me.

For brevity, I chose to use the same template tag for the 'name' and 'id' attributes. I also opted to concatenate the submit button name/id ("<?php bp_search_input_name(); ?>_submit"). Are you OK with these choices?

It'd be nice to get feedback from another core dev on the approach - moving markup out of filtered files and into template parts shared between components - before moving any further. I assume @johnjamesjacoby likes it as it's what bbPress does, but it would be nice to hear from @djpaul or @r-a-y or @imath.

#10 @hnla
8 years ago

@boonebgorges

For brevity, I chose to use the same template tag for the 'name' and 'id' attributes.

Yes it's the sensible approach and what I'd look to do always.

The one problem I have though is in using the bp_search_input_name() tag we generate a string with underscores, I still cling to the convention of never using underscores in token strings only hyphens ( although I'm aware of the BEM take on using them which I have glanced over and pondered.). Given we would need to update the scsslint.yml to allow underscores - further than I have done already in allowing specific existing strings and given we are not following BEM principles in token naming I would prefer we stuck to hyphens.

Rather than attempt to create yet another function, I tested:

id="<?php esc_attr_e(str_replace('_', '-', bp_get_search_input_name() ) ); ?>-submit"

Using the get function and a str_replace - looks and feels cumbersome?

So conversely considered going to the bp_get_search_input_name() and setting up a bp_search_input_id() version and doing a str replace there?

Or alternative is we or I accept the use of underscores, bite my tongue and try not to mind - after all it doesn't actually break anything!

Yes I too would like passing comment from the others, because I do want to press on and examine another cases of this task for reasons mentioned earlier.

On a sidenote I'll attempt an outline of all this in bpdevel over the weekend so we can allow for wider feedback/comments.

Last edited 8 years ago by hnla (previous) (diff)

#11 @boonebgorges
8 years ago

Rather than attempt to create yet another function, I tested:

Oh lord, let's please not do this!

The reason that bp_get_search_input_name() uses underscores is for backward compatibility. The existing element has underscores in its attributes, and we should avoid changing selectors.

#12 @hnla
8 years ago

The existing element has underscores in its attributes,

Sigh - yep had forgotten that, guess I deluded myself that we had moved to a point where that vile phrase :) wasn't relevant, my bad!

But can we please avoid underscores in future else I'll start writing functions with the braces on new lines and camelCasing vars

#13 @DJPaul
8 years ago

  • Milestone changed from Awaiting Review to Under Consideration

@hnla
8 years ago

Revert out the str_replace on the input ID token & is patch refresh

#14 @hnla
8 years ago

  • Milestone changed from Under Consideration to 2.6

Lets include this for 2.6 so that we:

A/ Don't waste the combined effort.
B/ Set a precedent for this and further endeavours of this nature.

My only concern remaining is in the naming of a include directory.

Suggested as '/common/' and used in testing on the template-pack but I still struggle with this, I've never used it personally as a dir naming convention, and can't recall seeing it in use(which isn't to suggest it isn't or isn't suitable).

A straw poll would be useful to gauge the most popular choice?

  • buddypress/common/
  • buddypress/includes/
  • buddypress/incl/
  • buddypress/_inc/
  • buddypress/template-parts/

Or something other?

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


8 years ago

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


8 years ago

#17 @hnla
8 years ago

  • Keywords commit added; has-patch removed
  • Owner set to hnla
  • Status changed from new to assigned

.03 patch:

  • Updates the docblocks where relevant to 2.6.0
  • Adds new sub-directoy 'search' /common/search/ as per dev chat/template chat
  • Updates get_template_part path strings.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


8 years ago

#19 @imath
8 years ago

I'm a bit afraid to view template changes this late in dev-cycle. But if you commit this, please make sure to update the @since to 2.6.0 and i think the esc_attr() shouldn't be in bp_search_input_name() function but in the markup.

@hnla
8 years ago

refresh, update docblocks, add sub directory 'search'

#20 @hnla
8 years ago

If we're not sure then best to just punt it.

#21 @dcavins
8 years ago

Beta hasn't gone out yet, so it doesn't seem that late in the dev cycle to me. :)

I generally like this approach. I would love for one of the leads to sign off on this change.

#22 @boonebgorges
8 years ago

  • Keywords needs-patch added; dev-feedback commit removed
  • Milestone changed from 2.6 to 2.7

I was about to commit this, but there are a couple of issues that need to be cleared up first. Somehow, the patch was generated in such a way that the new template is repeated three times. And there are a bunch of instances of esc_html_e() that have nothing to do with translation, so should be changed to the proper function (echo esc_attr(), I guess).

Waiting may also give us time to coalesce around a directory name. common is what's used here, and it's what I prefer, but I believe there has been near-endless discussion of a related issue in the bp-nouveau group.

This ticket was mentioned in Slack in #buddypress by boone. View the logs.


8 years ago

#24 @hnla
8 years ago

@boonebgorges no we had the discussion on names last week and can't keep on about names, we settled on your suggestion of common and further on sub-directories.

I'll fix those trans functions.

Not sure what this generation of template three times is but will look.

@hnla
8 years ago

Update esc functions, remove duplicate search form template part file.

#25 @DJPaul
8 years ago

  • Component changed from Appearance - Template Parts to Templates

@hnla
8 years ago

Re-builds files anew on clean install ( refreshes )

#26 @hnla
8 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed
  • Status changed from assigned to accepted

To be safe I re-built these files on a new install so they are refreshed and clean.

Can we have a further check with a view to committing this now.

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


8 years ago

@hnla
8 years ago

-06 replaces -05 & includes missing unversioned dir-search-form.php file.

This ticket was mentioned in Slack in #buddypress by mercime. View the logs.


8 years ago

#29 @boonebgorges
8 years ago

  • Owner changed from hnla to boonebgorges
  • Status changed from accepted to assigned

Reassigning to me for review.

#30 @mercime
8 years ago

Please add aria-hidden="true" to placeholder attribute so that information won't be read twice to screen readers. Thanks.

@boonebgorges
8 years ago

#31 @boonebgorges
8 years ago

6844.2.diff does the following:

  • Cleans up indentation (@hnla, could you have a look at your editor settings to make sure this happens automatically?)
  • Adds a bit of additional documentation (@since, etc)
  • Adds aria-hidden="true" to the label element - @mercime, can you verify that this is what you were asking for?
  • Fixes the duplicate dir-search-form.php weirdness that was causing the file to contain many duplicate copies of the same code :-/ @hnla your patch generating software has gone bonkers again ;)

#32 @hnla
8 years ago

Cleans up indentation (@hnla, could you have a look at your editor settings to make sure this happens automatically?)

What exactly were the issue never had issues here that I recall, getting seriously fed up with these bloody tools.

#33 @boonebgorges
8 years ago

@hnla Check out 6844-06.patch and, eg, the documentation blocks in bp-core-template.php. Not a huge deal to clean up, but it would be nice to avoid these being committed :)

#34 @hnla
8 years ago

@boonebgorges and normally I don't ... do I? All my settings check out as true I tab by default always have, but checked the new file dir-search-form and that had spaces, maybe it was a copy paste issue but normally I check that. Whats annoying me more is why when I clearly set auto eol in the editor svn says eol doesn't exist , tools hate em! uncertain configs hate em; the dev community always unclear on the exact standard settings...

Multiple files was idiot SVN listing all paths, folder, and file individually; select all i.e not paying attention and it assumes you want three versions...WHY???

#35 @boonebgorges
8 years ago

and normally I don't ... do I?

No! So maybe it's the tool that you're using to generate a patch, rather than your editor. Not a huge deal.

#36 @mercime
8 years ago

My bad @boonebgorges I wanted to extinguish that placeholder text so much :( Please remove the aria-hidden="true" from the labels.

The only way that we can remove that placeholder text and replace it with the label in that input field instead, then move the label up on mouseover, focus, or keydown required JS to do so ( see http://codepen.io/mercime/pen/oLARXG ). Unfortunately, pure CSS solutions I tried and researched required moving label below the search input field which is bad bad mojo. At this point, we could leave it as it was and let themers adjust as needed.

#37 @boonebgorges
8 years ago

  • Keywords has-patch dev-feedback removed

Thanks for the clarification, @mercime.

Let's get this in, and see if it needs iteration of some sort before 2.7.

Additionally, we could stand to have a bit of documentation somewhere about the new 'common' directory - its purpose, future, and advice for theme developers about how (and whether) to handle it. @hnla I don't think this needs to be done right away - let's let the improvement live in trunk for a while first - but could you think a little bit about what this documentation would look like? A codex page, a bpdevel post, etc.

#38 @boonebgorges
8 years ago

In 11048:

Templates: Move component directory search markup to 'common' template.

The new 'common' directory in the bp-legacy template pack will be a
storehouse for template parts that are shared between components. The
directory search form is the first implementation of this idea.

Props hnla, mercime.
See #6844.

#39 @r-a-y
8 years ago

Found an issue where trying to do an AJAX search from a component's directory page would not return the proper results.

The problem is due to moving the <input name=""> out of the <label> element and our AJAX JS looking explicitly for <label><input></label>.

I'm going to fix this by adjusting our JS to look for input[name] instead.

Last edited 8 years ago by r-a-y (previous) (diff)

#40 @r-a-y
8 years ago

  • Keywords has-patch added

Added ajax.patch to fix the issue I mentioned in comment:39.

This is what I'm doing in the patch:

search_terms = target.parent().find( '#' + object + '_search' ).val();

This targets our specific input text field.

Before, it was:
target.parent().children('label').children('input').val()

Tested on the Members, Groups, and Sites Directory pages and on a group's "Members" page.

The problem is theme devs that have overriden buddypress.js will not get this fix.

Last edited 8 years ago by r-a-y (previous) (diff)

@r-a-y
8 years ago

#41 @hnla
8 years ago

Found an issue where trying to do an AJAX search from a component's directory page would not return the proper results.

The problem is due to moving the <input name=""> out of the <label> element and our AJAX JS looking explicitly for <label><input></label>.

I'm going to fix this by adjusting our JS to look for input[name] instead.

Trust your judgement here - we always did trip up on the js hooks being too specific and tied to structures or constructs rather than tokens or elements.

With Nouveau however we have endeavoured to bring in a rule that we hook all JS to data-*="" attr could we do the same here?

#42 @r-a-y
8 years ago

In 11108:

bp-legacy: Fix AJAX search after directory form template refactoring.

Previously, our JS looked for a nested <label><input></label> element.
Since we moved the <input> out of the <label> element, our AJAX search
failed.

This commit changes the JS to look for the specific <input> field in
directory searches.

See #6844.

#43 @r-a-y
8 years ago

With Nouveau however we have endeavoured to bring in a rule that we hook all JS to data-*="" attr could we do the same here?

I took a quick look at what bp-nouveau is doing and it looks good!
https://github.com/buddypress/next-template-packs/blob/master/bp-templates/bp-nouveau/js/buddypress-nouveau.js#L614

Things such as using <input type="search"> and using data attributes are nice.

I don't think we can port these enhancements over though. There's a reason bp-legacy is named the way it is :)

#44 @hnla
8 years ago

There's a reason bp-legacy is named the way it is

:)

And just this morning I was thinking at long last bp-legacy will actually live up to that name properly... hopefully!

We trust - blindly - in your judgement :)

This ticket was mentioned in Slack in #buddypress by mercime. View the logs.


7 years ago

#46 @mercime
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.