WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 14 months ago

Last modified 12 months ago

#22234 closed enhancement (fixed)

Use access modifiers in classes, not the var keyword

Reported by: wonderboymusic Owned by:
Milestone: 4.0 Priority: normal
Severity: normal Version: 1.5
Component: General Keywords: has-patch
Focuses: docs Cc:

Description

The minimum required PHP version is 5.2.4. The var keyword is a relic of PHP 4. Let us open our hymnals to php.net:

Note: The PHP 4 method of declaring a variable with the var keyword is still supported for compatibility reasons (as a synonym for the public keyword). In PHP 5 before 5.1.3, its usage would generate an E_STRICT warning.

PHP 5 has much better support for OO features like access modifiers in classes. WP also has a history of including PHPDoc blocks with code and using the @access tag. However, the tag is meaningless if it doesn't match the supplied access modifier.

PHPDoc blocks are present 1) for inline documentation, sure but mainly 2) to allow automatic generation of documentation when used with a command-line tool like phpDocumentor or (IMO, the far superior) ApiGen.

If I specify the following:

/**
 * @access private
 */
var $prop;

$prop will appear in the documentation as public because var means public.

I have modified class properties throughout the codebase to use access modifiers instead of var.

  • If no PHPDoc was present, I made the property public, which it already was.
  • If the @access tag was present, I used its value for the property.
  • I then ran Unit Tests which produced some immediate fatal errors do to existing core code that was trying to access properties in a public way that had @access set to private.
  • I altered those properties to indeed be public and updated the PHPDoc

Attachments (4)

var-to-public.diff (63.3 KB) - added by wonderboymusic 3 years ago.
22234.diff (46.7 KB) - added by wonderboymusic 2 years ago.
22234.2.diff (47.3 KB) - added by wonderboymusic 2 years ago.
22234.3.diff (10.2 KB) - added by wonderboymusic 14 months ago.

Download all attachments as: .zip

Change History (82)

comment:1 @scribu3 years ago

Even if it was marked with @access private, we can't uniformly make previously public properties into private ones.

We have to evaluate the potential impact on third-party code on a case by case basis.

comment:2 @wonderboymusic3 years ago

This is just a conversation starter. Every property being public in every class in the codebase might actually be just fine, but then the PHPDocs are useless. @access private on public properties and public functions accomplishes nothing. If it's supposed to say to a dev "don't use me" - let's enforce that and change / move / remove those functions at will. If it is being used to actually affect visibility, it doesn't. If it's being used to accurately explain visibility in generated docs, it doesn't.

So are we trying to create documentation that is complete and accurate? We should, and let's do it with proper access modifiers AND proper PHPDocs, even if it requires changing them all to public.

comment:3 @scribu3 years ago

Well, yeah, we should remove the @access private for the properties that we can't make private. We just have to figure out which they are.

comment:4 @nacin3 years ago

All new classes and properties (since 3.2) get proper visibility keywords. For existing properties, I have been following this guideline:

  • If the property is public, declare it as such. If the property is designed to be protected or private but cannot be, keep "var" rather than announcing that it is "public" and keep @access.

I've begin to experiment with something else for wpdb: begin to set older properties as protected or private, but then providing a magic getter/setter to handle those values. That way, the functionality is the same, but A) the declarations and docs are accurate and B) we can throw deprecated property notices (but don't yet).

The pattern wpdb uses might not be perfect for all classes, though, in that PHP's magic methods aren't uniformly used and therefore it might not be backwards compatible for all use cases. (We ran into some issues in hyperdb but got that fixed, and otherwise wpdb access is uncommon.)

@wonderboymusic2 years ago

comment:5 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.6

Uploaded a new patch that uncontroversially only touches properties that were already public. It ignores all of the properties marked with var and an @access doc block of private or protected. I removed the public @access blocks.

The @access doc block is weird. It is not supposed to document an access modifier, or override it in documentation. It is supposed to be used to suppress documentation of certain elements in your code: http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial_tags.access.pkg.html

Protected is allowed, but does absolutely nothing.

While technically that might have been what people had in mind while using it, it seems like it is being used to control access, which access modifiers do automatically. Also, I doubt there is code that people explicitly want to not document - since the documentation generator will catch the private access modifier and mark things as such.

comment:6 @ryan2 years ago

  • Milestone changed from 3.6 to Future Release

@wonderboymusic2 years ago

comment:7 @wonderboymusic2 years ago

  • Milestone changed from Future Release to 3.7

comment:9 @dd322 years ago

