Page MenuHomePhabricator

Hook interface doc comment review
Closed, ResolvedPublic

Description

Prospective hook interface files have been uploaded to https://gerrit.wikimedia.org/r/c/mediawiki/core/+/574266 . The method doc comments in those files were based on the information available in docs/hooks.txt. All parameters were type-hinted as ?mixed, this is merely a placeholder for review. So, in each new file:

  • Edit the method description, making it into complete sentences that start with either: “Use this hook to…” or “This hook is called [when/after/etc]…”
  • When the method description says something about the return value, consider moving it into the @return section. The generic descriptions in the auto-generated @return can be replaced.
  • Replace DEPRECATED with @deprecated and place it in the interface doc block in place of @stable.
  • Replace the ?mixed placeholder with the actual type of the parameter. Usually the parameter description explains the type.
  • Edit the parameter description, making it start with a capital letter
  • If a parameter takes an object as the type, use the object name as the type hint, add a “Use…” statement to the file, and remove the description if it’s redundant with the type hint. For example: @param ?mixed $user User object can be changed to @param User $user.

In each of the 7 files with // phpcs:disable Generic.Files.LineLength -- Remove this after doc review, remove the line and rewrap the doc comments so the line length is less than 100 characters.

This should be done in a separate commit once the hook interfaces are approved and are reasonably unlikely to have further bulk changes.

The idea is that the hook interfaces will become the canonical source of hook documentation. Hooks.txt might be kept around for a few releases to document the legacy pre-HookContainer system, but eventually I imagine it will be deleted.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 4 2020, 1:10 AM
tstarling updated the task description. (Show Details)Mar 4 2020, 1:49 AM

I suppose we also need type hints, not just doc comment types. The RFC did promise "strong types". I'll add something about that. I think we can just say that every documented class type also becomes a type hint.

tstarling updated the task description. (Show Details)Mar 6 2020, 6:10 AM
Pchelolo added a subscriber: Pchelolo.

AFAIK @apaskulin is a part of green sub team now, moving it to the appropriate work board.

apaskulin added a comment.EditedMar 6 2020, 7:22 PM

Thanks for creating this task, @tstarling! I'm really excited about starting this work. Considering the number of hooks, it will likely take a few weeks to complete, and I'd like to schedule this work in a way that doesn't block the project. Do you have a sense of when you'd like to merge 574266?

I see that the task requests doc updates to be made in a separate commit. Would this be a separate patch request after 574266 is merged?

Thanks for creating this task, @tstarling! I'm really excited about starting this work. Considering the number of hooks, it will likely take a few weeks to complete, and I'd like to schedule this work in a way that doesn't block the project. Do you have a sense of when you'd like to merge 574266?

If you don't mind, I'd like to pitch in and help speed things up. Would you be okay with splitting the hooks and working on them together?

If you don't mind, I'd like to pitch in and help speed things up. Would you be okay with splitting the hooks and working on them together?

Absolutely! I started this etherpad https://etherpad.wikimedia.org/p/T246855 to track which hooks we're planning to work on.

Here are a few questions/clarifications/examples I'd like to add to the task before we start. (@tstarling)

Descriptions
  • Edit the method description, making it into complete sentences in some consistent style.

I think we could rewrite most descriptions as either “This hook is called…” or “Use this hook to…”, helping differentiate between those two types of descriptions.
For example:

  • “When invoking the page editor” becomes “This hook is called when invoking the page editor.”
  • “Add content models to the list of available models.” becomes “Use this hook to add content models to the list of available models.”

Aside from complete sentences and these two templates, are there any other style conventions we want to call out?

Deprecations
  • Replace DEPRECATED with @deprecated and place it below the main description as is conventional.

For example:

	 * DEPRECATED since 1.24! Use ApiQueryTokensRegisterTypes instead.
	 * Use this hook to ...
	 *
	 * @param \User $user
	 * @return bool|null|string True or null to continue, false to abort, or a string
	 *   to raise a fatal error with the specified message.

