mail: Round 6 of EmailUser refactoring

- Rename methods to follow the more standard convention used for
  commands.
- Update doc comments.
- Add new hooks with final method names and parameter types, to replace
  the old messy hooks.
- Deprecate the remaining old hooks.

Bug: T265541
Change-Id: I609709a70fb58ce00b9f179ee4de2f6ac5e0a1cf

RELEASE-NOTES-1.41

@@ -311,8 +311,10 @@ because of Phabricator reports.
   ::getLinkHTML() and ::getLinkWiki().
 * SearchResultThumbnail::getSize() has been deprecated to be dropped in the
   future as it is resource intensive and degrades performance.
-* The EmailUserPermissionsErrors hook has been deprecated in favour of the
-  UserCanSendEmail hook.
+* The EmailUserPermissionsErrors and UserCanSendEmail hooks have been
+  deprecated in favour of the EmailUserAuthorizeSend hook.
+* The EmailUser hook has been deprecated in favour of the EmailUserSendEmail
+  hook.
 * SiteConfiguration::extractVar() and ::extractGlobal() have been deprecated
   and also emit deprecation warnings.
 * Hooks::isRegistered(), ::getHandlers(), ::run() and ::runWithoutAbort(),

autoload.php

@@ -1515,6 +1515,8 @@ $wgAutoloadLocalClasses = [
 	'MediaWiki\\Mail\\EmailUser' => __DIR__ . '/includes/mail/EmailUser.php',
 	'MediaWiki\\Mail\\EmailUserFactory' => __DIR__ . '/includes/mail/EmailUserFactory.php',
 	'MediaWiki\\Mail\\Emailer' => __DIR__ . '/includes/mail/Emailer.php',
+	'MediaWiki\\Mail\\Hook\\EmailUserAuthorizeSendHook' => __DIR__ . '/includes/mail/Hook/EmailUserAuthorizeSendHook.php',
+	'MediaWiki\\Mail\\Hook\\EmailUserSendEmailHook' => __DIR__ . '/includes/mail/Hook/EmailUserSendEmailHook.php',
 	'MediaWiki\\Mail\\IEmailer' => __DIR__ . '/includes/mail/IEmailer.php',
 	'MediaWiki\\Mail\\UserEmailContact' => __DIR__ . '/includes/mail/UserEmailContact.php',
 	'MediaWiki\\MainConfigNames' => __DIR__ . '/includes/MainConfigNames.php',

includes/HookContainer/DeprecatedHooks.php

@@ -54,6 +54,8 @@ class DeprecatedHooks {
 		'ArticleUndelete' => [ 'deprecatedVersion' => '1.40', 'silent' => true ],
 		'MessageCache::get' => [ 'deprecatedVersion' => '1.41', 'silent' => true ],
 		'EmailUserPermissionsErrors' => [ 'deprecatedVersion' => '1.41' ],
+		'UserCanSendEmail' => [ 'deprecatedVersion' => '1.41', 'silent' => true ],
+		'EmailUser' => [ 'deprecatedVersion' => '1.41', 'silent' => true ],
 	];
 
 	/**

includes/HookContainer/HookRunner.php

@@ -6,9 +6,11 @@ use Article;
 use File;
 use IContextSource;
 use JsonContent;
+use MailAddress;
 use ManualLogEntry;
 use MediaWiki\Linker\LinkRenderer;
 use MediaWiki\Linker\LinkTarget;
+use MediaWiki\Mail\UserEmailContact;
 use MediaWiki\Page\PageIdentity;
 use MediaWiki\Page\ProperPageIdentity;
 use MediaWiki\Permissions\Authority;
@@ -171,6 +173,8 @@ class HookRunner implements
 	\MediaWiki\Hook\EmailUserFormHook,
 	\MediaWiki\Hook\EmailUserHook,
 	\MediaWiki\Hook\EmailUserPermissionsErrorsHook,
+	\MediaWiki\Mail\Hook\EmailUserAuthorizeSendHook,
+	\MediaWiki\Mail\Hook\EmailUserSendEmailHook,
 	\MediaWiki\Hook\EnhancedChangesListModifyBlockLineDataHook,
 	\MediaWiki\Hook\EnhancedChangesListModifyLineDataHook,
 	\MediaWiki\Hook\EnhancedChangesList__getLogTextHook,
@@ -1644,6 +1648,28 @@ class HookRunner implements
 		);
 	}
 
+	public function onEmailUserAuthorizeSend( Authority $sender, StatusValue $status ) {
+		return $this->container->run(
+			'EmailUserAuthorizeSend',
+			[ $sender, $status ]
+		);
+	}
+
+	public function onEmailUserSendEmail(
+		Authority $from,
+		MailAddress $fromAddress,
+		UserEmailContact $to,
+		MailAddress $toAddress,
+		string $subject,
+		string $text,
+		StatusValue $status
+	) {
+		return $this->container->run(
+			'EmailUserSendEmail',
+			[ $from, $fromAddress, $to, $toAddress, $subject, $text, $status ]
+		);
+	}
+
 	public function onEnhancedChangesListModifyBlockLineData( $changesList, &$data,
 		$rc
 	) {

includes/mail/EmailUser.php

@@ -109,7 +109,8 @@ class EmailUser {
 	}
 
 	/**
-	 * Validate target User
+	 * @internal
+	 * @todo This method might perhaps be moved to a UserEmailContactLookup or something.
 	 *
 	 * @param UserEmailContact $target Target user
 	 * @return StatusValue
@@ -155,13 +156,13 @@ class EmailUser {
 	}
 
 	/**
-	 * Check whether a user is allowed to send email
+	 * Authorize the email sending, checking permissions etc.
 	 *
 	 * @param string $editToken
 	 * @return StatusValue For BC, the StatusValue's value can be set to a string representing a message key to use
 	 * with ErrorPageError.
 	 */
-	public function getPermissionsError( string $editToken ): StatusValue {
+	public function authorizeSend( string $editToken ): StatusValue {
 		if (
 			!$this->options->get( MainConfigNames::EnableEmail ) ||
 			!$this->options->get( MainConfigNames::EnableUserEmail )
@@ -194,7 +195,7 @@ class EmailUser {
 
 		$hookErr = false;
 
-		// XXX Replace these hooks with versions that simply use StatusValue for errors.
+		// TODO Remove deprecated hooks
 		$this->hookRunner->onUserCanSendEmail( $user, $hookErr );
 		$this->hookRunner->onEmailUserPermissionsErrors( $user, $editToken, $hookErr );
 		if ( is_array( $hookErr ) ) {
@@ -204,14 +205,17 @@ class EmailUser {
 			$ret->value = $hookErr[0];
 			return $ret;
 		}
+		$hookStatus = StatusValue::newGood();
+		$hookRes = $this->hookRunner->onEmailUserAuthorizeSend( $this->sender, $hookStatus );
+		if ( !$hookRes && !$hookStatus->isGood() ) {
+			return $hookStatus;
+		}
 
 		return StatusValue::newGood();
 	}
 
 	/**
-	 * Really send a mail. Permissions should have been checked using
-	 * getPermissionsError(). It is probably also a good
-	 * idea to check the edit token and ping limiter in advance.
+	 * Really send a mail, without permission checks.
 	 *
 	 * @param UserEmailContact $target
 	 * @param string $subject
@@ -220,7 +224,7 @@ class EmailUser {
 	 * @param MessageLocalizer $messageLocalizer
 	 * @return StatusValue
 	 */
-	public function submit(
+	public function sendEmailUnsafe(
 		UserEmailContact $target,
 		string $subject,
 		string $text,
@@ -259,7 +263,7 @@ class EmailUser {
 		}
 
 		$error = false;
-		// FIXME Replace this hook with a new one that returns errors in a SINGLE format.
+		// TODO Remove deprecated ugly hook
 		if ( !$this->hookRunner->onEmailUser( $toAddress, $fromAddress, $subject, $text, $error ) ) {
 			if ( $error instanceof StatusValue ) {
 				return $error;
@@ -287,6 +291,20 @@ class EmailUser {
 			}
 		}
 
+		$hookStatus = StatusValue::newGood();
+		$hookRes = $this->hookRunner->onEmailUserSendEmail(
+			$this->sender,
+			$fromAddress,
+			$target,
+			$toAddress,
+			$subject,
+			$text,
+			$hookStatus
+		);
+		if ( !$hookRes && !$hookStatus->isGood() ) {
+			return $hookStatus;
+		}
+
 		[ $mailFrom, $replyTo ] = $this->getFromAndReplyTo( $fromAddress, $messageLocalizer );
 
 		$status = $this->emailer->send(

includes/mail/Hook/EmailUserAuthorizeSendHook.php

@@ -0,0 +1,26 @@
+<?php
+
+namespace MediaWiki\Mail\Hook;
+
+use MediaWiki\Permissions\Authority;
+use StatusValue;
+
+/**
+ * This is a hook handler interface, see docs/Hooks.md.
+ * Use the hook name "EmailUserAuthorizeSend" to register handlers implementing this interface.
+ *
+ * @stable to implement
+ * @ingroup Hooks
+ * @since 1.41
+ */
+interface EmailUserAuthorizeSendHook {
+	/**
+	 * This hook is called when checking whether a user is allowed to send emails.
+	 *
+	 * @param Authority $sender
+	 * @param StatusValue $status Add any error here
+	 * @return bool|void True or no return value to continue, false to abort, which also requires adding
+	 * a fatal error to $status.
+	 */
+	public function onEmailUserAuthorizeSend( Authority $sender, StatusValue $status );
+}

includes/mail/Hook/EmailUserSendEmailHook.php

@@ -0,0 +1,40 @@
+<?php
+
+namespace MediaWiki\Mail\Hook;
+
+use MailAddress;
+use MediaWiki\Mail\UserEmailContact;
+use MediaWiki\Permissions\Authority;
+use StatusValue;
+
+/**
+ * This is a hook handler interface, see docs/Hooks.md.
+ * Use the hook name "EmailUserSendEmail" to register handlers implementing this interface.
+ *
+ * @stable to implement
+ * @ingroup Hooks
+ * @since 1.41
+ */
+interface EmailUserSendEmailHook {
+	/**
+	 * This hook is called before sending an email, when all other checks have succeeded.
+	 *
+	 * @param Authority $from
+	 * @param MailAddress $fromAddress MailAddress of the sender
+	 * @param UserEmailContact $to
+	 * @param MailAddress $toAddress MailAddress of the target
+	 * @param string $subject
+	 * @param string $text
+	 * @param StatusValue $status Add any error here
+	 * @return bool|void True or no return value to continue. To abort, return false and add a fatal error to $status.
+	 */
+	public function onEmailUserSendEmail(
+		Authority $from,
+		MailAddress $fromAddress,
+		UserEmailContact $to,
+		MailAddress $toAddress,
+		string $subject,
+		string $text,
+		StatusValue $status
+	);
+}

includes/specials/Hook/EmailUserHook.php

@@ -10,8 +10,8 @@ use Status;
  * This is a hook handler interface, see docs/Hooks.md.
  * Use the hook name "EmailUser" to register handlers implementing this interface.
  *
- * @stable to implement
  * @ingroup Hooks
+ * @deprecated since 1.41 Handle the EmailUserSendEmail hook instead.
  */
 interface EmailUserHook {
 	/**

includes/specials/Hook/EmailUserPermissionsErrorsHook.php

@@ -8,9 +8,8 @@ use User;
  * This is a hook handler interface, see docs/Hooks.md.
  * Use the hook name "EmailUserPermissionsErrors" to register handlers implementing this interface.
  *
- * @stable to implement
  * @ingroup Hooks
- * @deprecated since 1.41 Handle the UserCanSendEmail hook instead.
+ * @deprecated since 1.41 Handle the EmailUserAuthorizeSend hook instead.
  */
 interface EmailUserPermissionsErrorsHook {
 	/**

includes/specials/SpecialEmailUser.php

@@ -267,7 +267,7 @@ class SpecialEmailUser extends UnlistedSpecialPage {
 	 */
 	public static function getPermissionsError( $user, $editToken, Config $config = null ) {
 		$emailUser = MediaWikiServices::getInstance()->getEmailUserFactory()->newEmailUserBC( $user, $config );
-		$status = $emailUser->getPermissionsError( (string)$editToken );
+		$status = $emailUser->authorizeSend( (string)$editToken );
 		if ( $status->isGood() ) {
 			return null;
 		}
@@ -360,7 +360,7 @@ class SpecialEmailUser extends UnlistedSpecialPage {
 		if ( !$target instanceof User ) {
 			return Status::newFatal( 'emailnotarget' );
 		}
-		$res = $this->emailUserFactory->newEmailUser( $this->getAuthority() )->submit(
+		$res = $this->emailUserFactory->newEmailUser( $this->getAuthority() )->sendEmailUnsafe(
 			$target,
 			$data['Subject'],
 			$data['Text'],
@@ -395,7 +395,7 @@ class SpecialEmailUser extends UnlistedSpecialPage {
 		$emailUser = MediaWikiServices::getInstance()->getEmailUserFactory()
 			->newEmailUserBC( $context->getAuthority(), $context->getConfig() );
 
-		$ret = $emailUser->submit(
+		$ret = $emailUser->sendEmailUnsafe(
 			$target,
 			(string)$data['Subject'],
 			(string)$data['Text'],

includes/user/Hook/UserCanSendEmailHook.php

@@ -8,8 +8,8 @@ use User;
  * This is a hook handler interface, see docs/Hooks.md.
  * Use the hook name "UserCanSendEmail" to register handlers implementing this interface.
  *
- * @stable to implement
  * @ingroup Hooks
+ * @deprecated since 1.41, handle the EmailUserAuthorizeSend hook instead.
  */
 interface UserCanSendEmailHook {
 	/**

includes/user/User.php

@@ -3202,7 +3202,7 @@ class User implements Authority, UserIdentity, UserEmailContact {
 		$permError = MediaWikiServices::getInstance()->getEmailUserFactory()
 			->newEmailUser( $this->getThisAsAuthority() )
 			// XXX Pass an empty edit token, nobody is using it anyway.
-			->getPermissionsError( '' );
+			->authorizeSend( '' );
 		return $permError->isGood();
 	}
 

tests/phpunit/unit/includes/mail/EmailUserTest.php

@@ -155,7 +155,7 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 	}
 
 	/**
-	 * @covers ::getPermissionsError
+	 * @covers ::authorizeSend
 	 * @dataProvider providePermissionsError
 	 */
 	public function testGetPermissionsError(
@@ -171,7 +171,7 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 			$this->expectDeprecationAndContinue( '/Use of EmailUserPermissionsErrors hook/' );
 		}
 		$emailUser = $this->getEmailUser( $performerUser, null, null, $userFactory, $configOverrides, $hooks );
-		$this->assertEquals( $expected, $emailUser->getPermissionsError( 'some-token' ) );
+		$this->assertEquals( $expected, $emailUser->authorizeSend( 'some-token' ) );
 	}
 
 	public function providePermissionsError(): Generator {
@@ -269,11 +269,26 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 			true
 		];
 
+		$emailUserAuthorizeSendError = 'my-hook-error';
+		$emailUserAuthorizeSendHooks = [
+			'EmailUserAuthorizeSend' =>
+				static function ( $user, StatusValue $status ) use ( $emailUserAuthorizeSendError ) {
+					$status->fatal( $emailUserAuthorizeSendError );
+					return false;
+				}
+		];
+		yield 'EmailUserAuthorizeSend hook error' => [
+			$validSender,
+			StatusValue::newFatal( $emailUserAuthorizeSendError ),
+			[],
+			$emailUserAuthorizeSendHooks
+		];
+
 		yield 'Successful' => [ $validSender, StatusValue::newGood() ];
 	}
 
 	/**
-	 * @covers ::submit
+	 * @covers ::sendEmailUnsafe
 	 * @dataProvider provideSubmit
 	 */
 	public function testSubmit(
@@ -288,7 +303,7 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 			->with( $target )
 			->willReturn( $target );
 		$emailUser = $this->getEmailUser( $sender, null, null, $userFactory, [], $hooks, $emailer );
-		$res = $emailUser->submit(
+		$res = $emailUser->sendEmailUnsafe(
 			$target,
 			'subject',
 			'text',
@@ -375,6 +390,22 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 			$emailUserHookUsingMessageHooks
 		];
 
+		$emailUserSendEmailHookError = 'some-pretty-hook-error';
+		$emailUserHookUsingStatusHooks = [
+			'EmailUserSendEmail' => static function ( $a, $b, $c, $d, $e, $f, StatusValue $status )
+				use ( $emailUserSendEmailHookError )
+			{
+				$status->fatal( $emailUserSendEmailHookError );
+				return false;
+			}
+		];
+		yield 'EmailUserSendEmail error as a Status' => [
+			$validTarget,
+			$validSender,
+			StatusValue::newFatal( $emailUserSendEmailHookError ),
+			$emailUserHookUsingStatusHooks
+		];
+
 		$emailerErrorStatus = StatusValue::newFatal( 'emailer-error' );
 		$errorEmailer = $this->createMock( IEmailer::class );
 		$errorEmailer->expects( $this->atLeastOnce() )->method( 'send' )->willReturn( $emailerErrorStatus );
@@ -399,7 +430,7 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 	}
 
 	/**
-	 * @covers ::submit
+	 * @covers ::sendEmailUnsafe
 	 */
 	public function testSubmit__rateLimited() {
 		$senderUser = $this->createMock( User::class );
@@ -410,7 +441,7 @@ class EmailUserTest extends MediaWikiUnitTestCase {
 
 		$this->expectException( RuntimeException::class );
 		$this->expectExceptionMessage( 'You are throttled' );
-		$res = $this->getEmailUser( $senderUser, null, null, $senderUserFactory )->submit(
+		$res = $this->getEmailUser( $senderUser, null, null, $senderUserFactory )->sendEmailUnsafe(
 			$this->getValidTarget(),
 			'subject',
 			'text',