#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)
Change History (85)
#2
@
12 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.
#3
@
12 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.
#4
@
12 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.)
#5
@
12 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.
#9
@
11 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
#11
@
11 years ago
Per the inline documentation standards we're sticking with using @var
in this context for the time being. However, if there missing access modifiers, patches are welcome to include them.
#12
@
11 years 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.
#16
@
11 years 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.
#17
@
11 years ago
hackificator
expects access modifiers for methods as well.
22234.3.diff is an example of converting classes and adding magic __get()
method.
#61
follow-up:
↓ 80
@
11 years ago
- Resolution set to fixed
- Status changed from new to closed
Marking this fixed - the only remaining var
s are in wpdb
and WP_Rewrite
. Those can be attacked later, if at all.
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.