Unchecked property overloading is detrimental to OOP.

[aercolino]

Original Summary: Unchecked property overloading is detrimental to OOP.

​Property and ​Method overloading (allowed by magic methods __get, __set, __isset, __unset, and __call) have been thoroughly introduced into WP code for Backward Compatibility, maybe starting from Changeset/21512, but mostly since version 4.0.0.

The copy-&-paste-d code I'm referring to is:

        /**
         * Make private properties readable for backwards compatibility.
         *
         * @since 4.0.0
         * @access public
         *
         * @param string $name Property to get.
         * @return mixed Property.
         */
        public function __get( $name ) {
                return $this->$name;
        }

        /**
         * Make private properties settable for backwards compatibility.
         *
         * @since 4.0.0
         * @access public
         *
         * @param string $name  Property to set.
         * @param mixed  $value Property value.
         * @return mixed Newly-set property.
         */
        public function __set( $name, $value ) {
                return $this->$name = $value;
        }

        /**
         * Make private properties checkable for backwards compatibility.
         *
         * @since 4.0.0
         * @access public
         *
         * @param string $name Property to check if set.
         * @return bool Whether the property is set.
         */
        public function __isset( $name ) {
                return isset( $this->$name );
        }

        /**
         * Make private properties un-settable for backwards compatibility.
         *
         * @since 4.0.0
         * @access public
         *
         * @param string $name Property to unset.
         */
        public function __unset( $name ) {
                unset( $this->$name );
        }

        /**
         * Make private/protected methods readable for backwards compatibility.
         *
         * @since 4.0.0
         * @access public
         *
         * @param callable $name      Method to call.
         * @param array    $arguments Arguments to pass when calling.
         * @return mixed|bool Return value of the callback, false otherwise.
         */
        public function __call( $name, $arguments ) {
                return call_user_func_array( array( $this, $name ), $arguments );
        }

Unchecked overloading (i.e. lazy programming) is detrimental to OOP because it neutralizes all protections. No matter what, all properties and methods of classes with unchecked overloading are always public.

I mean, not only legacy properties and methods that existed before introducing unchecked overloading but also all others that were, are being and will be introduced afterward and really meant to be private or protected.

This is a very important issue, whose implications will be appreciated in the future. Currently, it all seems fine, but it's an illusion. As time goes by, on one side the WP core will continue to migrate to PHP5 (and its access rules) and on the other side theme and plugin hackers will continue to squeeze WP as much as possible. When core developers introduce protection for new properties and methods, they expect that protection to be taken into account by the ecosystem, and finally by theme and plugin developers, which are the ones those protections were thought for. BUT if WP theme plugin and core developers will always be able to access whatever they want as they please, then I wonder why PHP5 protections were introduced in the first place.

Here is a tedious but simple fix (i.e. checked overloading):

if (! array_key_exists($name, $this->legacy_properties)) {
  $trace = debug_backtrace();
  trigger_error('Undefined property' ..., E_USER_NOTICE);
}
// rest of __get, __set, __isset, __unset code here

if (! array_key_exists($name, $this->legacy_methods)) {
  $trace = debug_backtrace();
  trigger_error('Call to undefined method' ..., E_USER_ERROR);
}
// rest of __call code here

If this issue is not fixed ASAP, developers will find these "backdoors" and start to use them. Then it gets worse.

[jdgrimes]

Related: #22234

In a comment on that ticket, @nacin mentioned throwing deprecated notices for the legacy private/protected properties. I think that's a good idea, though we'd need to implement something like this before we'll know which properties are legacy. Possibly we should followup this ticket with another for giving deprecated notices for the legacy properties and methods.

[aercolino]

In my suggested fix, legacy properties and methods are all the public members of each class before unchecked overloading was introduced into that class. Deprecation notices occurring when legacy non-public members are accessed from outside the hierarchy is a nice-to-have feature for synchronizing real usage with stated code and documentation along time, however that's not the issue I'm referring to here. Even without such notices, i.e. even allowing public access to legacy members forever, unchecked overloading should be fixed.

[jdgrimes]

Replying to aercolino:

In my suggested fix, legacy properties and methods are all the public members of each class before unchecked overloading was introduced into that class. Deprecation notices occurring when legacy non-public members are accessed from outside the hierarchy is a nice-to-have feature for synchronizing real usage with stated code and documentation along time, however that's not the issue I'm referring to here. Even without such notices, i.e. even allowing public access to legacy members forever, unchecked overloading should be fixed.

I agree—that's why I said maybe it could be a follow up to this. :-)

[nacin]

I 100% agree with this and it's been on my mind to do this for a while. I didn't bother to follow up and do it when it landed, because I knew we could adjust it later. But yes, sooner than later is better.

These should only work for a whitelisted set of properties, and it should throw an incorrect usage notice (_doing_it_wrong()) when a property is accessed, set, or unset.

We should be able to come up with some basic code to do this. One more method and one more property for each class. This would work best in a trait but we can't use those yet.

This will be pretty tedious — patches welcome if someone wants to take a crack at it. :-)

[wonderboymusic]

I am going to go through each class one by one right now and see what I can see.

For starters, take Custom_Background: 2 of the properties were marked private (following the @param annotation), but they are used as add_action callbacks, which means they have to be public.

I will try to correct usage before adding any _doing_it_wrong() logic, etc

[wonderboymusic]

In 31133:

In Custom_Background:

• In [28481], $admin_header_callback and $admin_image_div_callback were set to private based on their erroneous @param value
• $admin_header_callback and $admin_image_div_callback are used as hook callbacks - as such, they must be public
• In [28521] and [28524], magic methods were added for back-compat
• Currently, there are 2 properties marked private, $page and $updated - $page is never used and $updated was added by me in [30186] during 4.1

Set $admin_header_callback and $admin_image_div_callback to public.
Remove the $page property - it duplicated the $page local var and is referenced/used nowhere.
Remove the magic methods - they were beyond overkill and rendered moot by the above changes.

See #30891.

[jdgrimes]

Replying to wonderboymusic:

In 31133:

In Custom_Background:

• In [28481], $admin_header_callback and $admin_image_div_callback were set to private based on their erroneous @param value
• $admin_header_callback and $admin_image_div_callback are used as hook callbacks - as such, they must be public
• In [28521] and [28524], magic methods were added for back-compat
• Currently, there are 2 properties marked private, $page and $updated - $page is never used and $updated was added by me in [30186] during 4.1

Set $admin_header_callback and $admin_image_div_callback to public.
Remove the $page property - it duplicated the $page local var and is referenced/used nowhere.
Remove the magic methods - they were beyond overkill and rendered moot by the above changes.

See #30891.

I think this should be reverted. Reasoning:

• The $admin_header_callback and $admin_image_div_callback don't need to be public, because it isn't themselves that is hooked to an action, but their values. If this was true we'd have experienced Fatal errors all over the place.
• The $page property should be deprecated, but kept for backward compatibility.

Correct me if I'm wrong. :-)

[wonderboymusic]

In 31134:

In Custom_Image_Header:

• In [28481], $admin_header_callback and $admin_image_div_callback were set to private based on their erroneous @param values
• $admin_header_callback and $admin_image_div_callback are used as hook callbacks - as such, they must be public
• In [28521] and [28524], magic methods were added for back-compat
• Currently, there are 4 properties marked private: $uploaded_headers, $default_headers, $page, and $updated - $page and $uploaded_headers are never used and $updated was added by me in [30187] during 4.1. $default_headers does not necessarily need to be private

Set $admin_header_callback and $admin_image_div_callback to public.
Remove the $page property - it duplicated the $page local var and is referenced/used nowhere.
Remove the $uploaded_headers property - it is used nowhere and is dead code.
Set $default_headers to public.
Remove the magic methods - they were beyond overkill and rendered moot by the above changes.

See #30891.

[wonderboymusic]

I think this should be reverted

Hook callbacks get called in global scope, method calls on instances, which means those methods have to be public. The only reason there aren't fatal errors now is the magic methods.

I am the one who added all of those access modifiers in 4.0. Previously, most of them classes used PHP4 syntax which defaulted every method and property to public.

I made a mistake in the way I went about some of these in 4.0.

[jdgrimes]

Replying to wonderboymusic:

I think this should be reverted

Hook callbacks get called in global scope, method calls on instances, which means those methods have to be public. The only reason there aren't fatal errors now is the magic methods. [emphasis added]