Keep in mind, that we cannot use access modifiers in a range of files which are included when we bail for old versions of PHP (ie. PHP4): #24915

comment:10 @helen22 months ago

Letting it ride for now - docs-related.

comment:11 @DrewAPicture22 months ago

Per the inline documentation standards we're sticking with using @var in this context for the time being. However, if there are missing access modifiers, patches are welcome to include them.

Last edited 22 months ago by DrewAPicture (previous) (diff)

comment:12 @nacin22 months ago

  • Component changed from General to Inline Docs
  • Milestone changed from 3.7 to Future Release

I like this but it needs a close examination to make sure we're following our guidelines. I'm going to punt this and move it into "Inline Docs" for action whenever we're able.

comment:13 @nacin18 months ago

  • Component changed from Inline Docs to General
  • Focuses docs added

comment:14 @GaryJ17 months ago

Any reason that 22234.2.diff can't go ahead?

comment:15 @DrewAPicture16 months ago

#27167 was marked as a duplicate.

comment:16 @wonderboymusic14 months ago

These files are off-limits:

wp-includes/load.php
wp-includes/version.php
wp-includes/pomo/mo.php
wp-includes/pomo/translations.php
wp-includes/pomo/streams.php
wp-includes/l10n.php
wp-includes/locale.php
wp-includes/plugin.php

Related: #27881. Edit: magic getter sounds like a good solution.

Last edited 14 months ago by wonderboymusic (previous) (diff)

@wonderboymusic14 months ago

comment:17 @wonderboymusic14 months ago

hackificator expects access modifiers for methods as well.

22234.3.diff is an example of converting classes and adding magic __get() method.

comment:18 @wonderboymusic14 months ago

In 28481:

Use proper access modifiers and add a magic __get() method to Custom_Background and Custom_Image_Header.

See #27881, #22234.

comment:19 @wonderboymusic14 months ago

  • Milestone changed from Future Release to 4.0

comment:20 @wonderboymusic14 months ago

In 28483:

Remove public keyword from some JS functions. Sorry.

Props ocean90.
See #22234.

comment:21 @wonderboymusic14 months ago

In 28486:

Add access modifier (public) to members and methods in WP_Comments_List_Table and WP_Post_Comments_List_Table.

See #27881, #22234.

comment:22 @wonderboymusic14 months ago

In 28487:

Add access modifiers to members and methods in WP_Filesystem_Base. Add magic __get() method for backwards compatibility.

See #27881, #22234.

comment:23 @wonderboymusic14 months ago

In 28488:

Add access modifier (public) to methods in WP_Filesystem_Direct.

See #27881, #22234.

comment:24 @wonderboymusic14 months ago

In 28489:

Add access modifier (public) to members and methods in WP_Filesystem_FTPext.

See #27881, #22234.

comment:25 @wonderboymusic14 months ago

In 28490:

Add access modifier (public) to members and methods in WP_Filesystem_ftpsockets.

See #27881, #22234.

comment:26 @wonderboymusic14 months ago

In 28491:

Add access modifier (public) to members and methods in WP_Filesystem_SSH2.

See #27881, #22234.

comment:27 @wonderboymusic14 months ago

In 28492:

Add access modifier (public) to methods in WP_Importer.

See #27881, #22234.

comment:28 @wonderboymusic14 months ago

In 28493:

Add access modifiers to methods and members of list table classes:

  • WP_List_Table is the base class that implements __get() and __call() for BC
  • Adds unit tests to confirm that subclasses properly inherit magic methods
  • Add modifiers to subclasses: WP_Links_List_Table, WP_Media_List_Table, WP_MS_Sites_List_Table, WP_MS_Themes_List_Table, WP_MS_Users_List_Table, WP_Plugin_Install_List_Table, WP_Plugins_List_Table, WP_Posts_List_Table, WP_Terms_List_Table, WP_Theme_Install_List_Table, WP_Themes_List_Table

See #27881, #22234.

comment:29 @wonderboymusic14 months ago

In 28494:

Add access modifiers to methods and members of WP_Users_List_Table.

See #27881, #22234.

comment:30 @wonderboymusic14 months ago

In 28495:

Add access modifier (public) to methods and members of WP_Upgrader_Skin and its subclasses.

See #27881, #22234.

comment:31 @wonderboymusic14 months ago

In 28496:

Add access modifier (public) to methods and members of WP_Upgrader and its subclasses.

See #27881, #22234.

comment:32 @wonderboymusic14 months ago

In 28503:

Add access modifiers to methods/members in WP_Roles. Add a magic __call() method for BC.

