Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Model mutators not working using SettingsModel behavior #1047

Open
goldmont opened this issue Jan 25, 2024 · 16 comments · May be fixed by #1080
Open

Model mutators not working using SettingsModel behavior #1047

goldmont opened this issue Jan 25, 2024 · 16 comments · May be fixed by #1080
Assignees
Milestone

Comments

@goldmont
Copy link

Winter CMS Build

dev-develop

PHP Version

8.1

Database engine

MySQL/MariaDB

Plugins installed

No response

Issue description

Hi,

The latest version of WinterCMS is affected by an issue that prevents values returned by mutators to be persisted to the database. I read the SettingsModel behavior code and it seems that model $attributes are mistakenly overwritten by "original" values.

Steps to replicate

  1. Create a Settings model.
  2. Implement a mutator and return a different value for that form field.
  3. Read the persisted value.

Workaround

Apparently the only solution is to override the beforeSave() method inside the settings model and to manipulate the JSON contained into $this->attributes['value'] by first deserializing it and then serializing it back.

Example:

protected function beforeSave()
{
	$fields = json_decode($this->attributes['value'], true);
	$fields['token'] = Crypt::encrypt($fields['token'] ?? '');
	$this->attributes['value'] = json_encode($fields);
}
@goldmont goldmont added needs review Issues/PRs that require a review from a maintainer Type: Unconfirmed Bug labels Jan 25, 2024
@goldmont
Copy link
Author

Hi,

As a workaround, to make the code cleaner, it seems that the Encryptable trait works fine.

@LukeTowers
Copy link
Member

@goldmont could you provide an example of what you tried to do that didn't work?

@goldmont
Copy link
Author

goldmont commented Feb 21, 2024

@goldmont could you provide an example of what you tried to do that didn't work?

Hi, sure.

test/archive/Plugin.php

public function registerSettings()
{
	return [
		'settings' => [
			'label' => 'test.archive::lang.models.settings.title',
			'description' => 'test.archive::lang.models.settings.description',
			'category' => 'test.archive::lang.menu.init',
			'icon' => 'icon-gear',
			'class' => Settings::class,
			'order' => 500,
			'keywords' => 'settings setup'
		]
	];
}

test/archive/models/settings/fields.yaml

fields:
  token:
    label: 'test.archive::lang.models.general.token'
    span: left
    type: sensitive

test/archive/models/Settings.php

<?php namespace Test\Archive\Models;

use Illuminate\Support\Facades\Crypt;
use Model;
use Winter\Storm\Database\Traits\Validation;

class Settings extends Model
{
	use Validation;

	public $implement = [\System\Behaviors\SettingsModel::class];

	public $settingsCode = 'test_archive_settings';

	public $settingsFields = 'fields.yaml';

	public $settingsCacheTtl = 0;

	public $rules = [];

	public function getTokenAttribute()
	{
		$value = $this->attributes['token'] ?? '';
		return !empty($value) ? Crypt::decrypt($value) : null;
	}

	public function setTokenAttribute($value): void
	{
		$this->attributes['token'] = Crypt::encrypt($value ?? '');
	}
}

The value set into $this->attributes gets lost before save. I had to implement beforeSave() method as I said before.

@LukeTowers
Copy link
Member

@mjauvin any ideas?

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

@LukeTowers most likely caused by the changes in priority made in this PR

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

I tested above suspicion and looks like this is not the cause. Still investigating.

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

I reverted back to winter 1.2.1 and the bavior is till the same, so this has either never worked properly or it's been broken for a long time.

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

@goldmont see if #1069 resolves the issue for you, it does in my testing.

@mjauvin mjauvin self-assigned this Feb 27, 2024
@mjauvin mjauvin added this to the 1.2.5 milestone Feb 27, 2024
@mjauvin mjauvin added Type: Bug and removed Type: Unconfirmed Bug needs review Issues/PRs that require a review from a maintainer labels Feb 27, 2024
@goldmont
Copy link
Author

@goldmont see if #1069 resolves the issue for you, it does in my testing.

Hi @mjauvin,

thank you for helping me. On my side, the value returned by the mutator is now somehow ignored.

null value is persisted in fact. Let me know if I should test something else

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

@goldmont can you show your mutator code ?

@goldmont
Copy link
Author

@goldmont can you show your mutator code ?

It's like the one in the Mutators documentation:

public function setFirstNameAttribute($value)
{
	$this->attributes['first_name'] = Crypt::encrypt($value ?? '');
}

I read the code you wrote. I think I have to return the value instead of assigning it.

@goldmont
Copy link
Author

By the way I can confirm that the following code works to me too:

public function setFirstNameAttribute($value)
{
	return Crypt::encrypt($value ?? '');
}

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

Brain cramp alert!

I swapped mutator with accessor in my mind... ignore everything I said and did so far... 🙄

@mjauvin
Copy link
Member

mjauvin commented Feb 27, 2024

@goldmont Ok, try PR #1071

@goldmont
Copy link
Author

@goldmont Ok, try PR #1071

I can confirm that it works properly now. Thanks!

@bennothommo bennothommo reopened this Feb 28, 2024
@bennothommo
Copy link
Member

This will be closed automatically when the PR is merged.

@LukeTowers LukeTowers added this to the 1.2.6 milestone Mar 12, 2024
@LukeTowers LukeTowers modified the milestones: 1.2.6, 1.2.7 Apr 25, 2024
@LukeTowers LukeTowers modified the milestones: 1.2.7, 1.2.8 Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants