Reuse of Field names results in stale data properties #2800
Comments
|
As discussed here: and here: |
|
Sounds interesting to me also, on the coming Live Vue.
There, I depend on the fully pathed field signatures. This had somewhat
convinced me that the planned removal of contexts from craft_fields
wouldn't be a problem, but now your issue isn't reckoned, away from the
source code on a phone, anyway.
Maybe there's more normalization to be done before all becomes predictable and
straightforward with fields?
…On Tue, Apr 24, 2018, 17:28 Andrew Welch ***@***.***> wrote:
I had an issue reported to me today where someone had an existing field
from another plugin named "SEO" -- they installed SEOmatic, and changed the
Field type from the old plugin to SEOmatic's SEO Settings field type, and
they received this error:
2018-04-24 16:45:27 [75.50.55.134][1][39d75da9258539b5d0389dce43b111c5][error][yii\base\UnknownPropertyException] yii\base\UnknownPropertyException: Setting unknown property: nystudio107\seomatic\models\MetaBundle::title in /home/web/vhosts/stoneridge/vendor/yiisoft/yii2/base/Component.php:209
Stack trace:
#0 /home/web/vhosts/stoneridge/vendor/yiisoft/yii2/BaseYii.php(546): yii\base\Component->__set('title', '')
#1 /home/web/vhosts/stoneridge/vendor/yiisoft/yii2/base/BaseObject.php(107): yii\BaseYii::configure(Object(nystudio107\seomatic\models\MetaBundle), Array)
#2 /home/web/vhosts/stoneridge/vendor/nystudio107/craft-seomatic/src/models/MetaBundle.php(133): yii\base\BaseObject->__construct(Array)
#3 /home/web/vhosts/stoneridge/vendor/nystudio107/craft-seomatic/src/fields/SeoSettings.php(180): nystudio107\seomatic\models\MetaBundle::create(Array)
#4 /home/web/vhosts/stoneridge/vendor/craftcms/cms/src/base/Element.php(1904): nystudio107\seomatic\fields\SeoSettings->normalizeValue('{"title":"","de...', Object(craft\elements\Entry))
#5 /home/web/vhosts/stoneridge/vendor/craftcms/cms/src/base/Element.php(761): craft\base\Element->normalizeFieldValue('seo')
The issue is that the old "SEO" field has properties like title and
description that my field does not, so Yii2 rightly throws an error when
it tries to set properties that don't exist.
Now, I can do something like:
foreach ($config as $propName => $propValue) {
if (!property_exists($class, $propName)) {
unset($config[$propName]);
}
}
To sanitize the incoming data into something that my Field is expecting,
by removing any non-existent properties... but this feels like a core
issue, because this type of thing will happen with *any* field name
re-use.
Thoughts?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2800>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPIiS0VbIRu63QQ7OZxJ1aDhcwKavZcks5tr8M3gaJpZM4TinNb>
.
|
|
Maybe if the Type of a field is changed, the data should be set to Perhaps any field marked as “this may cause data loss” if the type is changed will actually cause data loss, by setting the content data to Also, if I delete a field, is the data from that field actually removed from the db? Or if I create a new field with the same handle, will it inherit that field's data blob? In any event, in the meantime, I'm guarding against this by sanitizing the configuration array that's passed in via: ...but it feels like something Craft should be doing in some way. |
|
Related issue: #1772 |
I strongly prefer not resorting to data destruction if we can possibly help it. (If I change a field type, then immediately change it back, I would be delighted to find that the data is still there and still works.) Whether by Craft's API or a plugin's individual self-protection, I'd rather solve this problem as close as possible (in time, and in code) to when it occurs, i.e. when a new plugin is trying to use data that was written by an old plugin. And I don't actually mind putting the responsibility for that on the plugins themselves, to self-protect. Counter-argument case: A plugin like Matrix/SuperTable that stores data elsewhere in its own schema. What's the balance between GC'ing obsolete elements if somebody changes a Matrix field to a Lightswitch, vs facilitating the magical sort of case where somebody changes a Matrix field to a SuperTable field and SuperTable wants to figure out how to actually make that work while preserving data....? What if there was a mechanism for a fieldtype to register some available migrations, to say "I know how to preserve data from Foo Fieldtype... if you're trying to change a field from Foo type to My type, run this migration." |
|
Playing off of what @michaelrog mentioned... a solution could be that the data stored in the content table is unique based on the class name or class handle of the Field (by default) but it could access field data from other field classes/handles, if it wants to be able to do some kind of mimicry/migration. This would solve the immediate problem of preserving data as a Field type is changed, and also ensuring that if a Field type is changed, it won't be getting stale data from some other Field type. The downside would be additional db storage, and migrations would be mostly manual, a Field would have to explicitly look for data from another Field type for mimicry/migration. Another possible alternative would be keeping the data unique per Field class/handle by default, but allowing it to override a method/property to specify that the Field data index should be. So anything that implemented a Rich Text editor could specify It'd be somewhat messy though, trying to coordinate compatibility without some kind of a more formal interface. |
Craft already has a very simple version of this in place, in that it will loosely compare the field types’ content column types. Field types with compatible content column types won’t get a In this case, I’m guessing both of these field types say that they store data in So yeah, a bit more information would be needed. I’m not sure it’s Craft’s place to store that extra info though, vs. just leaving it up to the field types. For example, presumably you are JSON-encoding your MetaBundle data in "class": "nystudio107\\seomatic\\models\\MetaBundle"And when decoding the value in if (is_string($value)) {
$value = Json::decodeIfJson($value);
if (is_array($value) && ArrayHelper::remove($value, 'class') === MetaBundle::class) {
$value = new MetaBundle($value);
} else {
$value = null;
}
}Then you’d just need to write a migration that adds that |
|
re: my comment above, I see I probably didn't read Andrew's original problem correctly on phone. This is not an issue where I would see it for Live Vue. Rog & Brandon's build of ideas on conversions, separately, sounds quite healthy 😄 |
|
@brandonkelly yeah thankfully my data structures do have an identifier built in that I can use to check against so that I can do something like: but given the change in how properties work with Yii2 models, I have the feeling that this may be an issue that other plugins will run into as well, especially after upgrading sites from Craft 2.x to Craft 3, where they may be switching to a different plugin to handle certain tasks. The main problem to me is that in this situation, people are going to have an exception thrown, and they'll be unable to access any Sections / Category Groups that have this field in their layout. |
I had an issue reported to me today where someone had an existing field from another plugin named "SEO" -- they installed SEOmatic, and changed the Field type from the old plugin to SEOmatic's SEO Settings field type, and they received this error:
The issue is that the old "SEO" field has properties like
titleanddescriptionthat my field does not, so Yii2 rightly throws an error when it tries to set properties that don't exist.Now, I can do something like:
To sanitize the incoming data into something that my Field is expecting, by removing any non-existent properties... but this feels like a core issue, because this type of thing will happen with any field name re-use.
Thoughts?
The text was updated successfully, but these errors were encountered: