Reuse of Field names results in stale data properties

[khalwat]

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?

[khalwat]

As discussed here:

https://github.com/nystudio107/craft-seomatic/issues/88

and here:

https://github.com/nystudio107/craft-seomatic/issues/87

[narration-sd]

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 notifications@github.com 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 https://github.com/craftcms/cms/issues/2800, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPIiS0VbIRu63QQ7OZxJ1aDhcwKavZcks5tr8M3gaJpZM4TinNb .

[khalwat]

Maybe if the Type of a field is changed, the data should be set to null? The only issue there is there are valid reasons to want to keep the text, like changing between Rich Text editors or such.

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 null if the change is made. I think this is preferable to throwing an exception and making the AdminCP inaccessible due to properties not existing.

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:

    /**
     * Remove any properties that don't exist in the model
     *
     * @param string $class
     * @param array  $config
     */
    protected static function cleanProperties(string $class, array &$config)
    {
        foreach ($config as $propName => $propValue) {
            if (!property_exists($class, $propName)) {
                unset($config[$propName]);
            }
        }
    }

...but it feels like something Craft should be doing in some way.

[carlcs]

Related issue: https://github.com/craftcms/cms/issues/1772

[michaelrog]

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 null if the change is made. I think this is preferable to throwing an exception and making the AdminCP inaccessible due to properties not existing.

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."

[khalwat]

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 richtext as the Field data index, allowing you to switch between anything that uses that.

It'd be somewhat messy though, trying to coordinate compatibility without some kind of a more formal interface.

[brandonkelly]

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."

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 ⚠️ emoji in the Field Type menu.

In this case, I’m guessing both of these field types say that they store data in text columns, so as far as Craft is concerned, the data seems like it might be compatible.

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 serializeValue(). Maybe in addition to that, you should just add an additional key:

"class": "nystudio107\\seomatic\\models\\MetaBundle"

And when decoding the value in normalizeValue(), check for that key before applying the values to the new MetaBundle instance.

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 class key to any existing field values in the content table.

[narration-sd]

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 😄

[khalwat]

@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:

            if (!isset($config['sourceBundleType'])
                || $config['sourceBundleType'] !== MetaBundles::FIELD_META_BUNDLE) {
                $config = [];
            }

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.