WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#32845 closed feature request (wontfix)

Introduce wp_include()

Reported by: johnjamesjacoby Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Plugins Keywords: close
Focuses: Cc:
PR Number:

Description

What if WordPress included a wrapper function for including & requiring files, that could be filtered by any plugin or theme to make everything fully modular, pluggable, and decoupled.

This function would replace all include/_once() & require/_once() calls, allowing for the complete override of core files without hacking or replacing huge swaths of core functionality.

Patch with the function and an imaginary use-case imminent.

Attachments (2)

32845.01.patch (10.5 KB) - added by johnjamesjacoby 4 years ago.
32845.02.patch (10.8 KB) - added by johnjamesjacoby 4 years ago.
require/include constructs are not functions

Download all attachments as: .zip

Change History (28)

#1 @johnjamesjacoby
4 years ago

No doubt that this function will slow down one of the fundamentally speedy core constructs of PHP. Benchmarks would probably be helpful to prove how bad this change would actually be.

That said, the multitude of filters applied during template loading process already suffer similar consequences for the sake of flexibility. My thinking is, that it would be really great of the rest of WordPress were just as flexible.

#2 follow-up: @bordoni
4 years ago

Just a small question, can we avoid the usage of extract here?

@johnjamesjacoby
4 years ago

require/include constructs are not functions

#3 in reply to: ↑ 2 ; follow-up: @johnjamesjacoby
4 years ago

Replying to bordoni:

Just a small question, can we avoid the usage of extract here?

We can, and I advocate strongly (and regularly) against the use of extract() everywhere, but... while I was writing the function, felt like this is exactly the use-case where it does makes sense – where the intention is to override all existing local variables.

I'm completely open to just re-assigning the variables based on the $r array though. Doesn't make a difference to me.

#4 follow-up: @Otto42
4 years ago

Minor problem: If you include/require from within a function, then the content of the included file is not in the global context anymore, but in the function context instead. I feel that this will break quite a lot of things.

#5 follow-up: @wonderboymusic
4 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Which multi-thousand line file would you like to completely override? At that point, you might as well fork the project.

#6 in reply to: ↑ 5 @johnjamesjacoby
4 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Replying to wonderboymusic:

Which multi-thousand line file would you like to completely override? At that point, you might as well fork the project.

I'm so incredibly shocked by this type of response from a release lead, that I'm not even sure how to respond to it.

I'm not going to play the open/closed game here, but I'd like to explore this idea further, and Trac is the place to do it.

If you don't support the idea, or don't find value in it, doesn't mean I don't, or hundreds of others may not if given the opportunity foster respectful discussion. Closing the idea within hours of it coming up is insulting and embarrassing to me, and the project, and makes me super concerned about how future contributors will be treated when sharing their ideas.

Unbelievable.

#7 @mordauk
4 years ago

I like the idea as it would provide developers the ability to replace specific files if they need to, similar to how pluggable functions can be replaced.

I can definitely see this being useful for when plugin A needs to modify plugin B but plugin B has not added adequate filters or actions. The same goes for when a plugin wants to modify an aspect of core that is short on filters and actions.

#8 follow-up: @ocean90
4 years ago

…similar to how pluggable functions can be replaced.

And we all know how bad pluggable functions are. You have zero control over them if a plugin replaces a function. I don't even want to think about what will happen if plugins are starting to override entire core files.

I’d like to know if this change would have any performance impacts.

#9 in reply to: ↑ 8 @mordauk
4 years ago

Replying to ocean90:

…similar to how pluggable functions can be replaced.

And we all know how bad pluggable functions are. You have zero control over them if a plugin replaces a function. I don't even want to think about what will happen if plugins are starting to override entire core files.

I’d like to know if this change would have any performance impacts.

Actions and filters are (for the most part) the same. You cannot control what another plugin does with them.

#10 follow-up: @Otto42
4 years ago

Actions and filters can chain. Multiple plugins can hook into them at the same time. Pluggable functions, and this idea, not so much.

#11 in reply to: ↑ 10 @mordauk
4 years ago

Replying to Otto42:

Actions and filters can chain. Multiple plugins can hook into them at the same time. Pluggable functions, and this idea, not so much.

Yes but one plugin can still disrupt the entire chain by doing something nasty. Just pointing out that it's not all that different.

#12 in reply to: ↑ 4 @johnjamesjacoby
4 years ago

Replying to Otto42:

Minor problem: If you include/require from within a function, then the content of the included file is not in the global context anymore, but in the function context instead. I feel that this will break quite a lot of things.

