Recursively search for files in theme and plugin editors

[torbens]

Themes (like Subtle: ​http://gluedideas.com/downloads/subtle/) might contain numerous CSS files. The theme editor, however, does not recognize any other CSS files other than style.css.

The files might be located some levels deeper in sub directories like '/wp-content/themes/glued-ideas-subtle-01/assets/css/print.css'.

[mrmist]

Could be lumped in with #6632 maybe. No point in having 2 generic editors. Not sure how common buried stylesheets are.

[mrmist]

See also #4131. This is possibly a dupe.

[Denis-de-Bernardy]

It's not common, but it occurs. The patch from #6632 partially sorts this if the files are in a direct subfolder. If you've a file any deeper, it won't do any good. example:

wp-content/themes/sem-reloaded/skins/copywriter-blue/skin.css

[Denis-de-Bernardy]

the same may also apply to plugins.

[chsxf]

Patch is attached.

It includes :

• recursive search for files in both Theme and Plugin editors
• sorting of files by path and name
• inclusion of JavaScript files in the Theme editor

[hm2k]

It seem it will already recursively search for PHP files.

Is there any logical reason why you would not display all of the theme's CSS files from subdirectories in the theme editor?

Changing to a bug because expected behaviour is that all CSS files in the theme are shown.

[chsxf]

The patch i've submitted two months ago brings this fonctionnality for CSS, JS and PHP files.

[hm2k]

Is there any reason why you would NOT want to apply this patch?

Is there any reason you would NOT want this feature?

This bug has been open for 4 years, is there any reason why this bug cannot be closed?

Thanks!

[prionkor]

I would like to see this on future release. Perhaps the directory structure with js/jq implement.

[SergeyBiryukov]

Related: #17036

[SergeyBiryukov]

#23500 was marked as a duplicate.

[SergeyBiryukov]

Related: #22924

[SergeyBiryukov]

#24050 was marked as a duplicate.

[MikeHansenMe]

patch from 24050 that was a duplicate of this ticket.

[Daedalon]

Thanks Mike for the patch! Verified, works. Also improves the coding style to match ​http://make.wordpress.org/core/handbook/coding-standards/php/.

I'd like to see this included in 3.6 after two improvements:

1. Use curly braces with all ifs. Avoids hassle and bugs in later edits. A decent percentage of bugs are caused by editing code like this:

} else {
	if ( plugin_basename("$dir/$file") != $plugin )
		$plugin_files[] = plugin_basename("$dir/$file");
}

2. Theme editor could use the same functionality.

Marking ticket as defect (bug) instead of enhancement. More appropriate for the file list not displaying all files of a plugin or theme. Milestone, anyone?

[Daedalon]

3. In order to improve performance, instead of calling this function twice inside a loop:

   plugin_basename("$dir/$file")
   

Split it into these parts, the first of which is to be called only once in the beginning of the function:

$plugin_basedir = plugin_basename($dir);
...
"$plugin_basedir/$file"

The patch in #24049 also applies this change.

[Daedalon]

Encountered a bug:

4. After opening a file from within a subdirectory, the Plugin Files lists only files and directories under that subdirectory.

[MikeHansenMe]

should fix the bug mentioned in comment 19

[MikeHansenMe]

Turns out the bug in comment 19 affects core now. Should it be a separate ticket, even though 24050.2.diff fixes it? Also I still think there needs to be some ui improvements for the long list.

[MikeHansenMe]

I think if we want to get this into core we need to get a UI that supports a long list of files and file tree structure.

[helen]

Let's not let this ticket get hung up on UI - see #24048 for that instead.

As for this, what if we go with displaying the path for some clarity in themes, I guess the way plugins do now? And could we get a unified patch addressing all of the above? Can't apply the existing ones together, but one only deals with plugins :)

[Daedalon]

Agreed with Helen. It's crucial to be able to see all the files in subfolders, and secondary to prettify it. Making all of the files visible is also pretty much a prerequisite for making the list neat. The sooner we display the full list in any ugly way, the sooner we can start to work on the chrome and polish for it.

Regarding Mike's excellent patches above, my opinion is that if there's one patch that fixes all of them for both plugins and themes, that'd be just perfect.