See #27881, #22234.

comment:33 @wonderboymusic14 months ago

In 28504:

Add access modifiers to methods/members in Walker_Category and Walker_CategoryDropdown.

See #27881, #22234.

comment:34 @wonderboymusic14 months ago

In 28505:

Add access modifiers to methods/members in WP_Feed_Cache, WP_SimplePie_File, and WP_Feed_Cache_Transient.

See #27881, #22234.

comment:35 @wonderboymusic14 months ago

In 28506:

Add access modifier to methods of HTTP classes. There are no new private or protected methods, so no need for __call().

See #27881, #22234.

comment:36 @wonderboymusic14 months ago

In 28507:

Add access modifier to methods/members in WP_oEmbed. Adds a magic __call() method for BC.

See #27881, #22234.

comment:37 @wonderboymusic14 months ago

In 28508:

Add access modifier to methods/members in WP_Ajax_Response. Adds a magic __get() method for BC.

See #27881, #22234.

comment:38 @wonderboymusic14 months ago

In 28509:

Add missing access modifiers to methods/members in WP_Customize_*.

See #27881, #22234.

comment:39 @wonderboymusic14 months ago

In 28510:

Add access modifiers to methods/members in WP_Embed.

See #27881, #22234.

comment:40 @wonderboymusic14 months ago

In 28511:

Add access modifiers to methods/members in WP_Error. Add a magic __get() method for BC.

See #27881, #22234.

comment:41 @wonderboymusic14 months ago

In 28512:

Add access modifiers to methods/members in WP_HTTP_IXR_Client.

See #27881, #22234.

comment:42 @wonderboymusic14 months ago

In 28513:

Add missing access modifiers to methods/members in WP_Image_Editor_* classes.

See #27881, #22234.

comment:43 @wonderboymusic14 months ago

In 28514:

Add missing access modifiers to methods/members in Walker and subclasses. Add a magic __get() method.

See #27881, #22234.

comment:44 @wonderboymusic14 months ago

In 28515:

Add missing access modifiers to methods in wp_xmlrpc_server. Add a magic __call() method for BC.

See #27881, #22234.

comment:45 @wonderboymusic14 months ago

In 28516:

Add missing access modifiers to methods in WP and WP_MatchesMapRegex. Add magic __call() and __get() methods to WP_MatchesMapRegex for BC.

See #27881, #22234.

comment:46 @wonderboymusic14 months ago

In 28517:

Add missing access modifiers to methods in WP_Dependencies and _WP_Dependency.

See #27881, #22234.

comment:47 @wonderboymusic14 months ago

In 28518:

Add missing access modifiers to methods in WP_Scripts and WP_Styles.

See #27881, #22234.

comment:48 @wonderboymusic14 months ago

In 28519:

Add missing access modifiers to methods in WP_Comment_Query. Add a magic __call() method for BC.

See #27881, #22234.

comment:49 @wonderboymusic14 months ago

In 28521:

Some classes with __get() method also need __set().

See #27881, #22234.

comment:50 @wonderboymusic14 months ago

In 28522:

Add missing access modifiers to methods in WP_Meta_Query.

See #27881, #22234.

comment:51 @wonderboymusic14 months ago

In 28523:

Add missing access modifiers to methods in WP_Query. Add magic methods for __get(), __set(), __isset(), __unset(), and __call().

Add unit test for magic methods.

See #27881, #22234.

comment:52 @wonderboymusic14 months ago

In 28524:

Classes that have __set() also need __isset() and __unset().

See #27881, #22234.

comment:53 @wonderboymusic14 months ago

In 28525:

Add access modifiers to WP_Text_Diff_Renderer_Table that are compatible with its parent class. Some of the inline docs suggest access that, if implemented, would produce fatal errors.

Add magic methods for BC: get(), set(), isset(), unset(), and call().

See #27881, #22234.

comment:54 @wonderboymusic14 months ago

In 28526:

In wpdb, make some things explicitly public. Do not set anything to private. This would instantly blow up hyperdb in the wild.

See #27881, #22234.

comment:55 @wonderboymusic14 months ago

In 28527:

Add public access modifier to methods/members of WP_Widget and WP_Widget_Factory.

See #27881, #22234.

comment:56 @wonderboymusic14 months ago

In 28528:

Add access modifiers to WP_User_Query.

Add magic methods for BC: get(), set(), isset(), unset(), and
call().

See #27881, #22234.

comment:57 @wonderboymusic14 months ago

In 28529:

