#33413 closed task (blessed) (fixed)
most PHP classes should be in their own file
Reported by: | wonderboymusic | Owned by: | nacin |
---|---|---|---|
Milestone: | 4.4 | Priority: | normal |
Severity: | normal | Version: | 4.4 |
Component: | General | Keywords: | |
Focuses: | Cc: |
Description
WordPress has a weird way of bundling things in files: symbols, functions, classes, top-level code, the works. This can get increasingly bad over time. If we were to start WP anew today, there is no way we would organize our code in this fashion. This begs the question: are we open to cleaning some of these things up, or do we need these files to continue to arbitrarily include the works?
PSR-1, if anyone cares, suggests:
Files SHOULD either declare symbols (classes, functions, constants, etc.) or cause side-effects (e.g. generate output, change .ini settings, etc.) but SHOULD NOT do both.
I am not suggesting that we adopt PSR-1, but ...
In #30947 and #32529, I was able to remove top-level code from several WP files. This allows the remaining files to only declare functions. default-filters
, admin-filters.php
, and ms-admin-filters.php
contain only function calls (top-level code). One remaining irksome piece: there are some multi-thousand line files in the codebase that declare a swath of functions, and then, seemingly out of nowhere: they declare a class.
There are several files in wp-includes
that declare classes properly: 1) in their own file and 2) using a consistent naming convention: class-{DASHED LOWERCASED CLASS}.php
. All classes should use this convention.
Forgetting that some of our classes are way too big and do way too much, some of these files contain hundreds/thousands of lines of function declarations and then still also declare a class of 100s/1000s of lines long.
I think we can see where this is heading, I would like to propose the creation of class files for:
WP_Comment_Query WP_Http_Cookie WP_Http_Curl WP_Http_Encoding WP_HTTP_Proxy WP_Http_Streams WP_Http WP_Meta_Query WP_Post WP_Query WP_Rewrite WP_Role WP_Roles WP_Tax_Query WP_User_Query WP_User WP_Widget_Factory WP_Widget
We had a similar problem with Media JS, which was alleviated by me in [31373]. It is WAY easier to absorb complex code when these units are broken up.
I have a patch, and I welcome feedback.
Attachments (8)
Change History (149)
#2
@
9 years ago
the patch uses svn cp
to create the files - does that persist history well enough, or is there another way?
#3
@
9 years ago
Ah, I see. Yeah, svn cp
should do the job.
Minor side note, classes.diff isn't applying, it seems to be having trouble with creating the new files, I think because svn diff
doesn't show copied files as files that need to be created.
#4
in reply to:
↑ 1
@
9 years ago
Replying to pento:
I'd probably add a block to include the classes in their original location if they're undefined, in case people are manually including individual files.
ie, in
query.php
:
if ( ! class_exists( 'WP_Query' ) ) { require( ABSPATH . WPINC . '/class-wp-query.php' ); }
I strongly agree with the premise of this ticket, and I also concur with @pento's concern above, with regard to making sure classes that are moved out of their original files can still be referenced by existing codebases.
With regard to coding standards, whether or not WordPress as an open-source project wants to adopt the PSR-1 standards, I have noticed from digging through the codebase a bit recently that there is quite a bit of code that fails to adhere to WordPress's own standards. I think it's worthy of discussion whether that's something to address for 4.4 (and future releases), as there are some code quality issues throughout core that could benefit from cleanup/simplification (such as ensuring braces always surround conditional blocks, not doing an if { return $this; } else { return $that; } etc. I'll dig around and see if there's already a ticket about this, and if not, I'll open one.
#5
@
9 years ago
Yes! This has been an issue that has plagued our code base for years. I'd say say any ground we can gain on subdividing some of these multi-thousand line files would be a win.
#6
@
9 years ago
I love this idea! I'd like to extend to default-widgets.php as well. We are already hoping to refresh that file for 4.4 ( #23012 ) and the next logical step would be to split those files out into separate classes.
#7
@
9 years ago
Totally agree with this idea. WordPress is growing fast and we need to keep things organized.
But we need to take care of some files that plugins developers usually include by their own, or at least be aware of this. (Although, looking at the patch, I didn't see any file change that can break backward compatibility)
#8
@
9 years ago
If you'd like to preview the diff in the browser, it can be viewed here: https://cloudup.com/caVNEnpSGxI
#20
follow-up:
↓ 24
@
9 years ago
It looks like r33748 broke WP-CLI:
PHP Fatal error: Cannot declare class WP_Http, because the name is already in use in /home/kovshenin/htdocs/develop.lo/src/wp-includes/class-http.php on line 15
It's happening because class-http.php is being included twice, once in http.php and the second time in WP-CLI's version of wp-settings.php just a moment later. I guess this may break other "forks" of wp-settings.php and possibly some plugins that include/require the files directly, though a quick search through the .org directory shows mostly include_once
and require_once
.
This ticket was mentioned in Slack in #docs by garyj. View the logs.
9 years ago
#22
@
9 years ago
@kovshenin: PR is here https://github.com/wp-cli/wp-cli/pull/2010
#24
in reply to:
↑ 20
@
9 years ago
Replying to kovshenin:
PHP Fatal error: Cannot declare class WP_Http, because the name is already in use in /home/kovshenin/htdocs/develop.lo/src/wp-includes/class-http.php on line 15
Good news! The vast majority of plugins that include class-http.php
as well as http.php
are either using include_once()
, or wrapping it in a class_exists( 'WP_Http' )
check.
These were the only two plugins I found that would likely run into the same bug.
clicksold-wordpress-plugin rrze-access
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#28
@
9 years ago
Fixed in wp-cli here: https://github.com/wp-cli/wp-cli/pull/2011
#30
@
9 years ago
rrze-access
hasn't been touched in 3 years and forks wp-settings
in a way that I think voids his warranty
#31
@
9 years ago
Would it make sense to wrap the moved classes into class_exists
and not break anything at all?
#32
@
9 years ago
WP_Http
is the only class loading that was "moved". Everything else loads from the exact same place. The internals of classes in the class-http.php
pre-4.4 require some functions from http.php
, like wp_http_validate_url()
- so loading just the class in a strange environment where someone thought these files are sandboxed modules would have broken in the right scenario anyway, once it reached one of those functions that wasn't loaded.
#55
@
9 years ago
Some documentation should be updated to point to new files. In attached patch I tried to catch everything.
#57
follow-up:
↓ 59
@
9 years ago
33413.3.diff handles the Customizer, and I promised @westonruter that I would do a breakdown before moving forward. Follows the same tenets/principles as above. Makes the code a LOT easier to understand. Existing files are way too huge.
Highlights:
- Moves classes to
wp-includes/customize/
_wp_customize_include()
now loadswp-includes/customize-functions.php
andwp-includes/customize/class-wp-customize-manager.php
- I found no use cases of these files being loaded by plugins: https://github.com/search?utf8=%E2%9C%93&q=user%3Awp-plugins+class-wp-customize&type=Code&ref=searchresults .... compared to: https://github.com/search?utf8=%E2%9C%93&q=user%3Awp-plugins+class-http&type=Code&ref=searchresultsWP_Customize_Manager::__construct()
loadswp-includes/customize.php
, which is a manifest that loads all of the files inwp-includes/customize/*
- this could probably change to not have to do this. The file should just load intheme.php
. In the past, it looked like the__construct()
was just loading a few classes, but each of those files was loading in aggregate 3 dozen classes.
Screenshot above shows new files.
#59
in reply to:
↑ 57
;
follow-up:
↓ 60
@
9 years ago
Replying to wonderboymusic:
33413.3.diff handles the Customizer, […]
_wp_customize_include()
now loadswp-includes/customize-functions.php
andwp-includes/customize/class-wp-customize-manager.php
- I found no use cases of these files being loaded by plugins: https://github.com/search?utf8=%E2%9C%93&q=user%3Awp-plugins+class-wp-customize&type=Code&ref=searchresults .... compared to: https://github.com/search?utf8=%E2%9C%93&q=user%3Awp-plugins+class-http&type=Code&ref=searchresults
I've actually required it directly in my plugins, however, this was done specifically in PHPUnit tests: https://github.com/xwp/wp-customize-widgets-plus/search?utf8=%E2%9C%93&q=class-wp-customize-manager.php&type=Code
If you search GitHub across all repos, you'll see many examples of this (though many are just clones of Core): https://github.com/search?l=php&q=class-wp-customize-manager.php&type=Code
I suppose it is acceptable to break plugin unit tests with this re-organization if this is all that breaks… but, loading other class files is not limited to unit tests. I see examples of directly loading the control class file require_once( ABSPATH . WPINC . '/class-wp-customize-control.php' )
:
- https://github.com/nathancorbier/condemnationlaw/blob/631af9ba9a9cf9b2b09c58e3c9ead930081b55a4/wp-content/themes/stacy/includes/class-fonts-customizer.php#L2
- https://github.com/xwp/wp-settings-revisions/blob/231ba0ff2991d15980c9ec34c180f20b26dea489/php/meta-control.php#L3
- https://github.com/engrnvd/Wordpress-Sandbox/blob/ac31a58db97df411b3dc3b698ad80379907bb93b/wp-content/themes/dw-fixel/inc/class-customization.php#L2
These plugins would all break.
#60
in reply to:
↑ 59
@
9 years ago
Replying to westonruter:
If you search GitHub across all repos, you'll see many examples of this (though many are just clones of Core): https://github.com/search?l=php&q=class-wp-customize-manager.php&type=Code
I suppose it is acceptable to break plugin unit tests with this re-organization if this is all that breaks… but, loading other class files is not limited to unit tests. I see examples of directly loading the control class file
require_once( ABSPATH . WPINC . '/class-wp-customize-control.php' )
:
- https://github.com/nathancorbier/condemnationlaw/blob/631af9ba9a9cf9b2b09c58e3c9ead930081b55a4/wp-content/themes/stacy/includes/class-fonts-customizer.php#L2
- https://github.com/xwp/wp-settings-revisions/blob/231ba0ff2991d15980c9ec34c180f20b26dea489/php/meta-control.php#L3
- https://github.com/engrnvd/Wordpress-Sandbox/blob/ac31a58db97df411b3dc3b698ad80379907bb93b/wp-content/themes/dw-fixel/inc/class-customization.php#L2
These plugins would all break.
What if we introduced the new file and loaded it from the old one for back-compat, meanwhile tossing a _deprecated_file()
notice, then delete the old file a few releases down the road, giving time for devs to update?
#61
@
9 years ago
@westonruter @DrewAPicture I think I can do a Take 2 on this that doesn't churn so much
This ticket was mentioned in Slack in #core by sergey. View the logs.
9 years ago
#72
in reply to:
↑ 71
@
9 years ago
Replying to wonderboymusic:
After this, one "This action is documented in" should be updated.
#113
@
9 years ago
@wonderboymusic: I'm caught up with your reorganization changes in terms of file header changes as of [34440] :-)
This ticket was mentioned in Slack in #core by drew. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by sergey. View the logs.
9 years ago
#116
@
9 years ago
- Resolution set to fixed
- Status changed from assigned to closed
Let's call this fixed and do new tickets for moving any remaining classes, including the Customizer split outlined in comment:57.
#117
@
9 years ago
A few months ago I told @wonderboymusic that I liked the direction here, but that I questioned the need to have such a complicated include path, and that I was probably going to go
So rather than:
wp-settings.php -> post.php -> post-functions.php -> class-wp-post.php
I think we should have:
wp-settings.php -> post.php -> class-wp-post.php
Performance-wise: If we're going to have all of these files, we should at least try to go with as flat of an include graph as possible.
Back compat-wise: Every file affected by this change is always included in wp-settings.php. Thus, the simplified include graph is only going to break a situation when someone is short-circuiting or forking wp-settings.php. We cannot support this. It puts our feet in too much cement to support this. We have never supported it before. We have split files many times before, including post-3.0 when multisite (and SHORTINIT) came in and made us a bit more cautious. And we have never done any BC work for files that are always included, beyond just putting it in wp-settings.php. If we support this now, then we must always support this, and WordPress will get even harder to maintain. Does this mean we also can't move functions between files? There are many things that we should be accounting for, but certain things we have to agree void the warranty and should not restrict us.
This also avoids the problem noted with class-http.php, though I agree with the sentiment that it won't break much or at all.
So, I'm going to commit a change that moves all (.*)-functions.php
files back to \1.php
and moves the includes up to wp-settings.php. It's not ideal in that wp-settings.php is now quite a bit more loaded up with includes. (I also load up the customize manager with includes -- it could break things, but it's a low risk.) I also move the constants back to rewrite.php. And, I always load the functions before the classes.
#119
@
9 years ago
Why does r35718 need to happen hours before RC, and not earlier in the cycle when developers can appropriately address fallout?
#121
in reply to:
↑ 118
@
9 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
#122
@
9 years ago
Why does r35718 need to happen hours before RC, and not earlier in the cycle when developers can appropriately address fallout?
For the record, this shouldn't have caused a problem for any developers; WP-CLI is a special child which does things it shouldn't :) Open a ticket to make it not have to do special things and we'll find a way.
#123
follow-up:
↓ 124
@
9 years ago
- Resolution set to fixed
- Status changed from reopened to closed
In 35725:
#124
in reply to:
↑ 123
@
9 years ago
Replying to SergeyBiryukov:
In 35725:
I guess we should really have a build tool that automatically regenerates these doc comments. :0)
#125
@
9 years ago
- Resolution fixed deleted
- Status changed from closed to reopened
r35718 causes a fatal for our use case. We extend Walker_Category_Checklist to create a Walker_Radio_Checklist class and before extending we had this require:
require_once ABSPATH . 'wp-admin/includes/template.php';
prior to r35718 the backward compat added by @wonderboymusic took care of the require. But r35718 not only breaks but causes fatals. Easy fix, but it does seem to be the kind of thing that other developers might run into.
I agree with @danielbachhuber that making this kind of adjustment so close to RC is kind of hairy.
#126
@
9 years ago
iiuc, if the user updates to WP 4.4 before updating their plugins, and if they have an active plugin, or theme, or custom code that extends Walker_Category_Checklist, where it previously just worked to require template.php [like in this example], then they will get a fatal error when they try to load up the WP admin.
@nerrad if you push your simple fix and release an update to your plugin right now, a percentage of users will not update the plugin before they update WP to 4.4. Won't that put some users in a tough spot when they get a fatal error right after they update to 4.4?
#127
@
9 years ago
Yes they'll be in a tough spot. Thing is, the Walker classes aren't explicitly loaded via wp-settings.php
... I'm not sure what the "correct" way is we should be requiring a file containing a Walker class we want to extend?
#128
@
9 years ago
It sounds like in this case, the plugin is _doing_it_wrong()
. No plugin should need to require a core file manually, either it's for use within the admin and should include the plugins file on admin_init
, or it's for use on the front end, and should either a) file a ticket to move it to wp-includes
and always load, or b) copy the core functionality needed to their own file.
In this case, Walker_Category_Checklist
is a very minimal class, I'd suggest the proper way would be to just copy the minimal required functionality into your own Walker class.
#129
@
9 years ago
The definition of _doing_it_wrong()
in this case can definitely be argued. However my concern is that this is such a severe breaking change that regardless of whether the code I have is wrong, we're putting WordPress users in the middle of this. They'll update to WordPress 4.4 and will immediately get a broken admin. I hope I'm the only one doing things wrong in a plugin, but it could be a nightmare if there are others out there doing something similar.
At the very least, please DON'T release this so late in this dev cycle. It would be a totally different story if this breaking change had been added in the beginning of the cycle, with the arguments given for why its needed, and then we (and possibly others) would have time to fix our wrong code and get releases out to users.
For what its worth, I *love* the changes made in this ticket to separate out classes and bring some order to the chaos :) Its just unfortunate that the last commit introduces potential pain for a number of WordPress users
This ticket was mentioned in Slack in #core by helen. View the logs.
9 years ago
#131
in reply to:
↑ 130
@
9 years ago
- Keywords dev-feedback removed
- Owner changed from wonderboymusic to nacin
- Status changed from reopened to assigned
- Type changed from enhancement to task (blessed)
Replying to slackbot:
This ticket was mentioned in Slack in #core by helen. View the logs.
[4:12 PM] nacin: So yeah, I can handle the fix for template.php in the admin.
All yours.
This ticket was mentioned in Slack in #core by wonderboymusic. View the logs.
9 years ago
#134
follow-up:
↓ 135
@
9 years ago
Thanks @helen for getting this resolved. However for clarification you made this comment:
While these classes are intended for admin use, ...
wp_dropdown_categories
uses Walker_CategoryDropdown
, is it therefore only a function that should be used in the admin? Since it's parent file, category-template.php
is found in wp-includes
and not wp-admin
it seems reasonable to me that developers would assume Walker classes would be exposed for use in places other than the admin ;)
#135
in reply to:
↑ 134
@
9 years ago
Replying to nerrad:
wp_dropdown_categories
usesWalker_CategoryDropdown
I'm not really following - the two classes referenced here are Walker_Category_Checklist
and WP_Internal_Pointers
.
#136
@
9 years ago
Duh, yeah sorry... you weren't making a generalized statement about walker classes in general, just those classes you moved in your commit.
I'd probably add a block to include the classes in their original location if they're undefined, in case people are manually including individual files.
ie, in
query.php
:The massive churn is not my favourite thing in the world, especially for some of the more complex classes that regularly need SVN log archeology to find out why a part works like it does.