Yes, but these aren't methods. They are properties. Properties don't get called. Their values get passed to some other function that calls the function represented by that value.

[wonderboymusic]

I made them private in [28481], that was unnecessary... and every bit of this class was public previously. Do you have a use case for these classes outside of _custom_header_background_just_in_time()?

[jdgrimes]

Replying to wonderboymusic:

I made them private in [28481], that was unnecessary... and every bit of this class was public previously. Do you have a use case for these classes outside of _custom_header_background_just_in_time()?

No, I don't have a use case. I just wanted to clarify that I don't think they have to be made public. If the goal is to remove the magic methods where they aren't absolutely needed, then this makes sense.

[wonderboymusic]

In 31135:

In WP_Text_Diff_Renderer_Table:

• In [28525], $_diff_threshold, $inline_diff_renderer, and $_show_split_view were marked protected; magic methods were also added.
• The magic methods should only perform operations on a whitelisted set of properties, now specified in $compat_fields
• Remove __call(), is unnecessary and can wreak havoc on the parent class.

This class is used in one place: wp_text_diff().

See #30891.

[wonderboymusic]

In 31136:

In WP_MatchesMapRegex:

• Exactly one method was made private in [28516], and is only used internally.
• 2 properties were made private, but they just store variables passed to the constructor.
• Instances of this class are never created in core. WP_MatchesMapRegex::apply() is called statically in WP->parse_request() and url_to_postid().

The chances that:
1) this class is used as an instance somewhere and
2) the properties that have always been marked @access private and begin with _ were used publicly

...is extremely low.

Remove the magic methods, I should not have added them.

While we're at it, use the PHP5-style __construct() instead of the class name.

See #30891.

[wonderboymusic]

In 31137:

In Walker:

• Every subclass of Walker overrides $db_fields and makes it public
• wp_list_comments() accesses ->max_pages on an instance of Walker, it must be public
• $has_children was added as protected in 4.0. doesn't need BC

Make $db_fields and $max_pages public and remove magic methods.

See #30891.

[wonderboymusic]

I think WP_Object_Cache in wp-includes/cache.php can stay the way it is, since the class is replaced completely by drop-ins. Thoughts?

[wonderboymusic]

In 31138:

In WP_Error:

• wp_send_json_error() accesses $errors on an instance, it must be public
• $error_data is a local message cache for error codes and doesn't particularly hide info, would be the only non-public field or method in the class

Make $errors and $error_data public and remove magic methods.

See #30891.

[wonderboymusic]

In 31139:

WP_Ajax_Response has one property only, $responses. It was public until [28508], when it became private in name only. Is it worth 4 magic methods to pretend that this property is private? It is not.

Make it public and remove the magic methods.

See #30891.

[boonebgorges]

Replying to wonderboymusic:

In 31137:

In Walker:

• Every subclass of Walker overrides $db_fields and makes it public
• wp_list_comments() accesses ->max_pages on an instance of Walker, it must be public
• $has_children was added as protected in 4.0. doesn't need BC

Make $db_fields and $max_pages public and remove magic methods.

See #30891.

After [31137], Tests_Comment_Walker::test_has_children() is throwing a fatal error, because $has_children is no longer accessible. It's possible to rewrite the test to account for a protected property like this, but I'm wondering if this property might be accessed in the wild. Just because the docs said that the property is protected in 4.0 doesn't mean that people aren't accessing it. I suggest making it public, or reintroducing a magic method with a whitelist for has_children.

[boonebgorges]

Replying to boonebgorges:

I suggest making it public, or reintroducing a magic method with a whitelist for has_children.

After private correspondence with wonderboymusic, going with the former option.

[boonebgorges]

In 31141:

Walker::$has_children should be public for backward compatibility.

See [31137]. See #30891.

[aercolino]

The fix consisting in removing unchecked overloading altogether and making legacy stuff explicitly public is clean, more WYSIWYG and saves clutter. However, here are some points to consider.

(1) Everything should be made public. Currently, even if modifiers say differently, everything is effectively public and has been for all the months after introducing unchecked overloading. The safest thing to do with that fix would be to indiscriminately declare public everything in a class with unchecked overloading (which is what unchecked overloading disguisedly did). This would be a reasonable fix because it would stop the bleeding, meaning that no fresh properties or methods would suffer the same problem again.

