Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
A major use case for redirects is supporting legacy URLs after migrating content.
Proposed resolution
We can provide a handler for the Migrate module. This way, a developer can add a mapping with a simple one-line addition to their migrate class:
$this->addFieldMapping('migrate_redirects', 'old_legacy_path');
Remaining tasks
Patch reviews
User interface changes
none
API changes
The patch implements hook_migrate_api(), and adds a migrate.inc file containing the handler. The autoloader will make sure that migrate.inc is only loaded when needed.
Comment | File | Size | Author |
---|---|---|---|
#128 | interdiff-125-128.txt | 2.41 KB | benjifisher |
#128 | support_migrate_module-1116408-128.patch | 5.83 KB | benjifisher |
|
Comments
Comment #1
mikeryanSupport for contrib modules would go into the Migrate Extras module.
Comment #2
Dave ReidI could add migration support to Redirect.module itself?
Comment #3
skitten CreditAttribution: skitten commentedIn the end I just wrote a small module to catch urls of the old format, look up the id in the migration db and redirect to the new node. But I'm not sure what the best way to write a generic migration/redirect mechanism would be.
Comment #4
mikeryanYes, Dave, you certainly could:-).
Comment #5
mlncn CreditAttribution: mlncn commentedNot sure how this should manifest in either the Redirect or Migrate modules, if either, but here is the code i'm using in a custom migration module that extends Migrate of course and also makes use of Redirect.
In the include file for the class that extends the BaseMigration class, most of the action is in a class extending MigrateDestinationHandler, show second.
Comment #6
grendzy CreditAttribution: grendzy commentedTo use this handler, simply add a mapping to your migrate class like so:
Comment #7
grendzy CreditAttribution: grendzy commentedComment #8
Mark Theunissen CreditAttribution: Mark Theunissen commentedAwesome, this is exactly what we need. I'll be testing soon and will report back.
Comment #9
marcusx CreditAttribution: marcusx commentedRedirect is not saved. Because the entity_uri can not been determined due to wrong argument 'entity type'. Acctually this is not the entity type it is the entity_type_bundle_type or whatever it is called by the entity api. e.g. in a page node migration this is 'page' not 'node'. I have no idea if we can get the migrate destination type at this place to make this generic for nodes and users or whatever. For most cases 'node' would work fine.
Patch for the patched version enclosed.
Comment #10
grendzy CreditAttribution: grendzy commentedHm, that is a puzzle: the handler in #6 works, but only because $entity->uri was already set by the time entity_uri() was called. As a result the (incorrect) type parameter was ignored.
I'd like to avoid hard-coding "node", so that it could work for other entities. I'm not sure what's the best way to handle this. We could just refer to $entity->uri directly, but I don't know if that's guaranteed to be set.
Comment #11
marcusx CreditAttribution: marcusx commentedI totaly agree that hardcoding 'node' is a bad idea. I had no time to figure out how I can get this information from migrate yesterday and needed this working. I suggest this better solution.
Patch against the patched version. Sorry for that, I know there is a way to create a follow up patch, I have to read git documentation about how this works again.
Comment #12
marcusx CreditAttribution: marcusx commentedAehm maybe we should also rename the variable "$destination->$redirect_destination" to avoid confusion.
Comment #13
grendzy CreditAttribution: grendzy commentedmarcusx: oh, that's excellent. I'll work on a re-roll.
BTW - do you happen to know if $entity->uid and $entity->language are valid for other entity types? Or are those node properties?
Comment #14
marcusx CreditAttribution: marcusx commentedAdded validation before the redirect is saved because I got some database errors due to duplicate entries if the redirect was already existing. There is also a proper error message in the migration if this happens.
I created a complete reroll of #6 #12 and the additional changes for the validation.
This patch is against the latest dev from git.
Comment #15
grendzy CreditAttribution: grendzy commentedThanks for the re-roll. Can you please remove tabs and trailing whitespace?
Comment #16
marcusx CreditAttribution: marcusx commentedHere we go... sorry for the inconvenience
Comment #17
marcusx CreditAttribution: marcusx commentedAhhh damn it! I can't set the current Migration here.
If I use
I get an Fatal error.
What is quite clear because at this point there is no "current" Migration.
I will rework this a soon as I can, but in case s.o. else has time I just want to document it.
Comment #18
Fidelix CreditAttribution: Fidelix commentedThis would be a much appreciated addition.
Thank you all for the excellent work!
Comment #19
mikeryanSo close! Just needed to put off pulling stuff dependent on the active migration to complete() (note we now have a static message display function that will work anywhere without a Migration object).
Comment #20
mikeryanOops! Caching the entity_type breaks when running a sequence of disparate migrations, since there's only one handler instance ever, not one per entity type... New patch coming.
Comment #21
mikeryanComment #22
mikeryanSome tweaks, in particular saving messages as informationals in the migrate message table rather than spewing them out.
Comment #23
bforchhammer CreditAttribution: bforchhammer commentedPatch #22 seems to be working quite well. Thanks for the great work!
Comment #24
mikeryanBefore committing, I just want to make sure it's properly credited - I just added the final tweaks, grendzy and marcusx did most of the work.
Comment #25
aidanlis CreditAttribution: aidanlis commentedShould the file not be called redirect.migrate.inc for consistency with other projects?
Comment #26
aidanlis CreditAttribution: aidanlis commentedPatch #22 is also missing the hook_migrate_api(). I've added that in and renamed the file "redirect.migrate.inc" to be consistant with every other module implementing migrate support.
I haven't contributed anything new to this patch, credit goes to marcusx and grendzy and others.
Comment #27
sirkitree CreditAttribution: sirkitree commentedSweet, so this is passing, but can I get a little hint on how to test it out?
I'm working on a migration currently, and one of my content types (let's say article) is migrating fine. I also have the:
I've applied the patch, and have it applied cleanly with
patch p1 < *.patch
.What do I add to my current node migration?
Edit:
I'm migrating from path_redirect
Comment #28
grendzy CreditAttribution: grendzy commentedsirkitree: addFieldMapping() is all. The first parameter is literally
'migrate_redirects'
, and the second parameter is the name of a field in your source data.Comment #29
sirkitree CreditAttribution: sirkitree commentedThis worked perfectly for me. Thank you, saved me so much time.
Comment #30
dillix CreditAttribution: dillix commented#26 doesn't work as expected, then enabled migrate extras and pathauto module. I use following code for migration:
But in redirect.migrate.inc in complete function I have entity:
My old url - ?go=obzor&oid=7 and my new url should be /article/1 but I got /node/1. How can I tell to MigrateRedirectEntityHandler pathauto generated alias?
Comment #31
dillix CreditAttribution: dillix commentedAs I wrote, patches #22, #26 needs work to integrate with pathauto. When I go to url /?go=obzor&oid=7 then it 301 redirect to /node/1 and then it 301 redirect /article/1. I think its because pathauto runs after node creation and migrate redirect runs before.
Comment #32
bforchhammer CreditAttribution: bforchhammer commentedPathauto generation does not have anything to do with this issue.
Besides, it should already work just fine if pathauto is configured correctly. In my case I was fine not adding anything to my migration classes, because the path is usually created automatically during node save...
This sounds like it is working, no? What is it that you're expecting?
Comment #33
dillix CreditAttribution: dillix commentedNo, it should set redirect from /?go=obzor&oid=7 to /article/1 in redirect module, but I have /?go=obzor&oid=7 to /node/1 and in path auto /node/1 to /article/1. And when I run migrate, I have a lot of notices here http://site.local/#overlay=admin/content/migrate/messages/NodeArticles like this:
Undefined index: path File /Users/macspro/Sites/site.local/sites/all/modules/migrate_extras/redirect.migrate.inc, line 73(file: /Users/macspro/Sites/site.local/sites/all/modules/migrate_extras/redirect.migrate.inc, line 73)
It happens because pathauto runs after we generate redirect with redirect.migrate.inc. As you can see in #30 (I have following data in complete function in redirect.migrate.inc):
Comment #34
mikeryan@dillix: I see no problem with the indirect 301 redirect, everything ends up as it should be. We don't know if and when pathauto will be creating an alias (and note that in migrations many people disable pathauto during migration and generate aliases in batch afterwards), so it's simplest and safest to just establish the redirect to the canonical address - it all works out in the end.
As for the notices, your paths don't have a 'path' component, just a query string - this updated patch should address it. Also note that I've moved hook_migrate_api() to the migrate.inc file, no need to have it in .module.
Comment #36
marvil07 CreditAttribution: marvil07 commentedI have just created another issue about migrate, which is not exactly the same of this at #1607038: Support migrate module: Individual destination class
As I mention there, I could merge the code here if maintainers thinks it's better.
Comment #37
dakhota CreditAttribution: dakhota commented#34: migrate_redirect-1116408-34.patch queued for re-testing.
Comment #39
Matt V. CreditAttribution: Matt V. commentedHere's an updated patch that is applying cleanly for me.
Comment #40
rbayliss CreditAttribution: rbayliss commentedThis works nicely, but as grendzy pointed out above, it requires you to map UID and language, which don't exist for all entities (taxonomy terms, for example). This patch adds isset() checks on these two properties.
Also, more generally, does it even make sense that the redirect would get the UID and language of the entity?
Comment #42
CaptainSully CreditAttribution: CaptainSully commentedI've tried the patches in #26 and #39, and in both cases I'm running into a problem where in the 'from' field of the redirect it parses the url such that there is a '/' in front. This is a problem since the beginning of the path is forced to be "http://localhost/drupal1/".
So if "http://localhost/drupal1/scholars/index" goes through the migration, I end up with
"http://localhost/drupal1//scholars/index" instead, and the // results in a page not found.
screenshot attached
Comment #43
elBradford CreditAttribution: elBradford commented39 worked for me - helped create redirects for over 2000 nodes. Thanks!
Comment #44
eojthebraveThis worked great for me. Using something like the following called from prepareRow() to lookup path_redirect module redirects.
Comment #45
mikeryanFor Migrate 2.5 and greater, handlers should be explicitly registered in hook_migrate_api().
Comment #46
brunodboTested the patch in #45, works great. Just one question on the syntax: how would I write a field mapping rule to import multiple legacy redirects for 1 node?
Comment #47
brianfisher CreditAttribution: brianfisher commented#45 worked perfectly for me, thanks
Comment #48
mikeryan@brunodbo - not a matter of syntax, what you need to do is populate the source field in your mapping with an array of the redirects you want (presumably in prepareRow).
Comment #49
brockfanning CreditAttribution: brockfanning commented#45 worked great for me, as well. I love that it implements the rollback too. Thanks everyone for putting this together!
Comment #50
Simon Georges CreditAttribution: Simon Georges commentedAt this point, I think we can consider it reviewed and tested ;-)
Comment #51
brockfanning CreditAttribution: brockfanning commentedI just noticed a minor issue when adding redirects for users (users which had in turn been created through another migration). The entity->language was apparently set as an empty string, so it was passing the "isset" conditional. Apparently redirects don't work if the language column is a blank string. It's an easy fix, just have to also test for blank strings. I will try to post my fix as a patch, just a temporary heads up until I get the time.
Comment #52
brockfanning CreditAttribution: brockfanning commentedVery sorry for the delay, here is that patch I promised above.
Comment #53
jamsilver CreditAttribution: jamsilver commentedThe patch in #52 did not work for me (using Migrate 2.5 - the latest stable version). Wrapping a line of the hook_migrate_api() implementation in an array did the trick. Turning:
Into:
Patch rerolled and attached.
Comment #54
rvilarPatch in #53 works for me.
Comment #55
DanChadwick CreditAttribution: DanChadwick commentedPatch 53 worked for me. Thank you very much.
Comment #56
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedpatch 53 works, thx. Tested on nodes and files.
Comment #57
JurriaanRoelofs CreditAttribution: JurriaanRoelofs commentedComment #58
brant.darilek CreditAttribution: brant.darilek commented#53: redirect-migration-support-1116408-53.patch queued for re-testing.
Comment #59
mikeker CreditAttribution: mikeker commentedThe patch in #53 includes a leading slash for absolute URLs. For example, "http://example.com/foo" ends up saving a redirect of "/foo" instead of "foo". (Note: the UI prevents leading slashes from being added to redirects).
The attached patch is #53 plus an ltrim() on the parsed path. Interdiff attached, or @line 81:
becomes
Comment #60
alforddm CreditAttribution: alforddm commented#53 worked for me on nodes. Really appreciate the work on this.
Comment #61
jeffschuler#59 is working great for me.
Thanks!!
Very minor docs changes:
"summary lines must start with a third person singular present tense verb" ("Validates") and should have an extra newline.
"simmilar" should be "similar"
Comment #62
kwseldman CreditAttribution: kwseldman commentedThese patches don't appear to be working for me. I've tried both #53 and #59. I'm using Redirect 7.x-1.0-rc1. After migrating my Wordpress site to Drupal and having pathauto generate new aliases, when I point my browser to any of my old paths on my new Drupal site, it takes me to the front page of my blog rather than giving me a 301 HTTP code.
I noticed that when I migrated my content and chose the option "Have pathauto generate new aliases," I got this message: "You have the Redirect module enabled. To be able to generate redirects for your imported WordPress content, you need to patch Redirect." This was displaying after I'd patched Redirect. Does this message go away after patching, or no? I'm not getting an option to generate redirects. Does this indicate that the patch hasn't worked?
My husband (oseldman) has also suggested that due to the format of my Wordpress paths (for example: http://littleenglishgirl.com/?p=1171) this method of redirecting may not work at all - does anyone have similar paths, and if so, did this method work? If not, is there something else I can try? Thanks.
Comment #63
kwseldman CreditAttribution: kwseldman commentedI realized I was patching 7.x-1.0-rc1 rather than the dev version. I patched the dev version with #59 and the option to generate redirects appeared.
Comment #64
kwseldman CreditAttribution: kwseldman commentedEven with the patch seemingly working, redirects still aren't being generated. Any thoughts as to why that might be happening? Sorry for bombarding the issue queue. Thanks.
Comment #65
rvilarThis is the patch in #59 with fixes from #61 applied and with coder review done. It works for me.
Comment #66
Bhanuji CreditAttribution: Bhanuji commentedPDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'xJMoIg4sq_0tnmtrrKRhOC6-2F1yga28A75jvr3tXCY' for key 'hash': INSERT INTO {redirect} (hash, type, source, source_options, redirect, redirect_options, language, status_code, count, access) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9); Array ( [:db_insert_placeholder_0] => xJMoIg4sq_0tnmtrrKRhOC6-2F1yga28A75jvr3tXCY [:db_insert_placeholder_1] => redirect [:db_insert_placeholder_2] => magazines/quiltsandmore/qam-table-contents-old_1.html [:db_insert_placeholder_3] => a:0:{} [:db_insert_placeholder_4] => node/32553 [:db_insert_placeholder_5] => a:0:{} [:db_insert_placeholder_6] => und [:db_insert_placeholder_7] => 0 [:db_insert_placeholder_8] => 0 [:db_insert_placeholder_9] => 0 ) in drupal_write_record() (line 7158 of /app/web/divinecaroline.com/includes/common.inc).
I guess, there might be multiple redirects in redirect table..
In migration, how to solve it.
As we are using Html (Static files) to Drupal migration.
Comment #67
kostajh CreditAttribution: kostajh commentedThis version of the patch in #65 fixes some Coder complaints, and also allows for multiple redirects. Checking $entity for "migrate_redirects", at least in my environment, would only ever return one redirect to evaluate. Checking
$row->migrate_redirects
however returned an array of redirects defined in my mapping, which correctly saved multiple redirects for a given entity.Comment #68
kostajh CreditAttribution: kostajh commentedSorry, use this one instead. This works in my environment for cases with one or multiple redirects.
Comment #69
kostajh CreditAttribution: kostajh commentedFound another issue. Here's a new version of the patch.
Comment #69.0
kostajh CreditAttribution: kostajh commentedadd summary
Comment #70
mariacha1 CreditAttribution: mariacha1 commentedI ran this through a handful of redirect migrations using the structure:
What I tested:
Redirects on Users
Redirects on Nodes
Redirects on Terms
Redirects using a separator (comma)
Redirects that produced the error "The source path is already being redirected."
Code looks solid as well. I think this is good to commit!
Comment #71
JeremyFrench CreditAttribution: JeremyFrench commentedI have just used this patch for the second time now and it works fine.
Also think it is good to commit.
Comment #72
benjifisherI am adding a related issue: #1607038: Support migrate module: Individual destination class.
The patch works as expected so far, updating migrated taxonomy terms.
Comment #73
kostajh CreditAttribution: kostajh commentedAny chance this could get committed?
Comment #74
joelstein CreditAttribution: joelstein commented+1 from me. I'm using this patch for Migrate support, and I've had no problems. Works as advertised!
Comment #75
roberttstephens CreditAttribution: roberttstephens commentedI've used this several times as well without issues.
Comment #76
heddnRelating #2238039: Support migrate module: MigrateDestinationEntity class here.
Comment #77
heddnShouldn't there be a redirect_parse_url on the destination path? In case it has query string parameters?
Comment #78
heddnSee #2238039-1: Support migrate module: MigrateDestinationEntity class for some potential improvements in reference to #1116408-77: Support migrate module: Destination handler class
Comment #79
benjifisherI am adding #1607038: Support migrate module: Individual destination class as a related issue. I tried to do that in #72, but I guess it did not work.
Comment #80
apanag CreditAttribution: apanag commentedPatch worked great. Thank you!
Comment #81
ckngPath #69 works great for import. However, any suggestion on handling rollback?
Comment #82
mikeryanRestoring the rtbc status.
@heddn: The destination of the field handler redirect is a newly-created entity - i.e., the destination path is always going to be something like node/123, user/456, etc., there will never be query string parameters.
Many people have been successfully using the patches from this issue for years - please, let's get it committed and address any narrow use cases missed in follow-up issues.
Comment #83
heddnI'm fine with follow-ups to address outstanding issues.
re #82: In my use case, the redirects were from a CSV dump from an SAP-based website that was getting migrated to Drupal and pointed to off-site content. The anchor tags and query parameters were important to parse and migrate.
Comment #84
cthos CreditAttribution: cthos commented@heddn - Wouldn't that use case be better addressed in a migration using MigrateDestinationRedirect?
If they're not going to the node being created as part of a field handler, that doesn't seem like the proper use of ->addFieldMapping('migrate_redirects');?
Comment #85
DanChadwick CreditAttribution: DanChadwick commentedRe #82: +1.
Comment #86
rodrigoaguileraI'm creating nodes with a language set but I want to redirect for all languages
example:
I create node/23 in Spanish with the redirect /myoldurl. The redirect will be created in Spanish so the redirect will happen on someone going to /es/myoldurl not /myoldurl as I wanted.
The language of the redirect depends on the language of the entity when it shouldn't be like that.
Comment #87
rodrigoaguileraPatch rerolled to apply clean and support a language override with a new migrate field
migrate_redirects_override_language
#81
handles rollback already
Comment #88
rodrigoaguileraforgot the interdiff
Comment #89
heddnCan't this just use 'language'? And again in line 111/112?
Comment #90
rodrigoaguilera@heddn I don't get you.
What I this enables is the possibility for the redirects to be in a different language from the entity or other language things involved.
And maybe the type of the redirection should be something that can be overriden because is hardcoded
Comment #91
rodrigoaguileraComment #92
rodrigoaguileramistake on line 111 using row instead of entity
Comment #93
heddnIs migrate_redirects_override_language a field/property on the entity? Can you explain more about it? It seems like a custom field, rather than the language property that is commonly available to entities. If it custom, this should be removed from the general use patch. It can be re-added to a custom extension of MigrateRedirectEntityHandler for your use case.
Comment #94
rodrigoaguileraI think the language of the redirection should be handled in this issue other than setting the same language as the entity that has the redirection.
My patch is a suggestion about how to handle this case: I want my entity in one language and the redirection in another.
Comment #95
jeffamThanks for all the good work on adding support for redirects to migrations. Just yesterday I needed to create redirects during a migration and it was a ton of help to find this work.
Since it seems like folks are still hashing out some details, and I didn't want to patch my copy of redirect, I figured I'd share a solution that uses this code in one's custom migration module.
I essentially applied the patch in #92 to my custom migration module. I did it manually, by adding
files[] = redirect.migrate.inc
to the.info
file and by creating theredirect.migrate.inc
file with the most of the contents of the patch. Lastly, I updated myhook_migrate_api()
settings to include the following in the returned array:With that done, I can now use a mapping like this:
Just tested it and it works great. Thanks, all!
Comment #96
Darren Shelley CreditAttribution: Darren Shelley as a volunteer commentedIf we look at the problem and motivation behind the patch we see that the intention is for "supporting legacy URLs after migrating content".
The most simple manifestation of this is where an entity has one to many urls associated with it and we have a need to redirect those from their previous location to a common path such as entitytype/entityid (Path module + global redirect can handle getting them to the pretty and often dynamic version)
Dave Reid early on highlighted Redirect as being the best location for this work.
For over 3 years the wordpress_migrate has been openly supporting patches from this issue queue and now actively supports patch #69.
I feel that it's important for the usability of the wordpress_migrate module to remove it's reliance on patching this module to perform what is a relatively common and important task to any migration.
Having reviewed the patches I feel that the patch in #69 best fits this need.
The later work by rodrigoaguilera whilst good hits a specific user need that the majority of migrations won't encounter so in keeping with agile methodology I see no reason to delay providing a solution in favour of waiting for agreement on how that additional functionality might manifest.
I would suggest that this multilingual request/work be moved to a separate issue queue.
I have attached a patch that applies cleanly against the latest 7.x-1.x-dev and have restored RTBC status.
I have tested this patch in a number of migrations successfully.
Thank you to all the developers for their hard work thus far on this matter.
Comment #97
osopolar+1 for getting this in, and discuss additional stuff in follow up requests.
This works for me, although I am using the same method as jeffam describes above in #95 with a tiny difference: I renamed the class to avoid a possible redeclare class error (which may happens on updating redirect module after the patch got committed).
Comment #98
rodrigoaguileraOk, I agree that the language situation can be handled on other issue. I will open it and make the patch once this is committed so we don't make patches over patches
Comment #99
dillix CreditAttribution: dillix commented+1 for RTBC
Comment #100
jonathan_hunt CreditAttribution: jonathan_hunt commentedPatch in #95 works for me, thanks. FYI, I used
$this->addFieldMapping('migrate_redirects', 'path');
in my extension of DrupalNode6Migration (from migrate_d2d).Comment #101
hussainwebThe call to isset is redundant.
This can be hugely simplified.
I have fixed the above and cleaned up the code in other places as seen by the interdiff.
Comment #102
rodrigoaguileraThis is not needed, the file will be detected automatically http://cgit.drupalcode.org/migrate/tree/migrate_example/migrate_example....
Comment #103
heddnI agree with #102, however that can be fixed on commit, unless someone gets to re-rolling this before then. Moving back to RTBC. The changes in #101 are sane.
Comment #104
hussainweb@rodrigoaguilera, @heddn: Are you sure about that? I have had trouble with classes not found in the past, but they could just be with destinations, not handlers.
Comment #105
rodrigoaguilera100% sure, never had a problem making migrate modules using that naming.
Also no problem with handlers #2512562: Migrate support
Comment #106
ohthehugemanatee CreditAttribution: ohthehugemanatee at Forum One commentedI've used this on a couple of projects now. RTBC for sure.
Comment #107
Summit CreditAttribution: Summit commentedHi,
What do you need to use this redirect.migrate.inc (https://www.drupal.org/node/1116408#comment-10275065)
Is this script in a bigger script enough?
EDIT, I see the MigrateRedirectEntityHandler handler within my Migrate Handlers.
But with adding this $this->addFieldMapping lines within my weblinks.migrate.inc script I do not see my redirects migrated from drupal 6 to drupal 7.
I see one redirect added and that is:
MigrateRedirectEntity node/1215
And furthermore a lot of mistakes like: The source path is already being redirected.
...What do I do wrong please?
Greetings, Martijn
Comment #108
emilorol CreditAttribution: emilorol commentedI was able to make it work and I posted my answer here:
http://drupal.stackexchange.com/questions/179083/drupal-6-to-drupal-7-mi...
Thank you for putting this together.
Comment #109
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedJust moving this along by uploading a patch with the change suggested by #102 and #104. Another +1 on getting this into the module!
Comment #110
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedAs an aside, if you're an oddball like me and you want to preserve all the existing redirects in an existing Drupal 7 site when doing a Migrate D2D migration of a content type, the simplest way to use this patch in your migration is to do the following:
Inside
prepare()
:And then later in the migration class, define
retrieveOldRedirects
:I'm certain there's a better/more efficient way to do this... but I was in a hurry and wanted to just grab all the redirects for a given source node, and this works great for me!
Comment #111
geerlingguy CreditAttribution: geerlingguy as a volunteer commented(Setting back to RTBC as the patch in #109 was a one-liner to remove a file from the .info files array. No functional difference.)
Comment #112
patrick.thurmond@gmail.comSo I used this patch in my current project. However, I had to modify it because the project I am working on uses entity translations instead of node translations and I need to be able to apply the English and French versions of the product aliases to the same node (which translations the field entities) and the translation functionality included in the prior patch didn't account for that situation.
I also improved error messaging and slightly (but not completely) improved some of the code formatting.
My changes allow for the user to specify the language being set for the alias using "migrate_redirects_language" as a mapping point. I wanted to use the standard "FIELD_NAME:language", but I was having trouble understanding how that mechanism would work here. So if someone knows how to tackle that then by all means please do so.
Attached is my updated version of the patch file.
Comment #113
patrick.thurmond@gmail.comWe probably should also create a rollback option for this as well. I might try to tackle that soon.
Comment #114
patrick.thurmond@gmail.comOk, I have updated this patch again to make sure it checks the row AND the entity for the migrate_redirect_language. This fixes a problem I had and allows the mappings to passively set it instead of doing so in the prepareRow().
Comment #115
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedBack to needs review since there are two new patches...
Comment #116
patrick.thurmond@gmail.comOne more update to this. I wasn't thinking and my error message was occurring in unrelated migrations. Fixed this by making sure that it found redirects specified in the current migration.
Comment #117
heddnCan you attach some interdiffs? I was following this so I'd like to see what has changed. https://www.drupal.org/documentation/git/interdiff
Comment #118
patrick.thurmond@gmail.comHere is the interdiff between the first patch I used and my most recent changes to it.
*** UPDATE: Here is a pastebin instead, so that this dumb forum system stops reporting the patch as broken. ***
http://pastebin.com/5hEPPWEq
Comment #120
patrick.thurmond@gmail.comJust re-uploading my last actual patch since the stupid system won't let me delete that diff file I uploaded before.
Comment #121
mikeker CreditAttribution: mikeker as a volunteer commented@pthurmond, anything with a .diff or .patch extension will be run by the testbot. If you name your interdiff
foo.interdiff.txt
it won't run. Details here: https://www.drupal.org/node/332678Not being able to delete files is by-design -- it keeps people from rewriting history. No matter how legitimate a rewrite it may be...
Comment #122
patrick.thurmond@gmail.comSo what does it take to get the contrib merged into the module at some point? Looks like everyone did the work for the module maintainer. He/she just needs to merge it in.
Comment #123
brockfanning CreditAttribution: brockfanning commented@pthurmond, it's now the community's job to review and test, so it can eventually be marked as "Reviewed & tested by the community". In the meantime, if the patch works for you, you're welcome to use it for your needs. Drupal 7 sites are likely going to employ more and more patches, as contrib maintainers have to spend more and more time on Drupal 8. (my day-job Drupal 7 site is using over 90 patches, and counting...)
Comment #124
tauno CreditAttribution: tauno at ThinkShout commentedRe-rolled the patch to create the new file in the correct directory when using drush. No other changes to the code.
Comment #125
mikeryanUmm... The migrate.inc file got taken out of the .info file along the way, but it does need to be there. If it only contained hook_migrate_api() it would not be necessary, but because it contains classes it must be there for the core class registry to know where to find those classes on any request not invoking hook_migrate_api().
Comment #126
geerlingguy CreditAttribution: geerlingguy as a volunteer commentedTechnically should have a comma at end of line here.
Pedantic, but should read "Determine the language for the current redirect."
I think we can drop the @param docs for these two params, and just add a note under the @return declaration like redirectValidate() has.
Needs a space between // and Defaults.
Inline comments need to be in // style, and in one paragraph. If necessary, maybe we pull this comment out into the main function docblock.
Comment #127
benjifisherI am working on this at MidCamp.
Comment #128
benjifisherI fixed a few other problems reported by CodeSniffer in addition to the ones mentioned in #126.
I rewrote the multiline comment and moved it to the doc block.
I rewrote the function: instead of setting
$language
to the default value and then looking for something better, I used a boring but consistentif .. elseif ... elseif .. else
structure.Comment #129
benjifisherComment #130
Ron Collins CreditAttribution: Ron Collins at Affinity Bridge commentedThe patch at #128 worked for me. It's been 5 years in the making. Time to commit?
Comment #131
Ron Collins CreditAttribution: Ron Collins at Affinity Bridge commentedComment #132
pifagorComment #133
pifagorRTBC
Comment #134
drupalfan2 CreditAttribution: drupalfan2 as a volunteer commentedIs this patch still necessary for latest Drupal 7 version?
Comment #137
pifagor