In WP_Filesystem_Base, the constructor is a noop, so it shouldn't even be declared. Setting it implies that parent::__construct() should be called by its subclasses.

See #27881, #22234.

comment:58 @wonderboymusic14 months ago

In 28531:

Upgrade _WP_List_Table_Compat to PHP5-style constructor.

Add public to methods/members of WP_Role.
Add public to methods/members of WP_User where appropriate. Don't set private where indicated until more study has occurred and tests have been written for compatibiliy with existing magic methods.

See #27881, #22234.

comment:59 @wonderboymusic14 months ago

In 28532:

WP_Date_Query was only missing one access modifier.

Add access modifier (public) to all default widgets' class methods.

See #27881, #22234.

comment:60 @wonderboymusic14 months ago

In 28533:

WP_Query was only missing one access modifier.

Add access modifier (public) to applicable class methods/members of WP_Rewrite. I am not brave enough to set some of the vars to private without more testing.

See #27881, #22234.

comment:61 @wonderboymusic14 months ago

  • Resolution set to fixed
  • Status changed from new to closed

Marking this fixed - the only remaining vars are in wpdb and WP_Rewrite. Those can be attacked later, if at all.

comment:62 @DrewAPicture12 months ago

In 29139:

Fill out inline documentation for magic methods added to the WP_Text_Diff_Renderer_Table class in [28525].

See #27881, #22234 and #28885.

comment:63 @DrewAPicture12 months ago

In 29140:

Fill out inline documentation for magic methods added to the WP_User_Query class in [28528].

See #27881, #22234 and #28885.

comment:64 @DrewAPicture12 months ago

In 29141:

Fill out inline documentation for magic methods added to the WP_Query class in [28523].

See #27881, #22234 and #28885.

comment:65 @DrewAPicture12 months ago

In 29142:

Fill out inline documentation for magic methods added to the WP_MatchesMapRegex class in [28516].

See #27881, #22234 and #28885.

comment:66 @DrewAPicture12 months ago

In 29143:

Fill out inline documentation for magic methods added to the Walker class in [28514].

See #27881, #22234 and #28885.

comment:67 @DrewAPicture12 months ago

In 29144:

Fill out inline documentation for magic methods added to the WP_Error class in [28511].

See #27881, #22234 and #28885.

comment:68 @DrewAPicture12 months ago

In 29145:

Fill out inline documentation for magic methods added to the WP_Ajax_Response class in [28524].

See #27881, #22234 and #28885.

comment:69 @DrewAPicture12 months ago

In 29146:

Fill out inline documentation for magic methods added to the WP_Object_Cache class in [28502], [28521], [28524].

See #27881, #22234 and #28885.

comment:70 @DrewAPicture12 months ago

In 29147:

Fill out inline documentation for magic methods added to the WP_Filesystem_Base class in [28521].

See #22234 and #28885.

comment:71 @DrewAPicture12 months ago

In 29148:

Fill out inline documentation for magic methods added to the WP_List_Table class in [28493], [28521], and [28524].

See #22234 and #28885.

comment:72 @DrewAPicture12 months ago

In 29149:

Fill out inline documentation for magic methods added to the Custom_Background class in [28481], [28521], and [28524].

See #22234 and #28885.

comment:73 @DrewAPicture12 months ago

In 29150:

Fill out inline documentation for magic methods added to the Custom_Image_Header class in [28481], [28521], and [28524].

See #22234 and #28885.

comment:74 @DrewAPicture12 months ago

In 29153:

Fill out inline documentation for the __call() magic method added to the WP_Roles class in [28503].

See #22234 and #28885.

comment:75 @DrewAPicture12 months ago

In 29155:

Inline documentation cleanup for 4.0 audit.

  • Fill out inline documentation for the __call() magic method added in [28507]
  • Inline documentation tweaks for get_provider(), added in [28728]
  • Inline documentation tweaks for _add_provider_early(), added in [28846]
  • @access tag added for _remove_provider_early(), added in [28846]

See #22234 and #28885.

comment:76 @DrewAPicture12 months ago

In 29161:

Fill out inline documentation for the __call() magic method added to the wp_xmlrpc_server class in [28515].

See #22234 and #28885.

comment:77 @DrewAPicture12 months ago

In 29162:

Fill out inline documentation for the __call() magic method added to the WP_Comment_Query class in [28519].

See #22234 and #28885.

comment:78 @DrewAPicture12 months ago

In 29165:

Add periods to short descriptions for magic methods added in [28501], [28521], and [28524].

See #22234 and #28885.

Note: See TracTickets for help on using tickets.