[MikeHansenMe]

Show path for plugins and themes

[MikeHansenMe]

Does not show if description and relative file are the same

[valendesigns]

The 6531.3.diff​ patch includes unit tests to prove that get_plugin_files will now return paths beyond two directory levels deep. This new patch subtly updates the UI for both plugins and themes. The previous patch didn't change the UI for plugins, but did for themes. I've updated the code to improve performance according to comment #18. I also solved an issue where the select dropdown was not showing the correct selected plugin when editing a file that wasn't the main plugin file.

C'mon Guys. Let's get this ticket closed! It's been open for 7 years.

[valendesigns]

I guess I didn't test the theme code thoroughly enough from the previous patch. When I was showing someone how to apply and create a patch I noticed a bug when viewing the tree with a non default theme. Some of the files had empty strings in the nonessential span element which meant you just saw "()" under the file without text inside. I've fixed it in the latest patch.

[wonderboymusic]

we need a better solution for recursion here

[DrewAPicture]

#24050 was marked as a duplicate.

[Maikuolan]

Any news on this?

[WraithKenny]

The patches on the ticket address the scanning of plugin files. For Themes, it's easier, and if we can't just bump up the depth (for whatever reason), maybe we could put filters and let themes "grant access" to its own deeper files.

foreach ( $file_types as $type ) {
	switch ( $type ) {
		case 'php':
			$allowed_files += $theme->get_files( 'php', apply_filters('wp_theme_scandir_depth', 1, $type, $stylesheet ) );
			$has_templates = ! empty( $allowed_files );
			break;
		case 'css':
			$style_files = $theme->get_files( 'css', apply_filters('wp_theme_scandir_depth', 0, $type, $stylesheet ) );
			$allowed_files['style.css'] = $style_files['style.css'];
			$allowed_files += $style_files;
			break;
		default:
			$allowed_files += $theme->get_files( $type, apply_filters('wp_theme_scandir_depth', 0, $type, $stylesheet ) );
			break;
	}
}

This would at least allow themes (TwentyEighteen, for example) to do

add_filter( 'wp_theme_scandir_depth', function( $depth, $type, $theme ) {
	if ('twentyeighteen' == $theme && 'css' == $type)
		return 2;
	return $depth;
},10,3 );

This would be useful for plugins that enhance the theme editor, such that they no longer have to completely rewrite the entire theme-editor.php file. (example: ​https://github.com/unFocus/Scripts-n-Styles/blob/17e3c1b522ef7aa349b417041f12ccd4853f80e6/dist/includes/theme-editor-page.php)

I've also rewrote the entire plugin-editor.php to use the patch on this ticket ​https://github.com/unFocus/Scripts-n-Styles/blob/17e3c1b522ef7aa349b417041f12ccd4853f80e6/dist/includes/plugin-editor-page.php (Currently in 4.0.0-alpha, not in the public release.)

[SergeyBiryukov]

#38470 was marked as a duplicate.

[WraithKenny]

Adds filters for theme editor

[westonruter]

Replying to wonderboymusic:

we need a better solution for recursion here

@wonderboymusic Do you have specific feedback on what needs improvement? Is it that recursion in general is bad or the current implementation in the patch needs revision?

I'm looking for other cases of recursive directory traversal in core and I see:

• list_files(), which isn't even used in core. (⁉️)
• recurse_dirsize() used in get_dirsize() which is used in get_space_used() which are then used in wp_dashboard_quota()). Results are stored in transient.
• wpmu_delete_blog() which is recursing directories via iteration.
• WP_Theme::scandir() which is used in WP_Theme::get_files() and currently limits depths by file type. This one seems it should be refactored to get the full list of files and then split them up by type, rather than do multiple directory iterations for each type.

One thing to consider as well is to add the list of theme/plugin files to a transient, with the theme/plugin version in the cache key, so that the recursion doesn't have to be done every single time the editor is loaded.

It seems that get_plugin_files() should be using list_files(), since the former is using nested loops rather than proper recursion.

I think it would be better to go the route of using a recursive directory iterator rather than add filters to extend the depth on a type-by-type basis.

[netweb]

#38470 was marked as a duplicate.

