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 :)

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.

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.

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.

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.

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

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.

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.

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

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

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!

@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?

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.

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

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 ;) ).

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.

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().

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.

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.