Model::__isset() is attribute-only while __get() is extension-aware, so `$model->extensionProperty ?? $default` silently returns the default
Nobody has claimed this yet.
- Dominant language
- PHP
- Stars
- 1.5k
- Forks
- 246
- Avg merge
- 19h 2m
- Merged PRs (30d)
- 7
Description
Summary
Winter\Storm\Database\Model::__isset() resolves attributes only, while __get() resolves through extendableGet(). The two disagree for any property provided by the extension system — addDynamicProperty() values and public properties declared on a Behavior.
The practical consequence is that $model->someExtensionProperty ?? $default always yields $default, silently, even though reading the property directly returns the right value. Plain Extendable objects do not have this problem, so the behaviour is inconsistent within Winter itself.
Reproduction
No plugins required:
// A — plain Extendable (no __isset defined)
$obj = new class extends \Winter\Storm\Extension\Extendable {};
$obj->addDynamicProperty('foo', 'bar');
$obj->foo; // 'bar'
isset($obj->foo); // false
$obj->foo ?? 'DEFAULT'; // 'bar' <-- works
// B — Database\Model
\System\Models\EventLog::extend(function ($m) {
$m->addDynamicProperty('foo', 'bar');
});
$m = new \System\Models\EventLog;
$m->foo; // 'bar'
$m->propertyExists('foo'); // true
isset($m->foo); // false
$m->foo ?? 'DEFAULT'; // 'DEFAULT' <-- silently wrong
// C — a property declared on a Behaviour, same result
class DemoBehavior extends \Winter\Storm\Extension\ExtensionBase {
public $demoProp = 'from behaviour';
}
\System\Models\Parameter::extend(fn ($m) => $m->extendClassWith(DemoBehavior::class));
(new \System\Models\Parameter)->demoProp; // 'from behaviour'
isset((new \System\Models\Parameter)->demoProp); // false
Why ?? breaks on models but not on Extendable
This is the part that makes it easy to miss. PHP's null-coalescing operator consults __isset() only if the class defines one; with __get() alone it falls through to __get():
class WithGetOnly { public function __get($n) { return 'bar'; } }
class WithGetAndIsset { public function __get($n) { return 'bar'; }
public function __isset($n) { return false; } }
(new WithGetOnly)->foo ?? 'DEFAULT'; // 'bar'
(new WithGetAndIsset)->foo ?? 'DEFAULT'; // 'DEFAULT'
Extendable defines no __isset (method_exists($obj, '__isset') === false), so it lands in the first case and ?? works. Database\Model defines one, so it lands in the second and ?? breaks.
So the model's __isset() is not merely failing to report extension properties — it is removing working behaviour that exists everywhere else in Winter.
Source
Winter\Storm\Database\Model (src/Database/Model.php, line 810 on both 1.2 and wip/1.3):
public function __get($name)
{
return $this->extendableGet($name); // extension-aware
}
public function __isset($key)
{
return !is_null($this->getAttribute($key)); // Eloquent only
}
Winter\Storm\Halcyon\Model::__isset() (line 1683) has the same shape — attributes and get-mutators only, no extendableGet() — so Halcyon models are presumably affected too, though I have only reproduced this on the database model.
How this shows up in practice
Found in LukeTowers.EasyAudit, where per-model overrides applied through its modelsToTrack config had no effect at all. The plugin registers them with addDynamicProperty() and reads them back as $this->subject?->trackableIgnoredAttributes ?? [], so every override resolved to the default — an audit log recording attributes the config explicitly excluded, and, less visibly, per-model IP/user-agent/URL logging opt-outs being ignored.
It went unnoticed for a long time because models that declare these as real class properties behave correctly; only the dynamically-registered path fails. Fixed plugin-side in LukeTowers/wn-easyaudit-plugin#8 by not coalescing on a property access, but the underlying asymmetry seems worth addressing.
Worth noting the trap is easy to fall back into: the first draft of that plugin fix still ended in $subject->$property ?? $default and reproduced the bug exactly.
Where it can surface silently
__isset() is not only reached through isset():
empty($model->foo)$model->foo ?? $default- Twig
{{ model.foo is defined }}and{{ model.foo|default(...) }}
Each of these currently reports "not set" for a property that reads back fine.
The compatibility question
Making __isset() extension-aware is a behavioural change across every model and behaviour, so it seems like a minor-release item rather than a patch, and I did not want to open a PR before the semantics call is made. The things that seem worth weighing:
-
isset()starts returning true for behaviour-declared properties. Any code branching onisset($model->foo)where some behaviour happens to declarefoochanges meaning. On the site this was found on,Winter.Searchputs asearchableproperty on every indexed model, so this is not hypothetical. -
Eloquent's null semantics.
isset()on an Eloquent attribute means "present and not null". Dynamic properties would need to follow the same rule to stay consistent — an explicitly-null dynamic property should presumably reportfalse. -
Double resolution / lazy loading.
extendableGet()falls back to the parent__get(), so a naive implementation that tries attributes and thenextendableGet()would resolve attributes twice, and can trigger relation lazy-loading during what looks like a cheap existence check.__isset()should probably consultextensionDatadirectly rather than routing throughextendableGet(). -
__unset()has the same asymmetry and would want considering alongside it.
A conservative version — check extensionData['dynamicProperties'] and extension objects directly, without touching the existing attribute path — would fix the reported problem while keeping attribute semantics untouched. But that still flips (1), so it is a judgement call for the maintainers.
Environment
- Winter CMS build 1.2.13
winter/stormdev-wip/1.3(e8424705); the same code is present on the1.2branch at the same line- PHP 8.3.24
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in src/Database/Model.php at __get() and __isset(), then compare the corresponding __isset() implementation in Winter\Storm\Halcyon\Model. Reproduce the dynamic-property and behavior-property cases from the issue, including isset() and ??, and review the compatibility questions before deciding the intended semantics. Done means model existence checks no longer disagree with extension-aware property reads, with the affected cases covered by regression tests.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- laravel, php
- Domain
- backend, database
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100