[schlessera]

I'm interested in tackling this for 4.9.

I'd like to investigate using proper PHP iterator objects, and provide some custom magic so that they can transparently cache into transients/object cache.

The main benefit is that you don't pull in all files and folders into memory just to pass them around, but rather you prepare an iterator object and pass that on. In some cases, the actual iteration will never happen as it is not needed, and in some cases you'll only end up with a partial iteration, which saves IO and memory as well.

Once the iterators are built, they can easily be adapted to replace the above functions.

Is there anything that would speak against such an approach?

[westonruter]

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

[WraithKenny]

Rewrites get_plugin_files() to use list_files, add filter to list_files (based on WP_Theme::scandir() ) and adds transient cache (based on recurse_dirsize() )

[WraithKenny]

same as last, but cleaned up and using plugin version in caching

[WraithKenny]

Changes for WP_Theme::get_files(), caches all files, and reduces returns based on $type and $depth

[WraithKenny]

full patch

[afercia]

Note: this is needed for #41729

[WraithKenny]

If anyone has feedback on the patch, I can update it. Just let me know.

[schlessera]

@WraithKenny Here are some observations regarding the patch:

• list_files() uses recursion, which can put a lot of strain on the system or even lead to failures in extreme cases.
• The caching mechanism will fail if the result of the traversal is indeed an empty set (like if the only plugin was excluded). This will disable the caching and iterate over the files every single time.
• The caching mechanism is prone to cache stampedes on high load servers, where the cache might be refreshed multiple times in parallel.

I would recommend returning PHP recursive iterators instead of using a recursive function and collecting the entire result set, and building a more fail-safe caching mechanism.

Using PHP iterators has the following advantages:

• Depending on the use case, traversal can stop early.
• Iterators can still be modified (i.e. further filtered) after they have been returned.
• Depending on the PHP version support, they can even be better optimized to transparently support parallel crawling or other neat tricks.

[westonruter]

Δ ​https://github.com/xwp/wordpress-develop/pull/271/commits/f80a849

[westonruter]

I've applied some fixes to PHP code style which were identified by ​PHPCS. See 6531.7.diff​ (and ​delta).

In addition to @schlessera's great feedback, one thing that became clear to me in testing the patch is that we should absolutely ignore certain directories from being traversed. I'm thinking node_modules, vendor, and bower_components to start. The list of files can get so incredibly long that it just becomes impossible to navigate. I don't think we can safely throw this on users if we don't also tackle #41729.

While this is out of scope for this ticket, the latency for recursing over all the files is compounded by the fact that every time the user saves a file, the result is a full page reload when there really is no need for that. In reality the new file's contents should get submitted over Ajax (er, eventually maybe REST API) and in that way the user wouldn't be continually pinging the server to re-generate the list of files every single time, and making the user have to re-locate their scroll position after saving. There is crossover here as well with #39766 and #39766 where we could avoid the whole need to detect when an PHP error happens and then when the plugin editor page reloads to throw in an iframe to try scraping for the error message again. The error message can simply be returned in the Ajax response and then shown to the user immediately. /cc @melchoyce

[WraithKenny]

Same as previous, but with extra excludes

[WraithKenny]

Here is a rundown of the current patch

• list_files() now contains a filter called 'list_files_exclusions' that will ignore hidden files and folders and folders named 'CVS', 'node_modules', 'vendor', or 'bower_components' when scanning for files. ('CVS' because it existed in 'theme_scandir_exclusions'.)
• WP_Theme::scandir() filter 'theme_scandir_exclusions' updated to exclude 'vendor', or 'bower_components' also.
• get_plugin_files now uses list_files()
• get_files now does scandir only once for all files at all depths, and filters it's return per call.
• get_plugin_files now caches file lists returned from list_files() into a transient for an hour, keyed to directory and Version.
• WP_Theme::get_files() caches file lists returned from scandir() into a transient for an hour, keyed to theme Name and Version.
• theme-editor.php calls to $theme->get_files() are passed with -1 to get all files of the type.

