WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 6 weeks ago

#24672 reopened enhancement

Remove final from WP_Post class

Reported by: carlalexander Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.5
Component: Posts, Post Types Keywords: 2nd-opinion
Focuses: Cc:

Description

After discussing it with stephdau and reading through #21309, I think a discussion should be had on the validity of using the final keyword on the class.

While I agree that a decorator pattern is probably best for building the class, there is still no reason for the keyword to be used. If someone wants to extend the class then they should be allowed to do so.

Change History (28)

#1 @toscho
5 years ago

  • Cc info@… added

#2 @johnbillion
5 years ago

  • Cc johnbillion added

#3 follow-up: @nacin
5 years ago

I think we are waiting for WP_Post to be joined by a WP_Comment and WP_Term, and then for all four classes (along with WP_User) to actually be decorated with methods. Removing that final makes it a free-for-all that will make it painful if not nearly impossible to make changes that don't break plugins. Very simply, final is the only thing that lets us turn this into a real API while not worrying about back compat. (This class was originally just for caching and sanitization sanity, not API.)

#4 @nacin
5 years ago

  • Version changed from trunk to 3.5

#5 @carlalexander
5 years ago

What version are those classes planned for? Extending this class is a pretty advanced use case for a plugin developer. They should be able to handle the back compat issues when the pattern gets implemented.

That said. If it's planned for 3.7, then that's not a big deal, but if it's a very long term plan. I'd take it off and amend the class doc to make note of the fact that this is not the final implementation of the class.

#6 @nacin
4 years ago

  • Component changed from General to Post Types

This ticket was mentioned in IRC in #wordpress-dev by danielbachhuber. View the logs.


4 years ago

#9 in reply to: ↑ 3 @SergeyBiryukov
3 years ago

Replying to nacin:

I think we are waiting for WP_Post to be joined by a WP_Comment and WP_Term, and then for all four classes (along with WP_User) to actually be decorated with methods.

Related: #32619

#10 @nacin
3 years ago

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

We will certainly not forget to do this when the time comes. Closing.

#11 @Gerkin
2 years ago

  • Type changed from defect (bug) to enhancement

Hi WordPress team, I'd like to extend WP_Post in my plugins too. Why don't you plan to create a new class for the junction with WP_Term, WP_Comment, etc etc, and let us extend WP_Post with plugin-specific logic?

Thank you for your attention.

#12 @schrapel
19 months ago

@nacin this is no longer blocked, right?

#13 @schrapel
19 months ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

#14 @Jabawack
19 months ago

Any news about this ticket?

#15 @netweb
18 months ago

  • Milestone set to Awaiting Review

Moving reopened tickets without a milestone back to Awaiting Review for review

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


11 months ago

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


8 months ago

#18 @Andy Schmidt
2 months ago

  • Keywords 2nd-opinion added

With the addition of the "posts_pre_query" hook, you are now inviting developers to populate posts with information sourced elsewhere - some might even come from external databases. Consequently, it would be very sensible to properly EXTEND the WP_Post class and substitute the necessary functionality/compatibility to the individual post.

Alternatively, if you are completely opposed to allowing WP_Post to be extendable, would you at least consider creating an Interface, then have WP_Post implement your interface. In those few places in the WP code where "is_a" or "instanceof" is used against the class WP_Post, change it to "instanceof Interface". This way developers can create their own classes that do properly implement your interface, and your "instanceof Interface" will still recognize compatible object instances, without you having to remove "final" from your class.

Right now, you are forcing developers to use PHP's "Componere" (or similar extensions) to bypass your "final" class declaration to do what's necessary. So your "final" declaration doesn't really accomplish anything as far as preventing dependencies.

#19 @hawaiipersson
2 months ago

I've noticed that the reluctancy to this change may have a lot to do with avoiding a situation where changes to WP_Post might break backwards compatibility for any plugins using this feature.

As much as I understand that I do believe (as a plugin developer) that the downsides are greater. On larger sites a Wordpress upgrade will always be subject to extensive testing to make sure nothing breaks. Also it seems impossible to make any kind of changes with the constant fear of breaking backwards compatibility. It happens, and it's up to the plugin / theme developers to keep up.

