dgrigg / craft-migration-assistant

Migration Manager for Craft CMS
Other
27 stars 10 forks source link

Migrations with Neo Field Result in foreach() Error #40

Open ghost opened 3 years ago

ghost commented 3 years ago

We're trying to use Migration Assistant to track block changes across some Neo fields we have. Unfortunately, any Migration Assistant migration trying to do this fails with the same error.

Exception: Invalid argument supplied for foreach() (<project-location>/vendor/dgrigg/craft-migration-assistant/src/services/Fields.php:600)

Here's the migration in question (this is just a "I wonder what will happen" example - the field setup is just

<?php
namespace craft\contentmigrations;

use craft\db\Migration;
use dgrigg\migrationassistant\MigrationAssistant;

/**
 * Generated migration
 */
class m211117_161407_migration_new_dummy_block extends Migration
{
    /**
    Migration manifest:

    FIELD
    - newNeoContent
    */

    private $json = <<<'JSON'
{"settings":{"dependencies":[],"elements":{"fields":[{"group":"Common","name":"New Neo Content","handle":"newNeoContent","instructions":"","translationMethod":"site","translationKeyFormat":null,"required":null,"searchable":true,"type":"benf\\neo\\Field","typesettings":{"minBlocks":"","maxBlocks":"","maxTopBlocks":"","maxLevels":"","wasModified":false,"propagationMethod":"all","blockTypes":{"new1":{"name":"Cool Block","handle":"coolBlock","maxBlocks":"0","maxSiblingBlocks":"0","maxChildBlocks":"0","childBlocks":null,"topLevel":"1","sortOrder":"1","fieldLayouts":{"tabs":[{"name":"Settings","sortOrder":1,"elements":[{"type":"craft\\fieldlayoutelements\\CustomField","label":null,"instructions":null,"tip":null,"warning":null,"required":false,"width":100,"fieldHandle":"description"}]}]}}}}}]}}}
JSON;

    /**
     * Any migration code in here is wrapped inside of a transaction.
     * Returning false will rollback the migration
     *
     * @return bool
     */
    public function safeUp()
    {
        return MigrationAssistant::getInstance()->migrations->import($this->json);
    }

    public function safeDown()
    {
        echo "m211117_161407_migration_new_dummy_block cannot be reverted.\n";
        return false;
    }
}

Now when I took a look at Fields.php:600 I noticed right away that the index fieldLayout doesn't exist in that migration. Instead the migration uses fieldLayouts . So I decided to change the migration to use fieldLayout and well, that got me to another error

Exception: Undefined index: handle (<project-location>/vendor/dgrigg/craft-migration-assistant/src/services/Fields.php:73)

"Could this be trying to use fieldHandle?" I said to myself. So changed fieldHandle to handle in the migration and got the same error, so now I'm stuck.

