Page MenuHomePhorge
Differential D1102 Diff 2866 src/app/Transaction.php

Changeset View

Standalone View

View Options

src/app/Transaction.php

  • This file was moved from src/app/Payment.php.
<?php <?php
namespace App; namespace App;
use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Model;
use Illuminate\Database\Eloquent\SoftDeletes;
/** /**
* A payment operation on a wallet. * A transaction.
* *
* @property string $wallet_id The ID of the wallet
* @property int $amount Amount of money in cents * @property int $amount Amount of money in cents
* @property string $description Payment description * @property string $description Payment description
* @property string $id Mollie's Payment ID * @property string $params Description parameters
* @property int $wallet_id The ID of the wallet * @property string $mollie_id Mollie's Payment ID
*/ */
class Payment extends Model class Transaction extends Model
{ {
use SoftDeletes;
public $incrementing = false; public $incrementing = false;
protected $keyType = 'string'; protected $keyType = 'string';
protected $casts = [ protected $casts = [
'amount' => 'integer' 'amount' => 'integer'
]; ];
protected $fillable = [
'wallet_id',
'amount',
'description',
'params',
'mollie_id',
'mollie_status'
];
/** /**
* The wallet to which this payment belongs. * The wallet to which this payment belongs.
* *
* @return \Illuminate\Database\Eloquent\Relations\BelongsTo * @return \Illuminate\Database\Eloquent\Relations\BelongsTo
*/ */
public function wallet() public function wallet()
{ {
return $this->belongsTo( return $this->belongsTo(
'\App\Wallet', '\App\Wallet',
'wallet_id', /* local */ 'wallet_id', /* local */
'id' /* remote */ 'id' /* remote */
); );
} }
} }
machniakUnsubmitted
Done
Inline Actions

I would propose to create public class constants for these labels.

machniak: I would propose to create public class constants for these labels.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

And I completely understand why -- I've already missed "credited" vs. "credit" ;-)

vanmeeuwen: And I completely understand why -- I've already missed "credited" vs. "credit" ;-)
machniakUnsubmitted
Done
Inline Actions

Why syntax with quotes+brackets?

machniak: Why syntax with quotes+brackets?
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

description is considered a keyword by my highlighter. It happens elsewhere for 'value' and 'key' as well, although these I believe are also different PHP or SQL implementation things.

vanmeeuwen: `description` is considered a keyword by my highlighter. It happens elsewhere for 'value' and…
machniakUnsubmitted
Done
Inline Actions

We call entitlement() so many times that we should cache it (in-memory) here or do some magic.

machniak: We call entitlement() so many times that we should cache it (in-memory) here or do some magic.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

It should be in the SQL query cache, but it can of course also be cached "here".

Caching overall shall be a different revision, because it would principally modify a number of approaches in a large number of places wrt. to proper cache invalidation.

vanmeeuwen: It should be in the SQL query cache, but it can of course also be cached "here". Caching…
machniakUnsubmitted
Done
Inline Actions

Would be better to call ->entitlement() once, The object_type check is redundant.

machniak: Would be better to call ->entitlement() once, The object_type check is redundant.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

While I'm aware it doesn't address your remark as to the duplicate execution of the $this->entitlement() method;

I personally very much dislike the construct of the following where the clause somewhat depends on function successful, as well as not false, as well as not null, and consider it a dysfunction of the language implementation.

I.e., very much dislike this;

if ($var = function()) {
}

I would rather;

$var = function();
if ($var) {
}

In the case above, I would therefore rewrite as;

if ($this->object_type == \App\Entitlement::class) {
  $smt = $this->entitlement();

  return $smt ? $smt->wallet()->{'description'} : 'Default wallet';
}

I don't know if that's what you were aiming at though.

vanmeeuwen: While I'm aware it doesn't address your remark as to the duplicate execution of the `$this…
machniakUnsubmitted
Done
Inline Actions

My version would probably looked like this:

if ($entitlement = $this->entitlement()) {
    $description = $entitlement->wallet->{'description'};
} elseif ($wallet = $this->wallet()) {
    $description = $wallet->{'description'};
}

return !empty($description) ? $description : 'Default wallet';
machniak: My version would probably looked like this: ``` if ($entitlement = $this->entitlement()) {…
machniakUnsubmitted
Done
Inline Actions

This line could be made a last line in this function. This way you would de-deduplicate code.

machniak: This line could be made a last line in this function. This way you would de-deduplicate code.
vanmeeuwenAuthorUnsubmitted
Done
Inline Actions

I'm not confident the only types of objects to recognize are entitlement|wallet.

I'm also not confident we will not have to insert log statements in between the retrieval and the return -- hence the assignment to the function-local variable.

vanmeeuwen: I'm not confident the only types of objects to recognize are entitlement|wallet. I'm also not…
machniakUnsubmitted
Done
Inline Actions

Use $entitlement var.

machniak: Use $entitlement var.
machniakUnsubmitted
Done
Inline Actions

If $description is always initialized, we don't have to use empty(), i.e. it could be: return $description ?: 'Default wallet';.

machniak: If $description is always initialized, we don't have to use empty(), i.e. it could be: `return…