Trigger an error when a required include fails to be included...

[al-the-x]

I encountered this while attmepting to deploy a Roots-based theme to @wpengine via git. A global .gitignore rule omitted the contents of roots/lib/ from the index, so none of the require_once lines from functions.php worked. The locate_template() function can require the file, given appropriate flags, and returns an empty string if it can't find it. More useful would be an error message when this occurs. Whether this deserves E_USER_ERROR, E_USER_WARNING or E_USER_NOTICE is a subject for debate. Please advise.

[QWp6t]

👍

But I have a feeling that the roots folks are gonna balk at your closure. You'll notice that closures are not used anywhere in roots.

[al-the-x]

@QWp6t Understood, if they were targeting PHP < 5.3.x, but closure support has been around a while... Is there an official published target PHP version for roots? I've already had to switch back to ugly array() syntax because of WPEngine. :[

Anyone from @roots care to weigh in?

[QWp6t]

I feel ya. I've mentioned both here and in Bedrock repo that I think all of the Roots projects shouldn't be afraid to just use modern PHP syntax. I think using short syntax for arrays, closures where appropriate, etc., shouldn't be considered controversial, and it's consistent with @swalkinshaw's quest to "drag WordPress into 2014."

On the other hand, it's harder to reach a wide audience when you start leaving people behind because their shared hosting account doesn't make modern versions of PHP available to them.

[al-the-x]

Yeah, I agree for the most part, but the last few versions of PHP have contained very major language changes and been released much faster than previous versions. It will take some time to build up momentum when your language runs 80% of the internet. ;)

[swalkinshaw]

We'll probably switch a PHP 5.3 requirement soon since we want to use namespaces. Nothing I hate more than prefixing function names.

[markthethomas]

^ 👍

[QWp6t]

Support for 5.3 ends next month. So that's still a bit old, but I suspect it's a good compromise between the old and new.

Hell yeah for namespacing. I ALMOST submitted a pull request for Soil to add namespacing because add_filters() is a common utility function and its presence in the plugin might screw people up when they have it in their theme, especially if they don't use the same logic as yours. But that's not relevant here, so I'll leave it at that.

[QWp6t]

On second look, you don't need to array_map this at all. I think this belongs in a foreach loop.

v6.x

$roots_includes = array(
    '/lib/utils.php',           // Utility functions
    '/lib/init.php',            // Initial theme setup and constants
    '/lib/wrapper.php',         // Theme wrapper class
    '/lib/sidebar.php',         // Sidebar class
    '/lib/config.php',          // Configuration
    '/lib/activation.php',      // Theme activation
    '/lib/titles.php',          // Page titles
    '/lib/cleanup.php',         // Cleanup
    '/lib/nav.php',             // Custom nav modifications
    '/lib/gallery.php',         // Custom [gallery] modifications
    '/lib/comments.php',        // Custom comments modifications
    '/lib/relative-urls.php',   // Root relative URLs
    '/lib/widgets.php',         // Sidebars and widgets
    '/lib/scripts.php',         // Scripts and stylesheets
    '/lib/custom.php',          // Custom functions
);

v7.x

$roots_includes = array(
    '/lib/utils.php',           // Utility functions
    '/lib/init.php',            // Initial theme setup and constants
    '/lib/wrapper.php',         // Theme wrapper class
    '/lib/sidebar.php',         // Sidebar class
    '/lib/config.php',          // Configuration
    '/lib/activation.php',      // Theme activation
    '/lib/titles.php',          // Page titles
    '/lib/nav.php',             // Custom nav modifications
    '/lib/gallery.php',         // Custom [gallery] modifications
    '/lib/comments.php',        // Custom comments modifications
    '/lib/scripts.php',         // Scripts and stylesheets
    '/lib/extras.php',          // Custom functions
);

Note that I separated the array out into a variable for readability purposes, but also in case anyone wants to manipulate it before the loop.

foreach loop

foreach($roots_includes as $file){
    require_once(locate_template($file));
};

The reason I didn't use locate_template to load the file (only to return the value of the file's location) is because it isolates the scope, and I want to be sure that variables set in each of the files can be read outside of those files (e.g., $content_width). array_map would have also prevented this to an extent, so it's another reason to use the foreach loop.

[al-the-x]

My primary motivation of this ticket was to avoid relying on the
"locate_template" function as an input to "require_once", as the return
value of "locate_template" is FALSE if the file isn't found. This leads
to unhelpful error messages if a file is missing as PHP juggles FALSE to
empty string for "require_once" and you're left wondering what PHP was
trying to require in the first place.

I proposed using a map operation to remove some of the repetition (require
as well as "lib/" everywhere) and isolate scope, actually. Don't you think
scope pollution should be intentional - through use of constants and (ugh)
globals - instead of accidental...?

Although debating the validity of scope isolation is off-topic a bit and
something of an implementation detail. If shared scope is the expected
behavior, then iteration is the right answer for sure. I didn't expect
that, but that's me. ;)
On Jun 12, 2014 5:53 PM, "QWp6t" notifications@github.com wrote:

On second look, you don't need to array_map this at all. I think this
belongs in a foreach loop.
v6.x