becomes

	 * Use this hook to ...
	 *
         * @deprecated since version 1.24 Use ApiQueryTokensRegisterTypes instead.
	 * @param \User $user
	 * @return bool|null|string True or null to continue, false to abort, or a string
	 *   to raise a fatal error with the specified message.
Parameters
  • Replace the ?mixed placeholder with the actual type of the parameter. Usually the parameter description explains the type. If the type is a single class, also add the type as a type hint to the relevant parameter.

Would this example be correct?

	 * @param ?mixed $name name of the action
	 * @param ?mixed $form HTMLForm object
	 * @param ?mixed $article Article object
	 * @return ...
	 */
	public function onActionBeforeFormDisplay( $name, $form, $article );

becomes

	 * @param string $name name of the action
	 * @param /HTMLForm $form HTMLForm object
	 * @param /Article $article Article object
	 * @return ...
	 */
	public function onActionBeforeFormDisplay( $name, /HTMLForm $form, /Article $article );

Is there a way to discover the type if it's unclear? For example:

@param ?mixed &$salts [ type => salt to pass to User::getEditToken(), or array of salt and key to pass to Session::getToken() ]
Return values
  • When the method description says something about the return value, consider moving it into the @return section. The generic descriptions in the auto-generated @return can be replaced.

Would this example be correct? Would we remove null in this case?

Return true to allow the normal editor to be used, or false if implementing a custom editor, e.g. for a special namespace, etc.

moves to

@return bool|null|string Return true to allow the normal editor to be used, false if implementing a custom editor, e.g. for a special namespace, or a string to raise a fatal error with the specified message.

Removing from Green Team workboard following conversation with Will

Thanks for creating this task, @tstarling! I'm really excited about starting this work. Considering the number of hooks, it will likely take a few weeks to complete, and I'd like to schedule this work in a way that doesn't block the project. Do you have a sense of when you'd like to merge 574266?

I see that the task requests doc updates to be made in a separate commit. Would this be a separate patch request after 574266 is merged?

It can be in a dependent commit before 574266 is merged. So you would check out 574266 into a local branch and make your changes in a new commit on top of that. I just wanted to delay until 574266 is reasonably complete, to avoid causing merge conflicts when you rebase on top of my changes. But let's say it'll be stable enough by the end of today.

Delaying the merge of 574266 until the doc review is done will avoid confusing people by merging incomplete work, although it means we'll have to rebase on top of any changes that come from master, like new hooks or parameters.

I think we could rewrite most descriptions as either “This hook is called…” or “Use this hook to…”, helping differentiate between those two types of descriptions.

Sounds good.

	 * Use this hook to ...
	 *
         * @deprecated since version 1.24 Use ApiQueryTokensRegisterTypes instead.
	 * @param \User $user
	 * @return bool|null|string True or null to continue, false to abort, or a string
	 *   to raise a fatal error with the specified message.

I think @deprecated should be separated from the parameters by an empty line. Following @daniel's comment on PS8, in PS9 there will be a "@stable for implementation" doc comment in every interface, separated from the parameters by an empty line:

	 * Use this hook to ...
	 *
	 * @stable for implementation
	 *
	 * @param \User $user
	 * @return bool|null|string True or null to continue, false to abort, or a string
	 *   to raise a fatal error with the specified message.

@deprecated can replace that annotation:

	 * Use this hook to ...
	 *
	 * @deprecated since 1.24 Use ApiQueryTokensRegisterTypes instead.
	 *
	 * @param \User $user
	 * @return bool|null|string True or null to continue, false to abort, or a string
	 *   to raise a fatal error with the specified message.

Maybe we could also add @since in that section.

For consistency with the rest of the code base, I suggest "@deprecated since 1.24" not "@deprecated since version 1.24".

