createRepeaterMatrixField() doesn't call setMatrixItems() on first run and gives error if no items provided

[ivangretsky]

Good day @BernhardBaumrock and @gebeer !

Thank you both for Rock Migrations in general and being able to use Repeater Matrix field with it!

I think I found a bug in createRepeaterMatrixField() method logic and probably some connected methods. When I run migration for the first time the items are never created. To fix this I did the following changes:

But that is not all. If no items for Repeater Matrix field are provided in the options array then $items is null and it causes error. This should be dealt with in several places.

Sorry for no PR but I cant do it right now. Please look into it if you have time.

[BernhardBaumrock]

Hey @ivangretsky thx for the report. As I'm not using RepeaterMatrix I'd wait for @gebeer to confirm that.

[gebeer]

@ivangretsky thank you for pointing this out and taking your time to try and find a solution.

The core problem with createRepeaterMatrixField is that when the field is created in first run of migration with $this->createField, it returns ProcessWire\Field and not ProcessWire\RepeaterMatrixField as we would expect.

So if we use your proposed solution, it will cause Exception Method Field::getMatrixTypes does not exist or is not callable in this context. Because in setMatrixItems we call $types = $field->getMatrixTypes(); and that method is not available on ProcessWire\Field

On the next run of the migration, $field has the correct class ProcessWire\RepeaterMatrixField and $types = $field->getMatrixTypes(); works.

I was aware of that problem. That is why I chose to check with wireInstanceOf($field, 'RepeaterMatrixField') to avoid the Exception.

Drawback is, that migrations have to run twice to create the field with all items.

I have already spent quite some time debugging this in the past to get to the bottom of why createField returns ProcessWire\Field instead of ProcessWire\RepeaterMatrixField and have not been able to find the root cause for it. createField method creates a field with standard PW fields API, so nothing out of the ordinary here:

      $field = $this->wire(new Field());
      $field->type = $type;
      $field->name = $_name;
      $field->label = $_name; // set label (mandatory since ~3.0.172)
      $field->save();

If we can fix the root cause, the problems would be solved.

So if you have an idea, let me know. Would be much appreciated.

As for the error when not passing matrixItems array to createRepeaterMatrixField options, I will handle that and make a PR.

[gebeer]

opened PR https://github.com/baumrock/RockMigrations/pull/67 to fix error when no matrixItems are passed

[gebeer]

I think I finally found a solution. It involves a conditional in https://github.com/baumrock/RockMigrations/blob/10861345ca9cda99d9f84478aa2c8e06a49f901d/RockMigrations.module.php#L824

If I change that to

      if ($type instanceof FieldtypeRepeaterMatrix) {
        $field = $this->wire(new RepeaterMatrixField());
      } else {
        $field = $this->wire(new Field());
      }

the field is created with all matrix items in one go on the first run of the migration and returned as ProcessWire\RepeaterMatrixField.

@BernhardBaumrock do you have any objections or can I make a PR with this solution?

[gebeer]

I just found that every FieldType has a method getFieldClass: https://github.com/processwire/processwire/blob/3cc76cc886a49313b4bfb9a1a904bd88d11b7cb7/wire/core/Fieldtype.php#L948

If a custom FieldType implements that method and returns the field's class, we can use it. In case of FieldtypeRepeatermatrix:

    public function getFieldClass(array $a = array()) {
        require_once(__DIR__ . '/RepeaterMatrixField.php');
        return 'RepeaterMatrixField';
    }

So we could have a more generic solution in the createField method of RockMigrations:

      // get fieldClass as string if implemented for that FieldType
      /** @var string $fieldClass */
      $fieldClass = $type->getFieldClass();
      // add namespace to field class so we can find the class
      if(!empty($fieldClass)) $fieldClass = "ProcessWire\\$fieldClass";
      if(class_exists($fieldClass)) {
        // use the specific class to create new field if it exists
        $field = $this->wire(new $fieldClass());
      } else {
        $field = $this->wire(new Field());
      }

I would opt for this solution since it can also work for other custom Fieldtypes if they implement getFieldClass.

@BernhardBaumrock what do you think?

[ivangretsky]

Hey @gebeer !

I think that @BernhardBaumrock used the generic Field class consciously not to deal with specificities. I think my solution (see the 1st screenshot) is more in line with the rest of the code.

Maybe we should get the RepeaterMatrixField later in setMatrixItems() on our own? I didn't find an easy way to do it yet though.

[gebeer]

Hey @gebeer !

I think that @BernhardBaumrock used the generic Field class consciously not to deal with specificities. I think my solution (see the 1st screenshot) is more in line with the rest of the code.

Maybe we should get the RepeaterMatrixField later in setMatrixItems() on our own? I didn't find an easy way to do it yet though.

It is still using the generic Field class as a fallback if no specific class is found. So I actually see it as an improvement to be able to be more specific when appropriate.

Your solution causes the error I described because $field->type is always correct, even if the class does not correspond to it.

And first thing I have tried was to get the field again later in setMatrixItems as you suggest. But also couldnt find a way to get it with the correct class. I even tried to save it one more time after getting it again. Also didn't help.

[BernhardBaumrock]

Hey @gebeer thx for your work on this.

Wouldn't it be possible to just check on the $field->type property?

But I also tried this and thought first creating a new field and then getting it via PW API might return the correct class:

But it also returns "Field", so I tried this:

Which also returns "Field"

So maybe you should check for $field->type instanceof ... instead of $field instanceof ... ?

[gebeer]

Ok, I'll try explain again since it seems I did not manage to get my point across.

When using $field->type this always returns the correct field type. But the class of the $field is still ProcessWire/Field.

Now when setMatrixItems is called with a field that has class ProcessWire/Field then https://github.com/baumrock/RockMigrations/blob/10861345ca9cda99d9f84478aa2c8e06a49f901d/RockMigrations.module.php#L6288 errors out. Because of $field->getMatrixTypes(). Class ProcessWire/Field has no method getMatrixTypes().

My proposed solution makes sure that the field is created with an instance of the correct field Class (RepeaterMatrixField).

[BernhardBaumrock]

@gebeer let's have a quick meeting?

[gebeer]

@BernhardBaumrock have to go to the dentist. Back in 1.5 hours

[gebeer]

@ivangretsky should be fixed with https://github.com/baumrock/RockMigrations/pull/68