D7 Migrate source plugin

Attached is a patch that I am testing with right now. I'm having a few issues still, but I'm not sure if it's related to this class (which is very simple) or my migration itself. Uploading this here for review while I continue testing...

I'm having a few issues still, but I'm not sure if it's related to this class

This is the error I'm seeing when I try to migrate logs right now:

[error] Error: Call to a member function getNamePattern() on null in Drupal\log\Entity\Log->getTypeNamePattern()

I'm not yet sure WHY this is causing an issue, but if I comment out the code in Log->getTypeNamePattern(), everything works. So I think we need to focus on that code and understand why it's failing in the migration context.

Some more details on the above issue:

The LogStorage class has a doPostSave() method, which checks to see if the Log needs to have its name auto-generated on save. It looks like this:

  protected function doPostSave(EntityInterface $entity, $update) {

    ...

    // Calculate whether the entity needs a name pattern.
    $name_pattern = $entity->getTypeNamePattern();
    $entity_needs_name_pattern = $entity->get('name')->isEmpty() && !empty($name_pattern);

    ...

So that is where getTypeNamePattern() is being called. As each log item is being saved by the migration, it tries to check if it needs a name.

Migrated logs will (should) always have a name already (assuming they were saved properly in the D7 database), so the second line of that code should see that and prevent the name generation from happening.

However, the error is happening before that, in getTypeNamePattern()...

  public function getTypeNamePattern() {
    /** @var \Drupal\log\Entity\LogTypeInterface $type */
    $type = \Drupal::entityTypeManager()
      ->getStorage('log_type')
      ->load($this->bundle());
    $name_pattern = $type->getNamePattern();
    return $name_pattern ?? '';
  }

The error suggests that $type is not being loaded properly (it is null), so it's failing when it tries to call $type->getNamePattern().

I'm not sure why $type is failing to load in this context. I'll test to see if the same is happening outside of migrations...

In the meantime, a simple fix would be to check if $type is not empty before calling its method.

I wonder if $this->bundle() is not working for some reason during the migration. Perhaps this points to an issue with my migration?

Not sure about the error you're reporting, does the log type exists on the destination?

Yes it does exist. Perhaps I can show it to you live on a call - I have a dev environment all set up for testing the migrations.

For the patch, I think we can just rely on the migrate plus table one, right?

Oh I wasn't aware of that - I was curious if there was some sort of "general custom entity" support we could leverage instead of providing our own class.

But... would the Table class support the D7 field migrations as well? The Log class in my patch extends from FieldableEntity (which extends from DrupalSqlBase <- SqlBase). I assume that FieldableEntity will give us the support for the D7 log field migrations, but I haven't tested any yet. Table extends directly from SqlBase, so it seems like it would be for simpler cases (we will probably want this for the sensor data migration in farmOS!).

Attached is an updated patch which moves the getTypeNamePattern() call after the isEmpty() check, per your suggestion.

Note that I plan to merge these changes as two separate commits (already in a branch here: https://github.com/mstenta/log/compare/2.x...mstenta:migrate) - just including them both in the patch for easier review/testing in this issue.

FWIW, I reinstalled my local dev environment and was no longer able to replicate my issue, so I think it was due to a broken log type config after all. Regardless, I agree that this small logic tweak makes sense, so let's do it anyway.

As for tests, I agree that ideally we could have a migration test with this patch. In the interest of farmOS 2.x development, I'm leaning towards committing this without a test and focusing on a more complete farmOS 2.x migration test. Once that is done we can consider cross-porting a simplified version of it back to this repo if we want.

The class that this patch adds simply defines the D7 Log entity's base fields and primary key, and it is copied almost verbatim from the core taxonomy source class, so if something breaks upstream it will be caught by those tests. This is just me justifying not putting in the time to write the tests+fixtures for this patch right now. :-)

I committed the initial source plugin class so that it can be used in farmOS 2.x. I'll work on adding tests over there and cross-port them to this in a new issue.