Would this example be correct?

	 * @param string $name name of the action
	 * @param /HTMLForm $form HTMLForm object
	 * @param /Article $article Article object
	 * @return ...
	 */
	public function onActionBeforeFormDisplay( $name, /HTMLForm $form, /Article $article );

Yes, it's fine. If it were new code, "HTMLForm object" would be a bit too obvious to bother saying, since it's already there in the type. So this would also be acceptable:

	 * @param string $name Name of the action
	 * @param /HTMLForm $form
	 * @param /Article $article
	 * @return ...
	 */
	public function onActionBeforeFormDisplay( $name, /HTMLForm $form, /Article $article );

There may be useful things we could say about the article and the form, but it won't be easy for you to find out what they are.

I capitalised "Name" in the first parameter of the above example, because I think it looks nicer that way in Doxygen.

Is there a way to discover the type if it's unclear? For example:

@param ?mixed &$salts [ type => salt to pass to User::getEditToken(), or array of salt and key to pass to Session::getToken() ]

You can search the code for the hook name, which is usually the method name without "on", to find its caller. Most hooks only have one caller. In this case:

			$salts = [
				'csrf' => '',
				'watch' => 'watch',
				'patrol' => 'patrol',
				'rollback' => 'rollback',
				'userrights' => 'userrights',
				'login' => [ '', 'login' ],
				'createaccount' => [ '', 'createaccount' ],
			];
			Hooks::run( 'ApiQueryTokensRegisterTypes', [ &$salts ] );

So you can see from the code that a "salt" is a string. If it's still not obvious, I would suggest making a list of difficult cases, and then reviewing them in a 1:1 meeting with an engineer (e.g. me).

Would this example be correct? Would we remove null in this case?

Return true to allow the normal editor to be used, or false if implementing a custom editor, e.g. for a special namespace, etc.

moves to

@return bool|null|string Return true to allow the normal editor to be used, false if implementing a custom editor, e.g. for a special namespace, or a string to raise a fatal error with the specified message.

You should certainly remove the thing about a string raising a fatal error. I'm a bit uncomfortable with having that in the default description since it's extremely rare and shouldn't be encouraged. Maybe I should just remove all mentions of the return value being potentially a string, and deprecate that behaviour. Otherwise I think your example is fine.

I'm no longer sure whether it should say "null" or "void" or both since PHPStorm makes a distinction between them. The point of allowing null to be returned is so that handlers can omit the return statement for brevity. It's always equivalent to returning true.

You should certainly remove the thing about a string raising a fatal error. I'm a bit uncomfortable with having that in the default description since it's extremely rare and shouldn't be encouraged. Maybe I should just remove all mentions of the return value being potentially a string, and deprecate that behaviour.

I removed it, and asked Nikki to hard-deprecate it in HookContainer.

I note that the generated interfaces currently do not specify type hints in the method signatures. Adding type hints would make things type safe for extensions. On the other hand, this would prevent us from replacing objects we no longer want to expose with fakes.

In any case, we can add type hints to the method signatures in the interface declarations later, without breaking implementing classes that do not specify type hints in the method signatures. It seems like PHP considers this legal.

OK, let's not do type hints just yet.

Change 580142 had a related patch set uploaded (by Alex Paskulin; owner: Alex Paskulin):
[mediawiki/core@master] docs: Hook interface doc comment review

https://gerrit.wikimedia.org/r/580142

I'm going to throw this into the CD "Waiting for Review" since that seems the most likely place if this isn't Green Team.

If it's still not obvious, I would suggest making a list of difficult cases, and then reviewing them in a 1:1 meeting with an engineer (e.g. me).

Or we could just use Gerrit comments.

Maybe I should just remove all mentions of the return value being potentially a string, and deprecate that behaviour.

Sounds good to me.

I'm no longer sure whether it should say "null" or "void" or both since PHPStorm makes a distinction between them. The point of allowing null to be returned is so that handlers can omit the return statement for brevity. It's always equivalent to returning true.

"void" sounds more correct, I don't think we actually want people explicitly returning null.

