#1116408: Support migrate module: Destination handler class is about adding a migration destination handler for redirect.

This issue is about adding one destination class, so we can create migration classes that only import redirects. I have a use case where I am not importing other entities at the same time, so this is the only clean way I could think to do it.

Patch following, naturally it started with the code on the other issue, but it's a different migration destination class.

PS: I though it will be clear to have it in a separate issue, but I could merge this into the other issue if one of the maintainers think it's better.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marvil07’s picture

Status: Active » Needs review
FileSize
7.62 KB
marvil07’s picture

Minor fixes: add inc to info file, change pk schema to avoid serial.

Pedro Lozano’s picture

This worked pretty well for me.

Note that Migrate destination classes usually have the name starting with MigrateDestination, so instead of naming this class MigrateRedirectDestination you should name it MigrateDestinationRedirect.

benjifisher’s picture

I agree with #3.

I thought that redirect_migrate_api() should declare the destination class, but it looks like I was wrong.

Redirects are entities. (At least, the Redirect module implements hook_entity_info().) So why not extend MigrateDestinationEntity instead of MigrateDestination?

Assuming that both this issue and #1116408: Support migrate module: Destination handler class are adopted, there will be duplicated code: both define redirectValidate() methods. What is the best way to handle this? My inclination is to make it public here and then call this version in the destination-handler class. Currently it is protected there and private (why?) here.

hussainweb’s picture

I have made some changes as per comments in #3 and #4. I have renamed the class, extended it from MigrateDestinationEntity and made redirectValidate method protected instead of private.

mallezie’s picture

I tested this on a site i'm migrating, and this works as advertised. Code also looks good.
Not sure if its possible to add a destination for is_new, which is used to generate the hashes.
Otherwise it looks good.

benjifisher’s picture

I marked #2238039: Support migrate module: MigrateDestinationEntity class as a duplicate. Based on the comment there, the proposed patch might have some improvements over the one here. Sorry, I do not have time to review it myself.

heddn’s picture

Not finding this issue, I recently went down the same path as this in #2238039: Support migrate module: MigrateDestinationEntity class. The attached patch has some of the goodness from my efforts. Some key things to point out from it. It supports parsing out redirects that might include query parameters and targets (#) that were omitted from this. Other things are really just clean-up. Since this extends from MigrateDestinationEntity, there's no need for implementations of prepare, complete, prepareRollback and completeRollback.

heddn’s picture

re: #1607038-4: Support migrate module: Individual destination class
We can't reuse the validate function between the two implementations until PHP 5.4 and traits. OO inheritance won't allow.

benjifisher’s picture

How does parsing the query string and fragment interact with the default setting for the redirect module, Retain query string through redirect.? If you describe what the difference between the patches in #5 and #8 is supposed to be, then I will run a test.

heddn’s picture

re: #1607038-10: Support migrate module: Individual destination class
From all I can tell from working with redirect, query string and targets aren't stored in the redirect or source column in the DB, but rather in the options row.

So, I think this works the same as if someone manually created a redirect. If a destination query string is set, it would take precedence over the source query string. All the migrate does is parse the query string into the source_options/destination_options. This saves someone the work of creating their own processRow() to properly split the values out of the redirect / source and place them into the options.

joachim’s picture

The patches here and at #1116408: Support migrate module: Destination handler class are going to crash, because they both add the same file and the same hook.

#1116408: Support migrate module: Destination handler class is RTBC, so we should assume that one will get in first. Therefore, here's a reroll of the patch at #8, on top of the RTBC patch at #1116408: Support migrate module: Destination handler class.

Status: Needs review » Needs work

The last submitted patch, 12: 1607038-12.redirect.migrate-destination-entity.patch, failed testing.

joachim’s picture

There is no point setting this patch to be tested again until #1116408: Support migrate module: Destination handler class is fixed! I've explained in my previous comment why it will fail tests at the moment.

benjifisher’s picture

Now that #1116408: Support migrate module: Destination handler class has been fixed, we can return to this issue. Luckily, @joachim already provided a patch that we can use.

I still do not like the duplicated code for the redirectValidate() method in the two classes. The code is nearly identical, but there are some differences in comments (including the doc blocks) and line breaks. That makes this duplicated code that already has some differences.

I already pointed this out in #4. In #9, @heddn suggested avoiding the duplicated code by using a trait, which would require PHP 5.4. Are we still supporting older versions of PHP? Either way, there are other ways to reuse the code. For example, in the MigrateDestinationRedirect class, we could declare the method public static and then in MigrateRedirectEntityHandler we could simply do

  protected function redirectValidate($redirect) {
    return MigrateDestinationRedirect::redirectValidate($redirect);
  }

Alternatively, we could declare a global function redirect_validate_for_migration() and reference that in both classes.

In addition to removing the duplicated code, please follow Drupal coding standards. I think the version in MigrateRedirectEntityHandler comes closer that the version in MigrateDestinationRedirect, but the doc block there needs an @param annotation.