#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.
Comment | File | Size | Author |
---|---|---|---|
#12 | 1607038-12.redirect.migrate-destination-entity.patch | 8.69 KB | joachim |
| |||
#8 | redirect-migrateDestinationEntity-1607038-7.patch | 9.61 KB | heddn |
#8 | interdiff.txt | 11.05 KB | heddn |
#5 | redirect-migrate-1607038-5.patch | 8.19 KB | hussainweb |
Comments
Comment #1
marvil07 CreditAttribution: marvil07 commentedComment #2
marvil07 CreditAttribution: marvil07 commentedMinor fixes: add inc to info file, change pk schema to avoid serial.
Comment #3
Pedro Lozano CreditAttribution: Pedro Lozano commentedThis 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.
Comment #4
benjifisherI 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.Comment #5
hussainwebI 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.
Comment #6
mallezieI 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.
Comment #7
benjifisherI 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.
Comment #8
heddnNot 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.
Comment #9
heddnre: #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.
Comment #10
benjifisherHow 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.Comment #11
heddnre: #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.
Comment #12
joachim CreditAttribution: joachim commentedThe 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.
Comment #15
joachim CreditAttribution: joachim commentedThere 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.
Comment #16
benjifisherNow 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 doAlternatively, 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.