$roots_includes = array(
'/lib/utils.php', // Utility functions
'/lib/init.php', // Initial theme setup and constants
'/lib/wrapper.php', // Theme wrapper class
'/lib/sidebar.php', // Sidebar class
'/lib/config.php', // Configuration
'/lib/activation.php', // Theme activation
'/lib/titles.php', // Page titles
'/lib/cleanup.php', // Cleanup
'/lib/nav.php', // Custom nav modifications
'/lib/gallery.php', // Custom [gallery] modifications
'/lib/comments.php', // Custom comments modifications
'/lib/relative-urls.php', // Root relative URLs
'/lib/widgets.php', // Sidebars and widgets
'/lib/scripts.php', // Scripts and stylesheets
'/lib/custom.php', // Custom functions);

v7.x

$roots_includes = array(
'/lib/utils.php', // Utility functions
'/lib/init.php', // Initial theme setup and constants
'/lib/wrapper.php', // Theme wrapper class
'/lib/sidebar.php', // Sidebar class
'/lib/config.php', // Configuration
'/lib/activation.php', // Theme activation
'/lib/titles.php', // Page titles
'/lib/nav.php', // Custom nav modifications
'/lib/gallery.php', // Custom [gallery] modifications
'/lib/comments.php', // Custom comments modifications
'/lib/scripts.php', // Scripts and stylesheets
'/lib/extras.php', // Custom functions);

Note that I separated the array out into a variable for readability
purposes, but also in case anyone wants to manipulate it before the loop.
foreach loop

foreach($roots_includes as $file){
require_once(locate_template($file));};

The reason I didn't use locate_template to load the file (only to return
the value of the file's location) is because it isolates the scope, and I
want to be sure that variables set in each of the files can be read outside
of those files (e.g., $content_width). array_map would have also
prevented this to an extent, so it's another reason to use the foreach
loop.

—
Reply to this email directly or view it on GitHub
#1042 (comment).

[QWp6t]

You're correct that the scope should be isolated, and I personally isolate it on my end, but it might yield unexpected results for certain users which is why I opted not to make that change in my post above. Also, locate_template already isolates the scope in the way you were using it, so using array_map to isolate the scope becomes redundant.

This would give you the results you want, yeah?:

foreach($roots_includes as $file){
    if($filepath = locate_template($file)) {
        require_once($filepath);
    } else {
        trigger_error("Error locating `$file` for inclusion!", E_USER_ERROR);
    }
}

[al-the-x]

You make a good point about WP devs being surprised by an isolated scope. That's just how WP rolls, amirite? If I might refactor your proposal... I prefer guards to else:

foreach($roots_includes as $file){
    if ( ! $filepath = locate_template($file) ) {
        trigger_error("Error locating `$file` for inclusion!", E_USER_ERROR);
    }
    require_once $filepath;
}

Since E_USER_ERROR is equivalent to E_ERROR (Fatal Error), script execution will cease at the first failure, with an appropriate message. Also, parens aren't necessary around require_once as it's a language construct (keyword), not a function, e.g. echo or throw.

I still prefer the brevity of my original proposal, but I can definitely concede your points.

[QWp6t]

If you can squash all of this into one commit and submit it as the following (to make it stylistically consistent with the rest of Roots), I've been given the green light to merge it.

$roots_includes = array(
  '/lib/utils.php',           // Utility functions
  '/lib/init.php',            // Initial theme setup and constants
  '/lib/wrapper.php',         // Theme wrapper class
  '/lib/sidebar.php',         // Sidebar class
  '/lib/config.php',          // Configuration
  '/lib/activation.php',      // Theme activation
  '/lib/titles.php',          // Page titles
  '/lib/cleanup.php',         // Cleanup
  '/lib/nav.php',             // Custom nav modifications
  '/lib/gallery.php',         // Custom [gallery] modifications
  '/lib/comments.php',        // Custom comments modifications
  '/lib/relative-urls.php',   // Root relative URLs
  '/lib/widgets.php',         // Sidebars and widgets
  '/lib/scripts.php',         // Scripts and stylesheets
  '/lib/custom.php',          // Custom functions
);

foreach($roots_includes as $file) {
  if(!$filepath = locate_template($file)) {
    trigger_error("Error locating `$file` for inclusion!", E_USER_ERROR);
  }
  require_once $filepath;
}
unset($file, $filepath);

[al-the-x]

Happy to do it. Is there a style document for the project so I can avoid
these conflicts in future? I can configure Syntastic to warn me or just fix
stylistic errors.
On Jun 15, 2014 9:25 PM, "QWp6t" notifications@github.com wrote:

If you can squash all of this into one commit and submit it as the
following (to make it stylistically consistent with the rest of Roots),
I've been given the green light to merge it.

$roots_includes = array(
'/lib/utils.php', // Utility functions
'/lib/init.php', // Initial theme setup and constants
'/lib/wrapper.php', // Theme wrapper class
'/lib/sidebar.php', // Sidebar class
'/lib/config.php', // Configuration
'/lib/activation.php', // Theme activation
'/lib/titles.php', // Page titles
'/lib/cleanup.php', // Cleanup
'/lib/nav.php', // Custom nav modifications
'/lib/gallery.php', // Custom [gallery] modifications
'/lib/comments.php', // Custom comments modifications
'/lib/relative-urls.php', // Root relative URLs
'/lib/widgets.php', // Sidebars and widgets
'/lib/scripts.php', // Scripts and stylesheets
'/lib/custom.php', // Custom functions);
foreach($roots_includes as $file) {
if(!$filepath = locate_template($file)) {
trigger_error("Error locating $file for inclusion!", E_USER_ERROR);
}
require_once $filepath;}unset($file, $filepath);

—
Reply to this email directly or view it on GitHub
#1042 (comment).

[al-the-x]

Squashed. Merge at your convenience...! Happy dad's day, all.

[al-the-x]

Rebased on master while I was at it...

[kuldeepphp]

Hello Every one i got the same error

My php version is 5.3.5.
Please advice me.

[chrisatomix]

@kuldeepphp This is not a support forum, you should ask for assistance on the Roots Discourse:
https://discourse.roots.io