Skip to content

Commit 32b3c8f

Browse files
jakubmikitajakubmikita
committed
fix: clone carrier recipients before parsing to prevent shared state mutation
During cron background processing, recipient instances from the shared RecipientStore lost their return_type/return_field context, causing email carriers to receive user IDs instead of email addresses. - Clone recipients in parseRecipients() before calling parseValue() - Add getRecipientReturnField() to BaseCarrier (null by default) - Override in Email carrier to enforce 'user_email' return field - Fire notification/recipient/pre_parse_value filter for extensibility
1 parent 5b7faf3 commit 32b3c8f

6 files changed

Lines changed: 292 additions & 0 deletions

File tree

‎readme.txt‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,7 @@ Yes! We offer [custom plugin development](https://bracketspace.com/custom-develo
235235
== Changelog ==
236236

237237
= [Next] =
238+
* [Fixed] Carrier recipient data lost during cron background processing, causing emails to receive user IDs instead of email addresses
238239
* [Fixed] Warning on activation when cron period setting is not yet initialized
239240

240241
= 9.0.9 =

‎src/Repository/Carrier/BaseCarrier.php‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,20 @@ public function prepareData()
472472
$this->data['parsed_' . $recipientsField->getRawName()] = $this->parseRecipients();
473473
}
474474

475+
/**
476+
* Gets the return field name that this carrier requires from recipients.
477+
*
478+
* Override in subclasses to enforce a specific return field (e.g. 'user_email' for Email).
479+
* When null, the recipient's own return field is used.
480+
*
481+
* @return string|null
482+
* @since [Next]
483+
*/
484+
protected function getRecipientReturnField(): ?string
485+
{
486+
return null;
487+
}
488+
475489
/**
476490
* Parses the recipients to a flat array.
477491
*
@@ -487,6 +501,7 @@ public function parseRecipients()
487501
return [];
488502
}
489503

504+
$carrierReturnField = $this->getRecipientReturnField();
490505
$parsedRecipients = [];
491506

492507
foreach ($this->recipientsResolvedData as $recipientData) {
@@ -503,6 +518,21 @@ public function parseRecipients()
503518
continue;
504519
}
505520

521+
// Clone to avoid mutating shared store instances.
522+
$recipient = clone $recipient;
523+
524+
if ($carrierReturnField !== null && method_exists($recipient, 'setReturnField')) {
525+
$recipient->setReturnField($carrierReturnField);
526+
}
527+
528+
/** @var Interfaces\Receivable $recipient */
529+
$recipient = apply_filters(
530+
'notification/recipient/pre_parse_value',
531+
$recipient,
532+
$this->getSlug(),
533+
$type
534+
);
535+
506536
$parsedRecipients = array_merge(
507537
$parsedRecipients,
508538
(array)$recipient->parseValue($recipientValue)

‎src/Repository/Carrier/Email.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,17 @@ public function formFields()
149149
);
150150
}
151151