"void" sounds more correct, I don't think we actually want people explicitly returning null.

In PS11 of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/574266 I changed the type from bool|null|void to bool|void. The description is still "True or null to continue or false to abort", not sure if it makes sense to talk about "returning void" or if there's a better way to say that.

How about "Return true or omit a return value to continue. Return false to abort"? Here's an example for CustomEditorHook.

Sounds good. Feel free to do a global search and replace -- I think that's an easier way to update the auto-generated interfaces than running the script again.

Sounds good. Feel free to do a global search and replace -- I think that's an easier way to update the auto-generated interfaces than running the script again.

Will do!

Here's an example of what the documentation looks like in Doxygen:

It seems like Doxygen is adding an extra slash before the object names and not recognizing the return types. These aren't serious issues and can likely be corrected with Doxygen filters, but I wanted to mention it here.

That's a good catch, it seems Doxygen doesn't really recognize PHP's namespaces that use backslashes. It seems to work if we do all the classes with a use directive at the top of the file, like

<?php

namespace MediaWiki\Hook;

use HistoryPager;
use Wikimedia\Rdbms\ResultWrapper;

// phpcs:disable Squiz.Classes.ValidClassName.NotCamelCaps
/**
 * @ingroup Hooks
 */
interface PageHistoryPager__doBatchLookupsHook {
    /**
     * This hook is called after the pager query was run, before any output is generated,
     * to allow batch lookups for prefetching information needed for display.
     *
     * @stable for implementation
     * @since 1.35
     *
     * @param HistoryPager $pager
     * @param ResultWrapper $result A ResultWrapper representing the query result
     * @return bool|void True or null to continue, or false to skip the regular
     *   behavior of doBatchLookups()
     */
    public function onPageHistoryPager__doBatchLookups( $pager, $result );
}
daniel triaged this task as Medium priority.Apr 7 2020, 12:51 PM
apaskulin updated the task description. (Show Details)Apr 9 2020, 11:54 PM

Change 587898 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] user/Hook: Hook interface doc comment review

https://gerrit.wikimedia.org/r/587898

Change 587969 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/core@master] specials/Hook: Hook interface doc comment review

https://gerrit.wikimedia.org/r/587969

apaskulin updated the task description. (Show Details)Apr 16 2020, 9:23 PM

@apaskulin the etherpad says that all have been done - does the task description need to be updated?
Also, https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/588936/ is keeping track of recent changes - 3 new hooks have been added. I asked if I should be cleaning up the docs in the same commit as those additions (and 9 depecations) but either way I can tackle the review for the 3 new ones

apaskulin updated the task description. (Show Details)Apr 20 2020, 4:06 PM

Perfect. Thanks, @DannyS712! I updated the task description. Tim has been coordinating the patches and relation chains, so I'll let him answer the question about using the same commit.

Change 580142 merged by jenkins-bot:
[mediawiki/core@master] docs: Hook interface doc comment review

https://gerrit.wikimedia.org/r/580142

Change 591408 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Move RL hooks to own namespace, use PSR-4

https://gerrit.wikimedia.org/r/591408

Change 587898 merged by jenkins-bot:
[mediawiki/core@master] user/Hook: Hook interface doc comment review

https://gerrit.wikimedia.org/r/587898

Change 587969 merged by jenkins-bot:
[mediawiki/core@master] specials/Hook: Hook interface doc comment review

https://gerrit.wikimedia.org/r/587969

Change 591408 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Move RL hooks to own namespace, use PSR-4

https://gerrit.wikimedia.org/r/591408

@tstarling I'm not sure lack of namespaces was intentional or just something to do later. Would this change be fine?

Change 591408 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Move RL hooks to own namespace, use PSR-4

https://gerrit.wikimedia.org/r/591408

Is his done? Can this be closed?

apaskulin closed this task as Resolved.May 13 2020, 5:35 PM

Resolving this task since the work described here has been completed. Thanks, all!