Fix settings model

[mjauvin]

No description provided.

[mjauvin]

The alternative is to fix it at the Database/Model level with a code snippet like this:

diff --git a/src/Database/Model.php b/src/Database/Model.php
index 40390596..2b56cadf 100644
--- a/src/Database/Model.php
+++ b/src/Database/Model.php
@@ -1378,6 +1378,12 @@ public function setAttribute($key, $value)
 
         $result = parent::setAttribute($key, $value);
 
+        if ($this->hasSetMutator($key)) {
+            if ($_value = parent::getAttributeValue($key)) {
+                $value = $_value;
+            }
+        }
+
         /**
          * @event model.setAttribute
          * Called after the model attribute is set
diff --git a/src/Halcyon/Model.php b/src/Halcyon/Model.php
index f51c3ac4..3322dd41 100644
--- a/src/Halcyon/Model.php
+++ b/src/Halcyon/Model.php
@@ -786,6 +786,8 @@ public function setAttribute($key, $value)
             // post processing logic when an attribute is set with explicit mutators. Returning from the mutator
             // call will also break method chaining as intended by returning `$this` at the end of this method.
             $this->{$method}($value);
+
+            $value = array_get($this->attributes, $key);
         }
         else {
             $this->attributes[$key] = $value;

[bennothommo]

@mjauvin any chance we can have a unit test for this scenario? Best to cover it while we're fixing it.

[mjauvin]

@mjauvin any chance we can have a unit test for this scenario? Best to cover it while we're fixing it.

Sure. Any feedback on my comment for doing this on the base Model ?

[bennothommo]

@mjauvin thank you for asking that question - it allowed me to deep-dive another hell hole 😂.

TL;DR: Let's can this fix, there's a better fix.

The parent Laravel model setAttribute() method already checks for the presence of a mutator, and returns whatever the mutator returns as the result. We, in turn, return the result of the parent setAttribute() method as the result of our own method. However, this result isn't passed on to the model.setAttribute event in any way - we only pass the key and the value, which besides being trimmed, or JSON encoded, or affected in some way by the model.beforeSetAttribute event, is untouched.

I would say the reason for that is because mutators, by design, aren't limited to just mutating the given attribute. For example, you could set up a mutator that takes a start date that automatically mutates the end date to a month later.

So I would suggest that perhaps, a more robust fix to the main Model class would be to in fact call $this->getAttribute() when firing the model.setAttribute event.

Instead of:

$this->fireEvent('model.setAttribute', [$key, $value]);

It should perhaps be:

$this->fireEvent('model.setAttribute', [$key, $this->getAttribute($key)]);

I'm not sure of whether this will break something - if so, maybe it could be added as a third parameter for the event, and the Settings model could be updated to suit.

[bennothommo]

@mjauvin I've proposed an alternate fix here (#1080) to shore up the API and incorporate @LukeTowers concerns about the event change, in case we don't end up changing the event.

[mjauvin]

Frankly, I don't like this.

[bennothommo]

@mjauvin may I ask what is the issue with it?

[mjauvin]

I think fixing the event is the best way to resolve the issue everywhere. That event makes no sense the way it is now.