(Or maybe I'm just completely off and none of this has nothing to do with the actual error 😅 )

We're running:

Any help (even maybe a different way to generate the migration - we're just point-n-clicking on the selected Neo field then generating the migration) is appreciated.

e: After posting I realized that the Neo version was a few behind, so I updated that to latest (2.11.18) but still no luck

dgrigg commented 3 years ago

@lwilkowskeBC the first thing to check/confirm is that you have any fields that Neo is using already migrated to the destination site. Since Neo uses existing fields those fields need to be on the server before the migration for the Neo field runs, bit of a chicken and egg situation.

ghost commented 3 years ago

Yes, all of the fields included in these Neo fields/blocks already exist before trying to create a migration. I've even had a migration with no changes (just selecting the field from the list, giving the migration a name, and creating it) fail in a similar way.

dgrigg commented 3 years ago

@lwilkowskeBC issue fixed. Update to 3.2.7 and create the Neo migration and then run again.

ghost commented 3 years ago

@dgrigg Thanks for the quick turn around on this, however, there seems to be another bug now.

Exception: Trying to access array offset on value of type null (<project-location>/vendor/spicyweb/craft-neo/src/services/BlockTypes.php:337)

I realize that this in Neo, but I thought maybe I'd reach out here to see if you were familiar enough with this to be able to help out any.

Thanks.

dgrigg commented 3 years ago

@lwilkowskeBC can you provide additional details from the log regarding the error. I have tested with the latest Neo field and the migrations were working correctly.

ghost commented 2 years ago

@dgrigg

Sorry for delay on this. We were having issues with getting consistent errors for the issues we were running into.

It seems that we're able to run Migrations with a Neo field now (👍 ) however, there are some caveats:

The fields inside of the Neo field (and therefore Neo field's Neo Blocks) need to be standard fields that come with Craft.

If we try to make a migration to add in some custom fields we have created, we start getting failing migrations (not too surprising really)

We noticed there's quite a bit in the docs about setting up Import and Export events for non-standard fields (https://github.com/dgrigg/craft-migration-assistant#field-type-support), but could not having those methods set for a field cause migrations to error out, or would not having those set just cause incomplete migrations? We were wanting to see if you knew the impact before we started hacking around in out fields.

dgrigg commented 2 years ago

@lwilkowskeBC the fields inside the Neo block can be custom but you do need to make sure that you have used the import/export events to properly handle them. If a migration was attempted for a field that had no support it could cause issues as the field would not get properly created and then when Neo goes looking for the field during it's migration it can find the field and thus results in an error.

tekstrand commented 2 years ago

@dgrigg are there other examples you could provide for those custom events? I am having a hard time following the docs.

dgrigg commented 2 years ago

@tekstrand those are the only examples I have. You would use those events inside the init function in either a plugin or custom module to modify the migration as needed.

tekstrand commented 2 years ago

@dgrigg does every attribute/property of a custom field need to be set manually? As far as we can tell we aren't doing anything special or unique with our custom fields. The custom fields themselves migrate just fine, but seem to cause issues when used inside of a neo field. Here's some json from the migration for the neo field for one of our custom fields if that helps give any context {"type":"craft\\fieldlayoutelements\\CustomField","label":null,"instructions":null,"tip":null,"warning":null,"required":false,"width":100,"fieldHandle":"citationLink"}

dgrigg commented 2 years ago

@tekstrand anything not in the normal set of attributes for a Craft field would need to get handled via the event listener. You need to write those attribute/values out when the migration is created and then read/parse back in when the migration gets run. In your example I can see all the normal Craft field properties but nothing that seems specific to your custom field.

tekstrand commented 2 years ago

I'm sorry for my trouble understanding here, I just feel like I must be missing some key piece of info. This custom field can be migrated by itself, why does it need special eventing when used inside of a Neo field?

dgrigg commented 2 years ago

@tekstrand sorry, my misunderstanding. For use within Neo you should not need any extra handling for doing a non-content migration, if the field was migrated and shows up on the destination side then any migration of a Neo field would be able to find the custom field used inside of Neo and make the appropriate associations. You would only need the event handling portion for doing the migration of the custom field, before Neo gets involved, to make sure all the settings etc specific to that field are setup correctly.

tekstrand commented 2 years ago

All good, thanks for the clarification! Do you have any other recommendations for how we can determine why we're getting this error when migrating the neo field? We've tried to chase down logs, but just keep winding up in that same spot. Exception: Trying to access array offset on value of type null (<project-location>/vendor/spicyweb/craft-neo/src/services/BlockTypes.php:337)

dgrigg commented 2 years ago

In the log files what lines precede that error to show where it is originating?

tekstrand commented 2 years ago

Nothing before.

> ./craft migrate --track=content --interactive=0
Checking for pending content migrations ...
Total 1 new migration to be applied:
    m220127_130927_migration_allfields

*** applying m220127_130927_migration_allfields
Exception: Trying to access array offset on value of type null (<project-location>vendor/spicyweb/craft-neo/src/services/BlockTypes.php:337)
#0 <project-location>vendor/spicyweb/craft-neo/src/services/BlockTypes.php(337): yii\base\ErrorHandler->handleError(8, 'Trying to acces...', '/Users/thomas.e...', 337, Array)
#1 <project-location>vendor/craftcms/cms/src/services/ProjectConfig.php(1189): benf\neo\services\BlockTypes->handleChangedBlockType(Object(craft\events\ConfigEvent))
#2 [internal function]: craft\services\ProjectConfig->handleChangeEvent(Object(craft\events\ConfigEvent))
#3 <project-location>vendor/yiisoft/yii2/base/Component.php(633): call_user_func(Array, Object(craft\events\ConfigEvent))
#4 <project-location>vendor/craftcms/cms/src/services/ProjectConfig.php(752): yii\base\Component->trigger('updateItem', Object(craft\events\ConfigEvent))
#5 <project-location>vendor/craftcms/cms/src/services/ProjectConfig.php(521): craft\services\ProjectConfig->_processConfigChangesInternal('neoBlockTypes.9...', true, NULL)
dgrigg commented 2 years ago

@tekstrand It looks like you might have project config enabled which could be causing conflicts. Migration Assistant was built before project config was an option. The two don't play together very well.

tekstrand commented 2 years ago

~We've got 'useProjectConfigFile' => false, in our config file and no config/project directory present.~

We have the writing of the project config file disabled with the following:

'projectConfig' => function() {
    // Disable writing proj config yaml files
    $config = craft\helpers\App::projectConfigConfig();
    $config['writeYamlAutomatically'] = false;

    return Craft::createObject($config);
},

I read through the docs here for this plugin and the Craft docs but didn't find anything additional for disabling config. Is there something else we need to do?

dgrigg commented 2 years ago

@tekstrand that could be the issue. If Neo is attempting to writing to the project config and something in that function you have is causing an issue, which I'm guessing could be the cause since the error codes point to the config being updated. Why not just use 'false'?

In the error log do you see anything reference the 'migration-assistant/services/XXXX'?

tekstrand commented 2 years ago

Why not just use 'false'?

I don't follow, just use false where? As far as I can tell from docs setting writeYamlAutomatically is as close as you can get to disabling the project config

dgrigg commented 2 years ago

Sorry, I was referencing the old way to disabling it. Looking at this link https://craftcms.com/docs/3.x/project-config.html#manual-yaml-file-generation it looks like you have the correct setup. I'm assuming that's in your config/app.php file.

Do you see any reference in your logs to migration-assistant/services/XXXX ? Also, are Craft and Neo up to date? Based on that error message it seems like Neo is failing to find a block type. What happens if you try to edit the Neo field in the UI and save it?

tekstrand commented 2 years ago

All good, my head is spinning trying to keep track of all that. Yes, that is what we have in config/app.php

No reference to migration-assistant/services in the logs. Neo is up to date, Craft is 2 small versions behind (we're on 3.7.28).

Saving the neo field via UI works fine, can make changes, new blocks, delete blocks, all good.

github-ab-martinez commented 2 years ago

@dgrigg - we discovered that the root cause of this issue was that during the Migration creation process, the getNeoField method in Fields.php assigns each of the field's blockTypeGroups a unique key in the following pattern: uid0. This resulted in the following structure in the migration's JSON:

"groups": {
              "uid0": { "name": "Group 1", "sortOrder": "13" },
              "uid1": { "name": "Group 2", "sortOrder": "21" },
              "uid2": { "name": "Group 3", "sortOrder": "24" }
            },

Before being imported, those keys are assigned as the id for the respective blockTypeGroup. Example $event->element object passed to EVENT_BEFORE_IMPORT_ELEMENT:

$event:
  element:
    _blockTypeGroups:
      0:
        id: "uid0",
        name: "Group 1"

Later when the Neo BlockTypes.php service attempts to use these ids for a db query, it throws an invalid parameter type error (expected an int, but got string).

To resolve the issue, we've added a module to craft which listens for the EVENT_BEFORE_IMPORT_ELEMENT event so that we can manually update the ids to either the id which exists for that blockTypeGroup in the db or null if not found.

dgrigg commented 2 years ago

@github-ab-martinez would you be able to share the code you have for updating the group id's. Hoping to get this resolved for the next release.

tekstrand commented 2 years ago
Event::on(Fields::class, Fields::EVENT_BEFORE_IMPORT_ELEMENT, function(ImportEvent $event) {
    if ($event->element instanceof NeoField) {
        $element = $event->element;
        $fieldId = $element->id;
        $blockTypes = $element->getBlockTypes();
        $groups = $element->getGroups();

        if (count($blockTypes) > 0) {
            $blockTypes = $this->assignExistingIds($blockTypes, '{{%neoblocktypes}}', $fieldId);
            $element->setBlockTypes($blockTypes);
        }

        if (count($groups) > 0) {
            $groups = $this->assignExistingIds($groups, '{{%neoblocktypegroups}}', $fieldId);
            $element->setGroups($groups);
        }

        $event->element = $element;
    }
});

/**
 * Assigns Neo field attribute ids for a given attribute array.
 *
 * @param array $items The attribute items to assign ids for
 * @param string $table The name of the table to get existing ids from
 * @param int $fieldId The id of the field the attributes are associated with
 */
private function assignExistingIds(array $items, string $table, int $fieldId): array {
    foreach($items as $item) {
        $result = (new \craft\db\Query())
        ->select([
            'id'
        ])
        ->from([$table])
        ->where([
            'name' => $item->name,
            'sortOrder' => $item->sortOrder,
            'fieldId' => $fieldId
        ])
        ->one();

        $item->id = $result ? $result['id'] : null;
    }

    return $items;
}
dgrigg commented 2 years ago

Thanks @tekstrand . Planning to have an update ready soon.