WordPress.org

Make WordPress Core

Opened 18 months ago

Last modified 7 weeks ago

#22234 new enhancement

Use access modifiers in classes, not the var keyword

Reported by: wonderboymusic Owned by:
Milestone: Future Release 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 (3)

var-to-public.diff (63.3 KB) - added by wonderboymusic 18 months ago.
22234.diff (46.7 KB) - added by wonderboymusic 15 months ago.
22234.2.diff (47.3 KB) - added by wonderboymusic 9 months ago.

Download all attachments as: .zip

Change History (18)

comment:1 scribu18 months 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 wonderboymusic18 months 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 scribu18 months 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 nacin18 months 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.)

wonderboymusic15 months ago

comment:5 wonderboymusic15 months 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 ryan11 months ago

  • Milestone changed from 3.6 to Future Release

wonderboymusic9 months ago

comment:7 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

comment:9 dd329 months 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 helen7 months ago

Letting it ride for now - docs-related.

comment:11 DrewAPicture7 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 7 months ago by DrewAPicture (previous) (diff)

comment:12 nacin7 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 nacin3 months ago

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

comment:14 GaryJ8 weeks ago

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

comment:15 DrewAPicture7 weeks ago

#27167 was marked as a duplicate.

Note: See TracTickets for help on using tickets.