152+
/**
153+
* {@inheritdoc}
154+
*
155+
* @return string
156+
* @since [Next]
157+
*/
158+
protected function getRecipientReturnField(): ?string
159+
{
160+
return 'user_email';
161+
}
162+
152163
/**
153164
* Sets mail type to text/html for wp_mail
154165
*

‎tests/unit/Carriers/TestParseRecipients.php‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
<?php
2+
3+
/**
4+
* Tests for BaseCarrier::parseRecipients()
5+
*
6+
* @package notification
7+
*/
8+
9+
namespace BracketSpace\Notification\Tests\Carriers;
10+
11+
use BracketSpace\Notification\Interfaces\Triggerable;
12+
use BracketSpace\Notification\Repository\Carrier\Email;
13+
use BracketSpace\Notification\Store\Recipient as RecipientStore;
14+
use BracketSpace\Notification\Tests\Helpers\Objects\CarrierWithRecipients;
15+
use BracketSpace\Notification\Tests\Helpers\Objects\RecipientWithReturnField;
16+
17+
it('clones recipients before parsing to prevent shared state mutation', function () {
18+
$carrier = new CarrierWithRecipients('clone_test');
19+
$recipient = new RecipientWithReturnField('dummy_return_field', 'user_email');
20+
21+
RecipientStore::insert('clone_test', 'dummy_return_field', $recipient);
22+
23+
$carrier->setRecipientsResolvedData([
24+
['type' => 'dummy_return_field', 'recipient' => 'test_value'],
25+
]);
26+
27+
// Add filter that modifies the recipient it receives.
28+
add_filter('notification/recipient/pre_parse_value', function ($recipientObj) {
29+
if (method_exists($recipientObj, 'setReturnField')) {
30+
$recipientObj->setReturnField('ID');
31+
}
32+
return $recipientObj;
33+
});
34+
35+
$carrier->parseRecipients();
36+
37+
// The original store instance must remain unchanged.
38+
$storeRecipient = RecipientStore::get('clone_test', 'dummy_return_field');
39+
expect($storeRecipient->getReturnField())->toBe('user_email');
40+
41+
remove_all_filters('notification/recipient/pre_parse_value');
42+
});
43+
44+
it('sets return field on recipients supporting HasReturnField when carrier declares one', function () {
45+
$carrier = new class ('return_field_test') extends CarrierWithRecipients {
46+
protected function getRecipientReturnField(): ?string
47+
{
48+
return 'ID';
49+
}
50+
};
51+
52+
$recipient = new RecipientWithReturnField('dummy_return_field', 'user_email');
53+
RecipientStore::insert('return_field_test', 'dummy_return_field', $recipient);
54+
55+
$carrier->setRecipientsResolvedData([
56+
['type' => 'dummy_return_field', 'recipient' => 'val'],
57+
]);
58+
59+
$result = $carrier->parseRecipients();
60+
61+
// The carrier declares 'ID', so parsed output should use 'ID', not 'user_email'.
62+
expect($result)->toBe(['ID:val']);
63+
});
64+
65+
it('does not override return field when carrier returns null', function () {
66+
$carrier = new CarrierWithRecipients('null_field_test');
67+
68+
$recipient = new RecipientWithReturnField('dummy_return_field', 'ID');
69+
RecipientStore::insert('null_field_test', 'dummy_return_field', $recipient);
70+
71+
$carrier->setRecipientsResolvedData([
72+
['type' => 'dummy_return_field', 'recipient' => 'val'],
73+
]);
74+
75+
$result = $carrier->parseRecipients();
76+
77+
// Carrier returns null for getRecipientReturnField, so recipient's own 'ID' should be used.
78+
expect($result)->toBe(['ID:val']);
79+
});
80+
81+
it('fires notification/recipient/pre_parse_value filter', function () {
82+
$carrier = new CarrierWithRecipients('filter_test');
83+
$recipient = new RecipientWithReturnField('dummy_return_field', 'user_email');
84+
85+
RecipientStore::insert('filter_test', 'dummy_return_field', $recipient);
86+
87+
$carrier->setRecipientsResolvedData([
88+
['type' => 'dummy_return_field', 'recipient' => 'test_value'],
89+
]);
90+
91+
$filterCalled = false;
92+
$capturedArgs = [];
93+
94+
add_filter('notification/recipient/pre_parse_value', function ($recipientObj, $carrierSlug, $type) use (&$filterCalled, &$capturedArgs) {
95+
$filterCalled = true;
96+
$capturedArgs = [
97+
'carrier_slug' => $carrierSlug,
98+
'type' => $type,
99+
];
100+
return $recipientObj;
101+
}, 10, 3);
102+
103+
$carrier->parseRecipients();
104+
105+
expect($filterCalled)->toBeTrue();
106+
expect($capturedArgs['carrier_slug'])->toBe('filter_test');
107+
expect($capturedArgs['type'])->toBe('dummy_return_field');
108+
109+
remove_all_filters('notification/recipient/pre_parse_value');
110+
});
111+
112+
it('returns user_email from getRecipientReturnField for Email carrier', function () {
113+
$email = new Email();
114+
115+
$reflection = new \ReflectionMethod($email, 'getRecipientReturnField');
116+
$reflection->setAccessible(true);
117+
118+
expect($reflection->invoke($email))->toBe('user_email');
119+
});
120+
121+
it('returns null from getRecipientReturnField by default', function () {
122+
$carrier = new CarrierWithRecipients();
123+
124+
$reflection = new \ReflectionMethod($carrier, 'getRecipientReturnField');
125+
$reflection->setAccessible(true);
126+
127+
expect($reflection->invoke($carrier))->toBeNull();
128+
});