In any case, "automatic" upgrades of the Wordpress core should prevent the upgrade if any of the installed plugins are not compatible with the new version (but I guess that's a different issue).

Why would you want to extend \WP_Post?

I've got two different, real life, examples:

First we use Wordpress admin gui as a system for creating and maintaining content that is sent to a different app for storage and rendering. The object sent to the external app consists of both core WP_Post properties, as well as a lot of meta data associated with the post. If I could extend WP_Post I would be able to collect all data for the object in a neater way.

Second - we are experimenting with using different rendering engines to actually render the front end (personally I like Twig, and strongly feel that WP must move in a direction where business logic and design in not mangeled together). When using Twig all data must be collected (and filtered) first, before the rendering process starts. It would help a lot if all data needed to render a specific post could be included in one single object. As of now the separation makes the data structure very unintuitive for our frontend devs.

There are of course solutions to both problems above. But development is not only about making this work, but to also make it beautiful and maintainable. In my opinion, making \WP_Post final prevents that.

Last edited 2 months ago by hawaiipersson (previous) (diff)

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


2 months ago

#21 follow-up: @jorbin
2 months ago

What happens when a plugin adds a method that returns unescaped data and handles the escaping later and later core adds a method with the same name where it is returned escaped? A 2nd plugin using that method could introduce a security vulnerability since it assumes the data is escaped by core.

#22 follow-ups: @MikeSchinkel
2 months ago

@jorbin Core should not add a method that returns escaped data...

Alternately core could follow a naming convention that is likely to not be one that a plugin would ever implement unescaped, for example:

$post->the_title_attr()    <-- Sanitizes with esc_attr()
$post->the_content_html()  <-- Sanitizes with wp_kses_post()
$post->the_permalink_url() <-- Sanitizes with esc_url()

Basically anything that begins with a the_ is a template tag and should echo escaped values but anything that just returns a value is never escaped (we have used this convention for many years, and it works brilliantly.)

That said, I am more and more concerned about my prior advocacy for this. It only works to subclass WP_Post when there can be only one, e.g. that the code is in the theme or specific to a site.

Beyond that if two plugins subclass a WP_Post and both plugins want to use in the same context — such as in a theme template page — only one of them can win.

So I am thinking that to extend a WP_Post we should use action and filter hooks instead of subclassing, and inspecting 'supports' to be the WordPress equivalent of for testing a class for an interface.

See my proposal here.

#23 in reply to: ↑ 22 ; follow-up: @hawaiipersson
8 weeks ago

Replying to MikeSchinkel:

That said, I am more and more concerned about my prior advocacy for this. It only works to subclass WP_Post when there can be only one, e.g. that the code is in the theme or specific to a site.

Beyond that if two plugins subclass a WP_Post and both plugins want to use in the same context — such as in a theme template page — only one of them can win.

So I am thinking that to extend a WP_Post we should use action and filter hooks instead of subclassing, and inspecting 'supports' to be the WordPress equivalent of for testing a class for an interface.

See my proposal here.

If a plugin where to enforce the use of its class directly in a theme that would be a question of bad plugin design, rather than a weakness for the extendability in itself. In my opinion plugin authors should never introduce code in the theme that would break the site it the plugin fails or is deactivated. In fact, the high use of custom functions from plugins called in themes is one of the biggest problems with Wordpress today, in my opinion.

Plugins should be encouraged to only introduce content modifications through filters and action hooks. With other words, even though I understand your concern, I do not believe that this should be dealt with by Wordpress core.

@jorbin

What happens when a plugin adds a method that returns unescaped data and handles the escaping later and later core adds a method with the same name where it is returned escaped? A 2nd plugin using that method could introduce a security vulnerability since it assumes the data is escaped by core.

Sorry but I don't get this. A second plugin would instantiate \WP_Post, not a plugins extension of it - would it not? Without (bad design) dependencies between plugins this would never happen. When calling \WP_Post methods the return format would always be as expected. Extending a class doesn't change the behavior of the core class itself.

Also it would be a security vulnerability introduced by the plugin, and should therefor be handled by the plugin. Wordpress core can not and should not raise concerns only because there are bad plugin authors :)

#24 in reply to: ↑ 23 @MikeSchinkel
8 weeks ago

Replying to hawaiipersson:

If a plugin where to enforce the use of its class directly in a theme that would be a question of bad plugin design, rather than a weakness for the extendability in itself.

I don't understand what you are trying to say here. If a plugin subclasses WP_Post how would it use it in theme templates in without conflicting with another plugin that used the same approach? IOW, what is your use-case for a subclass?

In my opinion plugin authors should never introduce code in the theme that would break the site it the plugin fails or is deactivated. In fact, the high use of custom functions from plugins called in themes is one of the biggest problems with Wordpress today, in my opinion.

You are thinking of only selected scenarios and selected use-case. Please understand there are other scenarios and use-cases than the ones you are focused on.