It will break some things, which IMO are issues with WordPress's application design that should be addressed anyhow.

We could extract $GLOBALS if we feel it is a huge deal, but that's yet another monkey-patch approach that this is trying to give developers the chance to avoid.

We can also cherry-pick the files that don't invoke anything in the global scope, where as files like wp-login.phphave obvious issues.

#13 @boonebgorges
4 years ago

I'm all in favor of taking steps toward a more decoupled WordPress. But given WP's current filesystem design, which is pretty haphazard, per-file overrides seem like a pretty bad way to accomplish it. Introducing a filter like this today will tie our hands a lot in the future. What if we decide, say, that a function in category-template.php should be moved to taxonomy.php? Plugins that are overriding category-template.php will get a fatal error due to a redefined function, while those overriding taxonomy.php will be missing the function altogether.

A truly modular WordPress would be designed for decoupling. Currently, we aren't. We should work toward that before allowing people to turn off arbitrary parts of the system.

#14 follow-up: @ericmann
4 years ago

Actually, I spec'd out a similar patch when we first discussed feature plugins as a path forward for core.

Think of it this way: often we want to test a larger refactoring of core that doesn't lend itself to hooking in with actions and filters. Case-in-point, I had to completely subclass override _the entire class_ to set up my Secure XMLRPC feature plugin. It works, but it means the plugin as it stands now merely _emulates_ how the new feature would work in core.

If includes were modular, then feature plugins could replace entire core files with potential release candidates. When we're ready to fold things in to core, it's a matter of moving the file from the plugin into core. _Much_ faster and more efficient.

Do I see this as a new way to build plugins for general use? Probably not. Do I see this as a way people can hack core without hacking core? Definitely.

Remember, despite our best efforts, not every feature is fully abstracted with actions and filters ...

#15 in reply to: ↑ 3 @ericmann
4 years ago

Replying to johnjamesjacoby:

I'm completely open to just re-assigning the variables based on the $r array though. Doesn't make a difference to me.

Since we explicitly advise against extract() elsewhere, I'd prefer we go through the effort of changing things rather than start adding exceptions to the rule.

#16 in reply to: ↑ 14 @bordoni
4 years ago

Replying to ericmann:

Actually, I spec'd out a similar patch when we first discussed feature plugins as a path forward for core.

If includes were modular, then feature plugins could replace entire core files with potential release candidates. When we're ready to fold things into core, it's a matter of moving the file from the plugin into core. Much faster and more efficient.

One of the many reasons for been able to "filter" when files are Included, I could see usage for things like overwriting files to make it faster on specific cases.

#17 @wonderboymusic
4 years ago

Sorry for my initial response. I was triaging a lot of tickets that day and made a flippant comment. Everyone deserves to have their ideas heard. The ticket is open for discussion.

#18 follow-up: @jorbin
4 years ago

  • Keywords close added

While this is an interesting idea that would improve the flexibility of WordPress, I think it is is impractical. If this went in, we would never be able to deprecate a function and move it to deprecated.php or reorganize any functions across files since it might cause a function redeclared fatal error. The flexibility given to plugins and themes would reduce flexibility in core. I don't think that tradeoff is worth it and thus suggest we close this as wontfix.

If an individual needs to be able to declare there own version of an include, they can modify the php include path to accomplish that (but i would caution them to be very careful in doing so and would strongly discourage doing so in a publicly released plugin).

#19 @nacin
4 years ago

Hi @jjj,

Thanks for the contribution to WordPress. This isn't something we're going to accept into core at this time.

We've had a fairly miserable, decade-long experience with overloading code, in the form of both pluggable functions and drop-in files. Neither of them worked out favorably. We no longer add pluggable functions, and drop-ins are often more trouble than they are worth, especially when allowing for further development.

The wp-db.php and cache.php files are, in particular, incredibly painful to make changes to. Our security-related work that tore up wp-db.php over the last year was significantly and negatively impacted by the existence of and specific implementation of drop-ins, for example. Improvements to caching in core have likewise been hampered.

We have a significant amount of technical debt in WordPress specifically designed around crazy things that plugins do or have done, or core has enabled or done itself. This already hamstrings development to some extent. This would put massive additional unforeseen burdens on contributors, as pretty much everything could break with any change (more than it already could).

