#6531 closed defect (bug) (fixed)
Recursively search for files in theme and plugin editors
Reported by: | torbens | Owned by: | pento |
---|---|---|---|
Milestone: | 4.9 | Priority: | high |
Severity: | minor | Version: | 2.5 |
Component: | Themes | Keywords: | theme-editor has-patch has-unit-tests |
Focuses: | administration | Cc: |
Description
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'.
Attachments (19)
Change History (92)
#4
@
16 years ago
- Type changed from defect (bug) to enhancement
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
#5
@
16 years ago
- Summary changed from CSS files are not shown in Theme Editor to Recursively search for files in theme and plugin editors
the same may also apply to plugins.
#7
@
13 years ago
- Cc christophe@… added
- Keywords has-patch needs-testing added; needs-patch removed
- Owner changed from anonymous to chsxf
- Status changed from new to assigned
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
#8
@
13 years ago
- Cc hm2k added
- Type changed from enhancement to defect (bug)
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.
#9
@
13 years ago
The patch i've submitted two months ago brings this fonctionnality for CSS, JS and PHP files.
#10
@
13 years ago
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!
#12
@
12 years ago
- Type changed from defect (bug) to enhancement
I would like to see this on future release. Perhaps the directory structure with js/jq implement.
#17
@
12 years ago
- Cc daedalon@… added
- Type changed from enhancement to defect (bug)
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:
- 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"); }
- 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?
#18
@
12 years ago
- 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.
#19
@
12 years ago
Encountered a bug:
- After opening a file from within a subdirectory, the Plugin Files lists only files and directories under that subdirectory.
#21
@
12 years ago
Turns out the bug in 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.
#23
@
11 years ago
- Keywords needs-ui added
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.
#24
@
11 years ago
- Keywords needs-patch added; has-patch needs-testing needs-ui removed
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 :)
#25
@
11 years ago
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.
#28
@
10 years ago
- Keywords dev-feedback added
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.
#29
@
10 years ago
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.
#31
follow-up:
↓ 38
@
9 years ago
- Keywords needs-patch added; has-patch dev-feedback removed
we need a better solution for recursion here
#34
@
8 years ago
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.)
This ticket was mentioned in Slack in #core by paaljoachim. View the logs.
7 years ago
#38
in reply to:
↑ 31
@
7 years ago
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 inget_dirsize()
which is used inget_space_used()
which are then used inwp_dashboard_quota()
). Results are stored in transient.wpmu_delete_blog()
which is recursing directories via iteration.WP_Theme::scandir()
which is used inWP_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.
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#41
@
7 years ago
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?
#42
@
7 years ago
- Priority changed from normal to high
Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.
@
7 years ago
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() )
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
7 years ago
@
7 years ago
Changes for WP_Theme::get_files(), caches all files, and reduces returns based on $type and $depth
This ticket was mentioned in Slack in #accessibility by schlessera. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by melchoyce. View the logs.
7 years ago
#50
@
7 years ago
@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.
#51
@
7 years ago
- Keywords needs-unit-tests added
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
This ticket was mentioned in Slack in #core by westonruter. View the logs.
7 years ago
#54
@
7 years ago
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 useslist_files()
get_files
now doesscandir
only once for all files at all depths, and filters it's return per call.get_plugin_files
now caches file lists returned fromlist_files()
into a transient for an hour, keyed to directory and Version.WP_Theme::get_files()
caches file lists returned fromscandir()
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!
#55
@
7 years ago
@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?
This ticket was mentioned in Slack in #core by wraithkenny. View the logs.
7 years ago
#57
follow-up:
↓ 58
@
7 years ago
@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 thelist_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.
#59
@
7 years ago
- Keywords has-unit-tests added; needs-unit-tests removed
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()
toget_plugin_files()
. As a result, the filter was renamed toget_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.
This ticket was mentioned in Slack in #core by schlessera. View the logs.
7 years ago
#61
@
7 years ago
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
#62
@
7 years ago
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.
#63
@
7 years ago
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 ;) ).
#64
@
7 years ago
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()
fromlist_files_exclusions
toget_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
This ticket was mentioned in Slack in #accessibility by wraithkenny. View the logs.
7 years ago
#70
@
7 years ago
A few notes on [41086], changed from 6531.11.diff:
- Changed the
get_plugin_files_exclusions
filter name toplugin_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 anarray_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 anarray_values()
call on the next line, to normalise the array keys. - In the
listFiles.php
tests, useassertContains()
andassertNotContains()
, and test forindex.php
instead ofabout.php
. - A handful for inline docs changes.
Could be lumped in with #6632 maybe. No point in having 2 generic editors. Not sure how common buried stylesheets are.