There are very valid scenarios and use-cases where code in the theme should be dependent upon a plugin. Those scenarios are custom "applications" where themes are developed specifically for a "(typically vertical market) application" plugin, and complex sites with a lot of custom functionality. And in post cases the plugin should be a must-use plugin so deactivation is not possible (btw, I don't understand the concept of a plugin "failing"; sounds like a plugin with a bug to me.)

In other words when sites are developed and deployed by professional developers and sysadmins vs. by end users then intertwining themes and plugins is not a concern.

Plugins should be encouraged to only introduce content modifications through filters and action hooks. With other words, even though I understand your concern, I do not believe that this should be dealt with by Wordpress core.

This is where I am especially confused. You say people "should be encouraged to only introduce content modifications through filters and action hooks" but then when I suggest adding filters and action hooks to WP_Post you argue that "this should not be dealt with by Wordpress core?" Your two statements seem to contradict each other?

#25 in reply to: ↑ 22 ; follow-up: @Andy Schmidt
7 weeks ago

Replying to MikeSchinkel:

Beyond that if two plugins subclass a WP_Post and both plugins want to use in the same context — such as in a theme template page — only one of them can win.

I'm not certain that I understand your concern (but that might be due to my lack of experience).

The possibility of an ill-behaved plug-in interfering with another plug-in already exists today with the "posts_pre_query" hook, and always does so in many other hooks.

Specifically, if the first plug-in decides it needs to populate the $posts array with WP_Post instances - and the next plug-in simply assumes to be the "only one" and DISCARDS the existing array and returns a hard-coded "null", or substitutes its own array - then it doesn't matter if the so discarded post instances had been created by the native WP_Post class or any subclass thereof?

Being able to properly sub-class WP_Post does not alter the potential of a conflict?

That said, I am more and more concerned about my prior advocacy for this. It only works to subclass WP_Post when there can be only one, e.g. that the code is in the theme or specific to a site.

If at any given time a plug-in or theme decides to hook into "posts_pre_query" and conditionally instantiate its own WP_Post subclass to emulate external data as "posts", or to handle a certain Custom-Post-Type, then it would do so after narrowly defining the case in which this occurs.

That wouldn't prevent a different plug-in to equally focus on its particular type of posts before creating its specialized WP_Post subclass.

Once the different instances have been created by their respective plug-ins, the only thing that matters is that they will be recognized as "instance of" WP_Post throughout the subsequent core code. This way method calls and property references by the core will continue to function throughout the subsequent code like as if they were instances of the native WP_Post class.

#26 in reply to: ↑ 21 @Andy Schmidt
7 weeks ago

Replying to jorbin:

when a plugin adds a method that returns unescaped data and handles the escaping later and later core adds a method with the same name where it is returned escaped

That sounds more like "name spacing" issue? That same potential exists if a plug-in creates a regular function in the root name space and later WordPress adding a function by that same name?

If developers add methods or properties, it is their responsibility to choose member names for which there is a reasonable expectation that they are highly unique to avoid future name clashes.

And as far as escaping - I've come across filters/hooks in WordPress that expressly instruct developers that the return value should be escaped. So the potential of someone not reading carefully (or copying imperfect code samples somewhere) and NOT escaping the return values is also not specific to the ability to extending the WP_Post class?

Last edited 7 weeks ago by Andy Schmidt (previous) (diff)

#27 @MikeSchinkel
6 weeks ago

I posted #43740 as an alternative to this ticket.

#28 in reply to: ↑ 25 @MikeSchinkel
6 weeks ago

Replying to Andy Schmidt:

The possibility of an ill-behaved plug-in interfering with another plug-in already exists today with the "posts_pre_query" hook, and always does so in many other hooks.

Yes. But using hooks it is possible for a site developer to fix the incompatibility without having to resort to modifying either plugin.

If two plugins both inherit from WP_Post you can use the enhancements from either child class but you cannot use both since PHP does not support multiple inheritance.

I guess you could create a 3rd class that could contain both of those instances and delegate, but that would only work for methods or properties that don't exist, not for ones that do, and PHP does not allow you to fully simulate a contained class in the container, for example method_exists() and property_exists() will return false for virtualized methods and properties.

then it doesn't matter if the so discarded post instances had been created by the native WP_Post class or any subclass thereof?

Unless there are hooks like 'save_post' that expect the post instances to have the additional methods and properties added by its plugin's child class.

then it would do so after narrowly defining the case in which this occurs.

What ensures that it would only do so narrowly?

the only thing that matters is that they will be recognized as "instance of" WP_Post throughout the subsequent core code.

This is an op-ed aside, but interesting in other areas of computing there has been a recognition that logic based on identity is much more fragile than logic based on attributes and behavior. For example, most of us know it is better in JS to test browsers for behavior than to maintain a list of browsers that are compatible. But in PHP it seems people still think "instance of" is a good solution.

A better solution would be for WP_Post to use the 'supports' attribute passed in during register_post_type to determine if a post has a specific _"supports"_ value. (This is basically dynamic interfaces.)

Along these lines #43740 proposes to add a 'wp_post_supports_instance' hook to allow a plugin to add a contained class to each instance of WP_Post to contain custom functionality.

If developers add methods or properties, it is their responsibility to choose member names for which there is a reasonable expectation that they are highly unique to avoid future name clashes.

The problem is in a shared code ecosystem where "responsibilities" are not policed, such as the WordPress plugin repo, too many developers will use common names that will clash. So it is the responsibility of the core developers to guard against (likely unintentional) bad behavior. Hence final.

Last edited 6 weeks ago by MikeSchinkel (previous) (diff)
Note: See TracTickets for help on using tickets.