Pluggable functions alone are a great case study for why this cannot work. They were bite-sized and it was still too much to support new development, functionality, and even minor bug or security fixes. It was clear in 2010, when we stopped adding pluggable functions, that the proper path forward was to ensure we were developing for modification in smart, targeted ways, through actions and filters. (For some time, as just one major example, the entire customizer could be disabled by removing a single action, though it has become much more integrated into WordPress in ensuing releases.)

The core committers simply cannot take on the additional mental burdens that this will result. If for nothing else, this won't be accepted. Thanks again for the contribution.

Last edited 4 years ago by nacin (previous) (diff)

#20 in reply to: ↑ 18 ; follow-up: @nacin
4 years ago

Replying to jorbin:

they can modify the php include path to accomplish that (but i would caution them to be very careful in doing so and would strongly discourage doing so in a publicly released plugin).

This won't work due to efforts to avoid implicit include_path usage in #12594, #17092.

#21 in reply to: ↑ 20 @jorbin
4 years ago

Replying to nacin:

Replying to jorbin:

they can modify the php include path to accomplish that (but i would caution them to be very careful in doing so and would strongly discourage doing so in a publicly released plugin).

This won't work due to efforts to avoid implicit include_path usage in #12594, #17092.

I stand corrected.

#22 @nacin
4 years ago

I'll also point out that "you might as well fork the project" is, from a technical level, precisely what this would allow for, but in a much less stable way. Better off keeping your own branch. Honestly! If this is what you want to do, maintain your own fork. It didn't come off as @wonderboymusic intended, but it's a valid suggestion.

#23 @dougal
4 years ago

Even though it seems that this will not move forward anyways, I just wanted to point out some flaws in the patch, as presented above (in case anybody is still thinking of experimenting with the idea):

The wp_include() function was defined in WPINC/plugins.php, but the patch attempted to use the new function several times before plugin.php is even loaded.

Obviously, the function could be moved. But even then, regular plugins aren't going to be able to affect a whole lot with it, because they aren't loaded until much later. The sunrise.php drop-in can do more, but still wouldn't be loaded early enough to allow filtering of several of the core includes.

@nacin has already pointed out the reasons why filterable includes are probably not a good approach (because it could create more technical debt and potential for compatibility breakage).

A better path forward would probably be to continue to create new filter hooks, and where possible, design new/refactored functionality with an eye towards interfaces that allow Dependency Injection.

EDIT: For the record, at first, I was on-board with the basic idea. But I got caught up in the question about performance. I was going to suggest a different approach -- creating a dictionary of include files, filtering that, then doing something like require($_WPFILEDICT['formatting']). Then I realized the precedence problems I mentioned. Then I saw @nacin cast a wider net and point out the larger problem.

Last edited 4 years ago by dougal (previous) (diff)

#24 @johnjamesjacoby
4 years ago

Thanks everyone that gave feedback so far. I have some responses that I'll bullet below, because I find Trac's quote styling to be pretty terrible and difficult to grok.

  • BuddyPress & bbPress are both *fully* pluggable, in that even the base class can be unhooked and replaced (as if wp(); could do the same thing.)
  • The file inclusion vs. load order is an issue. Ideally plugin.php would be moved higher up to be more accommodating in this regard (I had originally had it hacked around more, but wanted to minimize the footprint for the sake of making the intent a bit more clear.)
  • In my experience, rarely if ever do functions relocate anyways, for the reasons stated in this ticket. See also #21788 where I've championed something fairly innocuous for 3 years. This function makes where functions live less important, IMO, and instead forces everyone to consider the WordPress development environment for exactly what it's always been: unpredictable.
  • Actions and filters are creating technical debt, and a library and lexicon of calculated intercept points that need some kind of blessing and blue moon to make happen. See #23016.
  • Being told to fork WordPress one day, and being steered away from forking BackPress another, is I guess a topic for my blog and not for Trac, but consider that it seems there's no third option, which is make WordPress more flexible. Cake or death, basically.
  • It's fine with me if the core team thinks this is truly a horrible idea and wants to tuck it under the rug, but I'm still not sure I completely agree with that assessment, and would like to see/help the project get to a more flexible and decoupled place.
  • The pluggable nature of BuddyPress & bbPress has not introduced any additional burden on those respective teams, nor has it pigeonholed us from improving the software over time. If anything, no one even knows it's possible to do.

Feel free to close this again if y'all want. I'll figure out another solution that is less impactful to core.

Last edited 4 years ago by johnjamesjacoby (previous) (diff)

This ticket was mentioned in Slack in #core by jacobsantos. View the logs.


4 years ago

#26 @jorbin
4 years ago

  • Resolution set to wontfix
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.