Make WordPress Core

Opened 14 years ago

Closed 14 years ago

Last modified 14 years ago

#15061 closed enhancement (fixed)

Allow either array or string to be passed to locate_template

Reported by: chrisjean's profile chrisjean Owned by: scribu's profile scribu
Milestone: 3.1 Priority: normal
Severity: minor Version:
Component: Template Keywords: has-patch dev-reviewed
Focuses: Cc:

Description

Currently, template file names have to be passed as an array to the locate_template function:

locate_template(array('category-new.php', 'category.php'));

Passing in multiple template file names is a very powerful feature; however, outside of core, most code simply needs to locate a single template file name. As can be seen from this variety of locate_template calls made in BuddyPress:

locate_template( array( 'members/single/activity.php' ), true );
locate_template( array( 'members/single/messages.php' ), true );
locate_template( array( 'members/single/profile/edit.php' ), true );
locate_template( array( 'members/members-loop.php' ), true );
locate_template( array( 'activity/activity-loop.php' ), true );
locate_template( array( 'groups/single/forum.php' ), true );
locate_template( array( 'sidebar.php' ), true );
locate_template( array( 'groups/single/group-header.php' ), true );

Note that this is just a very small sampling. In total, locate_template is used 89 times in BuddyPress, and not once is more than a single file passed to locate_template.

My attached patch makes working with locate_template much easier for general usage by allowing either a string or array for the $template_names argument. A string argument will be changed into an array.

Attachments (1)

allow-string-locate_template-arg.diff (1.5 KB) - added by chrisbliss18 14 years ago.

Download all attachments as: .zip

Change History (16)

#1 @chrisbliss18
14 years ago

I should have noted in the ticket details that the patch will allow the following to work in the same manner:

locate_template( array( 'members/single/activity.php' ), true );
locate_template( 'members/single/activity.php', true );

Thus backwards-compatibility is maintained.

#2 in reply to: ↑ description ; follow-up: @filosofo
14 years ago

Replying to chrisbliss18:

Note that this is just a very small sampling. In total, locate_template is used 89 times in BuddyPress

Doesn't this fact argue against adding any unnecessary logic that would slow performance? What is the advantage of using strings alone?

#3 in reply to: ↑ 2 @chrisbliss18
14 years ago

Replying to filosofo:

Replying to chrisbliss18:

Note that this is just a very small sampling. In total, locate_template is used 89 times in BuddyPress

Doesn't this fact argue against adding any unnecessary logic that would slow performance? What is the advantage of using strings alone?

You're misunderstanding me. It is used 89 times in the entirety of BuddyPress' code, not 89 times in a single page load.

I decided to benchmark the code. The performance different when boiling down to just the difference in the old function and my patch's version is as follows for 10,000,000 function calls that randomly select between a string or an array argument:

Original: 11.932206869125
Patch:    14.583618879318

So the performance penalty is a 22.22% increase in execution time. This amounts to a performance penalty of 0.000000265 seconds for an individual call.

Looking at a standard page load (with BuddyPress since I talked about it in the ticket details), locate_template is called a total of four times.

What is the advantage of using strings alone? It's a much easier to remember argument syntax and is more in-line with other similar functions such as load_template, get_template_part (which takes a string and converts it into an array to call locate_template with), and is_page_template.

#4 @scribu
14 years ago

  • Milestone changed from Awaiting Review to 3.1
  • Owner set to scribu
  • Status changed from new to accepted

I was annoyed by this too. I think filosofo was put off by the fact that you use an if / else in the patch, when a simple cast to array would do.

#5 @scribu
14 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [15771]) Allow string as first parameter to locate_template(). Props chrisbliss18 for initial patch. Fixes #15061

#6 @hakre
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I don't like these casts to arrays in foreach. This prevent usage with other iterators in the future. Instead, we just could add a function that does the job with a single string.

Adding complexity (a parameter with two meanings at once) is not only about performance problems. Infact, performance is a no-brainer here: It just does not count. The major problem is that this patch makes the code harder to understand, but the ticket asks for easier code.

Don't get me wrong. The cast might look as a nice pinch on first sight but the headaches come later.

The tickets motivation is valid in the best sense: Why to pass an array if a string does the job for one template identifier?

#7 follow-up: @scribu
14 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

If you don't like casts to arrays, please open a separate ticket to address that issue across the board, since it's practiced in a lot of places. Thanks.

#8 in reply to: ↑ 7 @hakre
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to scribu:

If you don't like casts to arrays, please open a separate ticket to address that issue across the board, since it's practiced in a lot of places. Thanks.

Indeed. But that's a sidenote only, I reopened it because of the added complexity which is on topic.


I suggest to move locate_template() into locate_template_array() / locate_templates() and then to create locate_template() that works on a single template. In case there is code sharing between the two functions, this can be re-factored into an additional function as well. probably locate_templates() makes use of locate_template() as well.

#9 @nacin
14 years ago

hakre's points aside, the function is specifically designed to find a template given a list of templates, so I've always felt that the requirement for an array is not at all burdensome.

I don't see the need for a new function, but I don't see the need for the cast either.

#10 follow-up: @scribu
14 years ago

Besides traversing a list of possible templates, it also checks the parent/theme hierarchy, so I'm constantly doing locate_template( array('specific-file.php'), true ) in my theme.

That's why I think also allowing a string is necessary.

#11 @scribu
14 years ago

  • Severity changed from normal to minor

#12 in reply to: ↑ 10 @scribu
14 years ago

What I meant (from the inline docs):

  • Searches in the STYLESHEETPATH before TEMPLATEPATH so that themes which
  • inherit from a parent theme can just overload one file.

#13 @nacin
14 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

That's fine. It isn't a strenuous objection by any means.

hakre, I don't see a need for additional refactor or functions. If you have an issue with array casts in general, that belongs in a new ticket. Re-closing.

#14 @westi
14 years ago

  • Keywords dev-reviewed added

Supporting a string here is a good idea.
Changing to two separate functions is just pointless

#15 @hakre
14 years ago

Two different functions because that are two different responsibilities.

BTW introduction functions is not that bad as some folks think.

Regarding the (array) casts, this is a PHP 5.2 topic.

Note: See TracTickets for help on using tickets.