Patch 6531.9.diff is to address some of @schlessera's comments, particularly the "empty set" condition. I'm not sure I fully understand the implications however, since an empty set would cause the function to scan for 0 files when the theme editor is loaded. I updated the patch to check for false === $all_files which should make sure that it won't run more than once per hour. As for the "stampedes" issue, I don't fully understand, but that'd be a wider issue for transients and caching in general rather than this ticket, right?

As for PHP recursive iterators, I'm not too familiar with them, but I'll research after work to see what I can do, unless you'd like to take a stab at it :) Thanks for the feedback!

@westonruter thanks for the link to wpcs/phpcs; I've got it installed now.

I've got the folders you've suggested filtered out now (and devs can use the filter to adjust to their liking, if they really really want to opt into including 'vendor' or whatever after #24048 folders are added).

It also looks like you've already solved a lot of the issues in the second paragraph over in #24048! Awesome job there!

[WraithKenny]

@schlessera and @westonruter I could add back in the filters from 6531-theme.diff and add a similar one (or better named one) onto the list_files() call in get_plugin_files() so that sites concerned could put the depth back to 1 if they have issues. (it'd be no worse of a load then currently, right?) Thoughts?

[schlessera]

@WraithKenny :

particularly the "empty set" condition. I'm not sure I fully understand the implications however, since an empty set would cause the function to scan for 0 files when the theme editor is loaded.

The main issue was that caching would have been disabled as a side effect, thus fetching from cache every single time, failing every single time, and then starting to look through the filesystem every single time.

With the check for false you did now, the cache will be checked only once, and it will correctly cache the fact that it didn't find any files.

As for PHP recursive iterators, I'm not too familiar with them, but I'll research after work to see what I can do, unless you'd like to take a stab at it :) Thanks for the feedback!

I will see if I can manage to fit in that change before the deadline...

Additionally:

• I'd suggest having a way to define/override the exclusions for list_files() through the function call, and putting the filters outside of that "reusable" function. I don't think the filter conceptually fits into the list_files() implementation.

• The transient name needs to be truncated to control the length, to avoid running into obscure bugs where a long directory name breaks the caching.

[WraithKenny]

I can work on the 2 "additionally"

[schlessera]

Iterator implementation.

[schlessera]

Starting from @WraithKenny 's 6531.9.diff patch, I did the following changes:

• I switched the list_files() implementation from a recursive function to an iterator-based approach.
• The max. $level argument has been deprecated, as it doesn't make any sense with iterators. I kept it in the signature though to stay safe wrt BC. If we know this is not used, we can remove it from the signature to clean up the code.
• The function returns an iterator object, not an array. This allows consuming code to add further filtering, while working as usual with foreach() loops.
• The iterator returns SplFileInfo objects. You can retrieve the path or the filename individually from these. Casting them to a string will result in a filename that includes the absolute path.
• I moved the exclusions filter from list_info() to get_plugin_files(). As a result, the filter was renamed to get_plugin_files_exclusions.
• I added the possibility to match a regex. This helps further filtering files.
• I added the possibility to choose whether to skip hidden files or not.
• I truncated the transient label and added an md5 to keep it unique.
• I added basic unit tests for the list_files() function.

[WraithKenny]

I'm getting an error testing the patch:

<?php
Fatal error: Uncaught Error: Trying to clone an uncloneable object of class RecursiveIteratorIterator in /var/www/html/build/wp-includes/option.php:452 Stack trace: #0 /var/www/html/build/wp-includes/option.php(766): add_option('_transient_list...', Object(RecursiveIteratorIterator), '', 'no') #1 /var/www/html/build/wp-admin/includes/plugin.php(227): set_transient('list_files_cach...', Object(RecursiveIteratorIterator), 3600) #2 /var/www/html/build/wp-admin/plugin-editor.php(69): get_plugin_files('gutenberg/guten...') #3 {main} thrown in /var/www/html/build/wp-includes/option.php on line 452

[WraithKenny]

alternative

[WraithKenny]

I'm trying to debug 6531.10.diff to see if it's my dev setup or something, but in the meantime, I had the patch I was working on 6531.10.a.diff which is like .9 but with the filter for list_files moved out, and better transient keys.

[schlessera]

I tried several permutations now to transform the iterator into an array to stay safe and not try to serialize the actual iterator object, but the RecursiveFilterIterator seems buggy in its PHP 5.2 incarnation.

Normally, with PHP 5.4+, you'd use a CallbackFilterIterator, but with PHP 5.2, you have to create a custom class instead, which will need a second constructor argument to inject the options. And whatever combinations I have tried, the conversion to an array always created new instances of that filter and omitted the second argument to the constructor.

Without the filter, it is not possible to filter folders, only files, so it would not be possible to exclude folders such a node_modules. This is a show-stopper for using iterators in this case.

I suggest going with the recursive function for now, and re-investigate using iterators when WordPress Core uses PHP 5.4 as a minimum (hopefully next year, then ;) ).

[schlessera]

Recursive approach + tests

[schlessera]

I added another patch named 6531.11.diff. It takes @WraithKenny 's 6531.10.a.diff patch and adds the following changes:

• Transient key is truncated and made unique with an MD5
• Fixes an issue with double slashes in the generated paths
• Renames the filter in get_plugin_files() from list_files_exclusions to get_plugin_files_exclusions to bring it in line with the theme specific filter
• Makes sure single files are always producing the correct path by wrapping them into plugin_basename()
• Adds unit test for get_plugin_files() when testing against a plugin with subfolders
• Adds unit test for list_files() to return arbitrary files
• Adds unit test for list_files() to support exclusions

[WraithKenny]

The latest patch looks really solid at this point. Tested on php 5.6 and 7.1

[WraithKenny]

#39316 was marked as a duplicate.

[pento]

In 41806:

File Editor: Add support for more than one sub-directory level.

The theme and plugin editors now list all files in the selected theme or plugin, recursing through subdirectories as necessary.

Props WraithKenny, schlessera, chsxf, MikeHansenMe, Daedalon, valendesigns, westonruter, pento.
Fixes #6531.

[pento]

A few notes on [41086], changed from 6531.11.diff​:

• Changed the get_plugin_files_exclusions filter name to plugin_files_exclusions. Though it's slightly different from the function name, it more accurately reflects what it's for. (It's for filtering the plugin files exclusions, not for filtering whether to get the plugin files exclusions.)
• In get_plugin_files(), swap the $plugin_files += $list_files line for an array_merge(), because the former didn't work properly in PHP 5.2/5.3 ($list_files had different keys in different PHP versions). Similarly, add an array_values() call on the next line, to normalise the array keys.
• In the listFiles.php tests, use assertContains() and assertNotContains(), and test for index.php instead of about.php.
• A handful for inline docs changes.

[westonruter]

In 42112:

Theme Editor: Ensure files listed recursively can be both viewed and edited.

Prevent edits to 2-level deep theme files from returning a disallowed_theme_file error when attempting to save an edit. Aligns logic for gathering $allowed_files in theme-editor.php for listing files with the validation logic in wp_edit_theme_plugin_file().

Amends [41806].
See #6531.
Fixes #42425.

[dd32]

In 42242:

Theme/Plugin Editor: Remove the caching added in [41806] as it causes more problems than it fixes.

While caching here seemed like a good idea in theory, in practice the cache would be often stale causing development issues.
We exclude common folders (such as node_modules) from the scanning to avoid directories which are not useful to the end-user, so as long as those exclusion lists are held up this shouldn't cause too much of a degredation in the future.
We may consider adding caching here again in the future if it's determined that it is really needed.

Props precies, ibenic, mariovalney, schlessera, and all the others who commented on the ticket(s).
This partually reverts [41806].
See #6531.
Fixes #42573.

[dd32]

In 42243:

Theme/Plugin Editor: Remove the caching added in [41806] as it causes more problems than it fixes.

While caching here seemed like a good idea in theory, in practice the cache would be often stale causing development issues.
We exclude common folders (such as node_modules) from the scanning to avoid directories which are not useful to the end-user, so as long as those exclusion lists are held up this shouldn't cause too much of a degredation in the future.
We may consider adding caching here again in the future if it's determined that it is really needed.

Props precies, ibenic, mariovalney, schlessera, and all the others who commented on the ticket(s).
This partually reverts [41806].
Merges [42242] to the 4.9 branch.
See #6531.
Fixes #42573 for 4.9.