(2) Existing modifiers would be lost. Both before and after introducing unchecked overloading, core developers have been adding proper modifiers, not only expecting them to be honored but also documenting their intentions. It's valuable information that deserves some ponderation before applying such destructive fix.

(3) Fine tuning checked overloading. Whitelisting is definitely tedious, but we can fine tune it at will. For example, we could decide to apply point 1 to stay safe. It should be easy to extract all properties and methods of an ill class and whitelist all of them. Then, as @nacin suggested, we could signal incorrect usage on access to whitelisted non-public stuff. This is like taking the point 1 and the opposite of point 2, which is good.

(4) Non-whitelisted or undefined stuff. My idea is to maintain the standard outcome when accessing undefined stuff in PHP, but we can signal incorrect usage on access to non-public non-whitelisted properties. In fact we can easily imagine this to be the destiny of hopefully all legacy properties some time in the future. So it could be something along ​these lines.

(5) Migrating legacy to fresh. As I briefly hinted at above, for improving information hiding along time, i.e. for continuing migrating old code to PHP5, whitelisted overloading allows for a smooth path. Along time, public legacy properties will first continue being used normally. Then they will get more reasonable non-public modifiers and their misuse will be signaled by _wrong_access notices. Then they will be finally un-whitelisted and their misuse will be signaled by _too_wrong_access fatal errors.

[wonderboymusic]

In 31144:

In WP_User_Query, only call magic method internals against a whitelist of properties, $compat_fields.

See #30891.

[wonderboymusic]

In 31145:

In WP_Filesystem_Base, make the only private member, $cache, public and remove magic methods. $cache was always public until [28487], has been essentially public via a magic method since.

See #30891.

[wonderboymusic]

In 31146:

In WP_List_Table, only call magic method internals again whitelists of properties and methods, $compat_fields and $compat_methods.

See #30891.

[wonderboymusic]

In 31147:

In WP_Roles, only allow __call() to run against ->_init().

See #30891.

[wonderboymusic]

In 31148:

In WP_oEmbed, only allow __call() to run against a whitelist of methods, $compat_methods.

See #30891.

[wonderboymusic]

In 31149:

In wp_xmlrpc_server, only allow __call() to run against ->_multisite_getUsersBlogs().

See #30891.

[wonderboymusic]

In 31150:

In WP_Comment_Query, only allow __call() to run against ->get_search_sql().

See #30891.

[wonderboymusic]

In 31151:

In WP_Query, only call magic method internals again whitelists of properties and methods, $compat_fields and $compat_methods. Remove __unset() since __set() is not implemented.

See #30891.

[SergeyBiryukov]

[31146] broke comment action links on Comments screen.

[SergeyBiryukov]

30891.comments-user-can.patch​ fixes the comment actions, but I'm not sure if that's the correct approach.

[31146] means that assigning and retrieving any custom variable in any list table won't work if it's not whitelisted in WP_List_Table.

Should we also update the @return values for those magic methods to account for the changes in [31146]?

[wonderboymusic]

In 31161:

After [31146], properly declare $user_can as a private field in WP_Comments_List_Table.

See #30891.

[wonderboymusic]

In 31162:

Add 2 private fields to WP_Media_List_Table, $detached and $is_trash.

See #30891.

[wonderboymusic]

In 31163:

In lieu of refactoring, add a private field to WP_MS_Themes_List_Table, $has_items. Ideally, this class would overload ->has_items() and not set a private field.

See #30891.

[wonderboymusic]

In 31164:

Add a private field to WP_Plugin_Install_List_Table, $error.

See #30891.

[wonderboymusic]

In 31165:

Add a private field to WP_Posts_List_Table, $is_trash.

See #30891.

[wonderboymusic]

In 31166:

Add a private field to WP_Terms_List_Table, $level.

See #30891.

[wonderboymusic]

In 31167:

WP_Themes_List_Table accesses $_pagination_args from the parent class. Switch $_pagination_args to protected in WP_List_Table.

See #30891.

[DrewAPicture]

@wonderboymusic: What's left here? Can we call this fixed for 4.2?

[wonderboymusic]

I went through every class and analyzed every method call. Done.