‎tests/unit/Helpers/Objects/CarrierWithRecipients.php‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
<?php
2+
3+
/**
4+
* Carrier with recipients field for testing
5+
*
6+
* @package notification
7+
*/
8+
9+
declare(strict_types=1);
10+
11+
namespace BracketSpace\Notification\Tests\Helpers\Objects;
12+
13+
use BracketSpace\Notification\Repository\Carrier\BaseCarrier;
14+
use BracketSpace\Notification\Interfaces\Triggerable;
15+
16+
/**
17+
* Carrier with recipients field
18+
*/
19+
class CarrierWithRecipients extends BaseCarrier
20+
{
21+
/**
22+
* Constructor
23+
*
24+
* @param string $slug Carrier slug.
25+
*/
26+
public function __construct(string $slug = 'test_carrier')
27+
{
28+
parent::__construct($slug, 'Test Carrier');
29+
}
30+
31+
/**
32+
* {@inheritdoc}
33+
*/
34+
public function formFields()
35+
{
36+
$this->addRecipientsField();
37+
}
38+
39+
/**
40+
* {@inheritdoc}
41+
*
42+
* @param Triggerable $trigger Trigger object.
43+
*/
44+
public function send(Triggerable $trigger)
45+
{
46+
}
47+
48+
/**
49+
* Exposes protected recipientsResolvedData for testing
50+
*
51+
* @param array<mixed> $data Resolved data.
52+
*/
53+
public function setRecipientsResolvedData(array $data)
54+
{
55+
$this->recipientsResolvedData = $data;
56+
}
57+
}

‎tests/unit/Helpers/Objects/RecipientWithReturnField.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
<?php
2+
3+
/**
4+
* Recipient with HasReturnField trait for testing
5+
*
6+
* @package notification
7+
*/
8+
9+
declare(strict_types=1);
10+
11+
namespace BracketSpace\Notification\Tests\Helpers\Objects;
12+
13+
use BracketSpace\Notification\Repository\Recipient\BaseRecipient;
14+
use BracketSpace\Notification\Repository\Field\InputField;
15+
use BracketSpace\Notification\Traits\HasReturnField;
16+
17+
/**
18+
* Recipient with HasReturnField trait
19+
*/
20+
class RecipientWithReturnField extends BaseRecipient
21+
{
22+
use HasReturnField;
23+
24+
/**
25+
* Constructor
26+
*
27+
* @param string $slug Recipient slug.
28+
* @param string $returnField Return field name.
29+
*/
30+
public function __construct(string $slug = 'dummy_return_field', string $returnField = 'user_email')
31+
{
32+
parent::__construct([
33+
'slug' => $slug,
34+
'name' => 'Dummy',
35+
'default_value' => '',
36+
]);
37+
38+
$this->setReturnField($returnField);
39+
}
40+
41+
/**
42+
* {@inheritdoc}
43+
*
44+
* @param string $value Raw value.
45+
* @return array<mixed>
46+
*/
47+
public function parseValue($value = '')
48+
{
49+
return [$this->getReturnField() . ':' . $value];
50+
}
51+
52+
/**
53+
* {@inheritdoc}
54+
*
55+
* @return object
56+
*/
57+
public function input()
58+
{
59+
return new InputField([
60+
'label' => 'Recipient',
61+
'name' => 'recipient',
62+
'css_class' => 'recipient-value',
63+
]);
64+
}
65+
}

0 commit comments

Comments
 (0)