Make WordPress Core

Opened 11 years ago

Closed 11 years ago

#27167 closed enhancement (duplicate)

Drop @access in favour of @internal

Reported by: garyj's profile GaryJ Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.9
Component: General Keywords:
Focuses: docs Cc:

Description

Our documentation standards say that:

"@access: Only use for functions if private. If the function or class method is private, it is intended for internal use only, and there will be no documentation for it in the code reference."

AFAIK, @access was introduced via phpDocumentor into the defacto phpDoc standards during PHP 4. The public, protected and private visbility keywords were not introduced for class methods and properties until PHP5, but since then, @access has been redundant for use in classes.

There are 979 instances of @access. 464 of them are marked as public, 119 and protected and 396 are private, meaning that nearly 60% of them shouldn't be there according to the standards. Many of them are also wrong anyway and contradict the code. e.g.

class FooBar {
	/**
	 * @access protected
	 */
	var $my_property;

	/**
	 * @access private
	 */
	function _my_func() {}
}

The @access tag is NOT part of the upcoming PSR-5 document, which means that documentation generators may not support it.


Since the only non-redundant usage for @access is on functions, and the only valid value according to the documentation standards is @access private, I propose that this is swapped out for the more descriptively accurate @internal.

@internal IS part of PSR-5. It can be used in two ways:

  1. As a normal tag "indicating that the associated structural element is used purely for the internal workings of this piece of software". WP-Parser already excludes importing functions and methods marked with @internal unless explicitly requested.
  1. As an inline tag to "add internal comments or additional description text inline to the Long Description". This should take the form of {@internal Some comment.}}.

WP currently has 124 instances of @internal. Of those, the majority are for missing short and long descriptions (inline tag). Of the remainder, some are normal tags which should be inline tags, and only a few are actually used for what are intended to be private functions. Those elements which use the normal tag incorrectly, will not be appearing in WP-Parser results.


To summarise:

  • @access is being used more incorrectly than correctly according to our documentation standards.
  • @access is inconsistent with the code it is describing.
  • @access is not in PSR-5, and is generally redundant.
  • @internal is in PSR-5, is more flexible and more descriptive.
  • @internal is not used correctly, causing incorrect WP-Parser results and therefore missing entries on DevHub code reference.

A plan:

  • Convert normal @internal tags to inline {@internal ...}} tags where appropriate.
  • Remove @access public and @access protected from everywhere.
  • Convert @access private to @internal for functions where correct.
  • Remove remaining @access private instances.

Thoughts?

Related #22234 (and vaguely #16682).

Change History (4)

#1 @helen
11 years ago

Thoughts?

My thought is that Trac can be a poor venue for discussions of this nature. Is this the result of an IRC or some other discussion (either real-time or async) with those heavily involved in inline docs? It would be good to cross-reference if so.

#2 @nacin
11 years ago

We still use the var keyword plus @access private|protected for all properties introduced in a class prior to WP 3.2 (when we moved to PHP 5.2). Properties meant to be explicit get shifted to public, but we deliberately don't want to specify a property meant to be private or protected as public and we also can't use private or protected because that can cause major errors in existing plugins.

Thus the examples that follow "Many of them are also wrong anyway and contradict the code" are actually correct.

WP Parser can parse whatever we tell it to. For this case, we can easily parse @access for var properties and determine whether to document it as private or protected. Same for methods.

I'm fine with converting @access public + var to public, but as voiced in #22234, this isn't just a find and replace. It needs to happen with a fine-toothed comb to make sure the documentation is actually correct, otherwise we back ourselves into a corner.

We can move to @internal for private-use functions. But @access private|protected for methods meant to be private or protected is more explicit to me.

We know we're not following PSR-5, we're just doing what is practical for a codebase riddled with private-use functions, properties, and methods that we can't actually lock down without breaking things.

#3 follow-up: @nacin
11 years ago

This seems to me like a duplicate of #22234, thus.

#4 in reply to: ↑ 3 @DrewAPicture
11 years ago

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

Replying to nacin:

This seems to me like a duplicate of #22234, thus.

Agreed.

Note: See TracTickets for help on using tickets.