Manually fix coding style violations

Add visibility keywords to constants
Move & improve in-line comments

src/Authorization/DonationAuthorizer.php

@@ -12,16 +12,28 @@ interface DonationAuthorizer {
 
 	/**
 	 * Should return false on infrastructure failure.
+	 *
+	 * @param int $donationId
+	 *
+	 * @return bool
 	 */
 	public function userCanModifyDonation( int $donationId ): bool;
 
 	/**
 	 * Should return false on infrastructure failure.
+	 *
+	 * @param int $donationId
+	 *
+	 * @return bool
 	 */
 	public function systemCanModifyDonation( int $donationId ): bool;
 
 	/**
 	 * Should return false on infrastructure failure.
+	 *
+	 * @param int $donationId
+	 *
+	 * @return bool
 	 */
 	public function canAccessDonation( int $donationId ): bool;
 

src/DataAccess/DoctrineEntities/Donation.php

@@ -28,10 +28,17 @@
  */
 class Donation {
 
-	public const STATUS_NEW = 'N'; // status for direct debit
-	public const STATUS_PROMISE = 'Z'; // status for bank transfer
-	public const STATUS_EXTERNAL_INCOMPLETE = 'X'; // status for external payments
-	public const STATUS_EXTERNAL_BOOKED = 'B'; // status for external payments
+	// direct debit
+	public const STATUS_NEW = 'N';
+
+	// bank transfer
+	public const STATUS_PROMISE = 'Z';
+
+	// external payment, not notified by payment provider
+	public const STATUS_EXTERNAL_INCOMPLETE = 'X';
+
+	// external payment, notified by payment provider
+	public const STATUS_EXTERNAL_BOOKED = 'B';
 	public const STATUS_MODERATION = 'P';
 	public const STATUS_CANCELLED = 'D';
 

src/Domain/Model/Donation.php

@@ -15,18 +15,23 @@
  */
 class Donation {
 
-	public const STATUS_NEW = 'N'; // status for direct debit
-	public const STATUS_PROMISE = 'Z'; // status for bank transfer
-	public const STATUS_EXTERNAL_INCOMPLETE = 'X'; // status for external payments
-	public const STATUS_EXTERNAL_BOOKED = 'B'; // status for external payments
+	// direct debit
+	public const STATUS_NEW = 'N';
+
+	// bank transfer
+	public const STATUS_PROMISE = 'Z';
+
+	// external payment, not notified by payment provider
+	public const STATUS_EXTERNAL_INCOMPLETE = 'X';
+
+	// external payment, notified by payment provider
+	public const STATUS_EXTERNAL_BOOKED = 'B';
 	public const STATUS_MODERATION = 'P';
 	public const STATUS_CANCELLED = 'D';
 
 	public const OPTS_INTO_NEWSLETTER = true;
 	public const DOES_NOT_OPT_INTO_NEWSLETTER = false;
 
-	public const NO_APPLICANT = null;
-
 	private $id;
 	private $status;
 	private $donor;
@@ -134,6 +139,10 @@ public function getPaymentMethodId(): string {
 
 	/**
 	 * Returns the Donor or null for anonymous donations.
+	 *
+	 * @todo Return AnonymousDonor instead
+	 *
+	 * @return Donor|null
 	 */
 	public function getDonor(): ?Donor {
 		return $this->donor;
@@ -143,9 +152,6 @@ public function setDonor( Donor $donor ): void {
 		$this->donor = $donor;
 	}
 
-	/**
-	 * Returns the DonationComment or null for when there is none.
-	 */
 	public function getComment(): ?DonationComment {
 		return $this->comment;
 	}
@@ -170,9 +176,6 @@ public function getOptsIntoNewsletter(): bool {
 		return $this->optsIntoNewsletter;
 	}
 
-	/**
-	 * @throws RuntimeException
-	 */
 	public function cancel(): void {
 		if ( $this->getPaymentMethodId() !== PaymentMethod::DIRECT_DEBIT ) {
 			throw new RuntimeException( 'Can only cancel direct debit' );

src/DonationContextFactory.php

@@ -85,8 +85,11 @@ private function getTokenGenerator(): TokenGenerator {
 		return $this->tokenGenerator;
 	}
 
-	// Setters for switching out classes in tests
-
+	/**
+	 * Should only be called in tests for switching out the default implementation
+	 *
+	 * @param TokenGenerator|null $tokenGenerator
+	 */
 	public function setTokenGenerator( ?TokenGenerator $tokenGenerator ): void {
 		$this->tokenGenerator = $tokenGenerator;
 	}

src/Infrastructure/DonationConfirmationMailer.php

@@ -37,7 +37,7 @@ private function getConfirmationMailTemplateArguments( Donation $donation ): arr
 			],
 			'donation' => [
 				'id' => $donation->getId(),
-				'amount' => $donation->getAmount()->getEuroFloat(), // number is formatted in template
+				'amount' => $donation->getAmount()->getEuroFloat(),
 				'interval' => $donation->getPaymentIntervalInMonths(),
 				'needsModeration' => $donation->needsModeration(),
 				'paymentType' => $donation->getPaymentMethodId(),

src/UseCases/AddDonation/AddDonationRequest.php

@@ -44,7 +44,7 @@ class AddDonationRequest {
 
 	# tracking
 	private $tracking = '';
-	private $source = ''; # TODO: generated from referer
+	private $source = '';
 	private $totalImpressionCount = 0;
 	private $singleBannerImpressionCount = 0;
 	// Legacy values, will be deprecated in the future
@@ -126,21 +126,21 @@ public function setSingleBannerImpressionCount( int $singleBannerImpressionCount
 		$this->singleBannerImpressionCount = $singleBannerImpressionCount;
 	}
 
-	/*
+	/**
 	 * @deprecated
 	 */
 	public function getColor(): string {
 		return $this->color;
 	}
 
-	/*
+	/**
 	 * @deprecated
 	 */
 	public function getSkin(): string {
 		return $this->skin;
 	}
 
-	/*
+	/**
 	 * @deprecated
 	 */
 	public function getLayout(): string {

src/UseCases/AddDonation/AddDonationResponse.php

@@ -62,9 +62,10 @@ public function isSuccessful(): bool {
 	}
 
 	/**
-	 * WARNING: we're returning the domain object to not have to create a  more verbose response model.
-	 * Keep in mind that you should not use domain logic in the presenter, or put presentation helpers
-	 * in the domain object!
+	 * ATTENTION: We're returning the domain object in order to avoid a verbose read-only response model.
+	 * Keep in mind that your presenters should only query the domain object
+	 * and NOT call any of its state-changing methods!
+	 * @return Donation|null
 	 */
 	public function getDonation(): ?Donation {
 		return $this->donation;

src/UseCases/AddDonation/AddDonationUseCase.php

@@ -29,8 +29,8 @@
  */
 class AddDonationUseCase {
 
-	const PREFIX_BANK_TRANSACTION_KNOWN_DONOR = 'XW';
-	const PREFIX_BANK_TRANSACTION_ANONYNMOUS_DONOR = 'XR';
+	private const PREFIX_BANK_TRANSACTION_KNOWN_DONOR = 'XW';
+	private const PREFIX_BANK_TRANSACTION_ANONYNMOUS_DONOR = 'XR';
 
 	private DonationRepository $donationRepository;
 	private AddDonationValidator $donationValidator;

src/UseCases/GetDonation/GetDonationResponse.php

@@ -30,6 +30,12 @@ private function __construct( Donation $donation = null, string $updateToken = n
 
 	/**
 	 * Returns the Donation when @see accessIsPermitted returns true, or null otherwise.
+	 *
+	 * ATTENTION: We're returning the domain object in order to avoid a verbose read-only response model.
+	 * Keep in mind that your presenters should only query the domain object
+	 * and NOT call any of its state-changing methods
+	 *
+	 * @return Donation|null
 	 */
 	public function getDonation(): ?Donation {
 		return $this->donation;

src/UseCases/GetDonation/GetDonationUseCase.php

@@ -39,7 +39,8 @@ public function showConfirmation( GetDonationRequest $request ): GetDonationResp
 		}
 
 		return GetDonationResponse::newValidResponse(
-			$donation, // TODO: create a DTO to not expose the Donation Entity beyond the UC layer
+			// TODO: create a DTO to not expose the Donation Entity beyond the UC layer
+			$donation,
 			$this->tokenFetcher->getTokens( $request->getDonationId() )->getUpdateToken()
 		);
 	}

src/UseCases/HandlePayPalPaymentNotification/HandlePayPalPaymentCompletionNotificationUseCase.php

@@ -244,16 +244,14 @@ private function newDonationFromRequest( PayPalPaymentNotificationRequest $reque
 		);
 	}
 
-	private function createErrorResponse( \Exception $ex ): PaypalNotificationResponse {
-		return PaypalNotificationResponse::newFailureResponse(
-			[
-				'message' => $ex->getMessage(),
-				'stackTrace' => $ex->getTraceAsString()
-			]
-		);
-	}
-
-	// TODO Move this check to the payment domain use case, see https://phabricator.wikimedia.org/T192323
+	/**
+	 * @todo Move this check to the payment domain use case, see https://phabricator.wikimedia.org/T192323
+	 *
+	 * @param Donation $donation
+	 * @param string $transactionId
+	 *
+	 * @return bool
+	 */
 	private function donationWasBookedWithSameTransactionId( Donation $donation, string $transactionId ): bool {
 		/**
 		 * @var PayPalPayment $payment
@@ -267,4 +265,13 @@ private function donationWasBookedWithSameTransactionId( Donation $donation, str
 		return $payment->getPayPalData()->hasChildPayment( $transactionId );
 	}
 
+	private function createErrorResponse( \Exception $ex ): PaypalNotificationResponse {
+		return PaypalNotificationResponse::newFailureResponse(
+			[
+				'message' => $ex->getMessage(),
+				'stackTrace' => $ex->getTraceAsString()
+			]
+		);
+	}
+
 }

tests/Data/ValidDonation.php

@@ -44,7 +44,8 @@ class ValidDonation {
 
 	public const DONOR_EMAIL_ADDRESS = '[email protected]';
 
-	public const DONATION_AMOUNT = 13.37; // Keep fractional to detect floating point issues
+	// Use fractional value to detect floating point issues
+	public const DONATION_AMOUNT = 13.37;
 	public const PAYMENT_INTERVAL_IN_MONTHS = 3;
 
 	public const PAYMENT_BANK_ACCOUNT = '0648489890';
@@ -63,7 +64,8 @@ class ValidDonation {
 	public const TRACKING_SKIN = 'default';
 	public const TRACKING_SOURCE = 'web';
 	public const TRACKING_TOTAL_IMPRESSION_COUNT = 3;
-	public const TRACKING_TRACKING = 'test/gelb'; // WTF name?
+	// "tracking" is the name of the property on the object, "TRACKING" is our prefix, hence TRACKING_TRACKING
+	public const TRACKING_TRACKING = 'test/gelb';
 
 	public const PAYPAL_TRANSACTION_ID = '61E67681CH3238416';
 

tests/Fixtures/DonationRepositorySpy.php

@@ -18,11 +18,12 @@ class DonationRepositorySpy extends FakeDonationRepository {
 
 	public function __construct( Donation ...$donations ) {
 		parent::__construct( ...$donations );
-		$this->storeDonationCalls = []; // remove calls coming from initialization
+		$this->storeDonationCalls = [];
 	}
 
 	public function storeDonation( Donation $donation ): void {
-		$this->storeDonationCalls[] = clone $donation; // protect against the donation being changed later
+		// protect against the donation being changed later
+		$this->storeDonationCalls[] = clone $donation;
 		parent::storeDonation( $donation );
 	}
 

tests/Integration/DataAccess/SerializedDataHandlingTest.php

@@ -11,8 +11,7 @@
 use WMDE\Fundraising\DonationContext\Tests\TestEnvironment;
 
 /**
- * @license GPL-2.0-or-later
- * @author Kai Nissen < [email protected] >
+ * @covers \WMDE\Fundraising\DonationContext\DataAccess\DoctrineDonationRepository
  */
 class SerializedDataHandlingTest extends \PHPUnit\Framework\TestCase {
 

tests/Integration/UseCases/HandlePayPalPaymentNotification/HandlePayPalPaymentCompletionNotificationUseCaseTest.php

@@ -432,7 +432,8 @@ public function testWhenNotificationIsForNonExistingDonation_bookingEventIsLogge
 
 		$useCase->handleNotification( $request );
 
-		$this->assertEventLogContainsExpression( $eventLogger, 1, '/booked/' ); // 1 is the generated donation id
+		$expectedAutogeneratedDonationId = 1;
+		$this->assertEventLogContainsExpression( $eventLogger, $expectedAutogeneratedDonationId, '/booked/' );
 	}
 
 	private function assertDonationIsCreatedWithNotficationRequestData( Donation $donation ): void {

tests/Unit/UseCases/UpdateDonor/UpdateDonorValidatorTest.php

@@ -59,7 +59,7 @@ public function testGivenFailingDonorValidator_validationFails() {
 		);
 	}
 
-	public function testgivenEmptyDonorRequestValues_validationFails() {
+	public function testGivenEmptyDonorRequestValues_validationFails() {
 		$validator = new UpdateDonorValidator(
 			new AddressValidator( ValidatorPatterns::COUNTRY_POSTCODE, ValidatorPatterns::ADDRESS_PATTERNS ),
 			new EmailValidator( new SucceedingDomainNameValidator() )