Problem/Motivation
When doing a migration from d7, query option is processed on the d7_path_redirect plugin, but not the fragment.
If this is right, then the related d6 process plugin may also be wanted to be changed.
I am not sure about schema in d6, I do not have access to one instance now, but the change is the same.
Proposed resolution
Add code to support fragment migration from d7 to d8.
Remaining tasks
Write a patch for d7.- If d6 version is wanted too, and d6 options table field has the same structure than d7, apply the same changes, if not a new issue may be a better idea.
User interface changes
N.A.
API changes
N.A.
Data model changes
N.A.
Comments
Comment #2
marvil07 commentedComment #3
marvil07 commentedUpdate description: patch added, also tweaked proposed resolution a bit.
Comment #4
benjifisherBoth D7 and D8 use structured data for redirects: both source and target have a URL (path or absolute) and an options array. The options array can contain query, fragment, and other data.
At first I thought it was odd that the patch takes the fragment out of the options array and appends it to the URL, building a URL string like
https://www.example.com?foo=bar#section-1. However, the code already pulls the query out of the options array and adds it to the URL string, so I guess this is the right thing to do.@marvil07 and I are working on the same project, and I tested the patch in that context. It works as expected.
Comment #5
berdirHi @marvil07, long time no see!
Thanks for the fix. Would be awesome if you could extend the test coverage here, should be a one-line change both in the drupal6 fixture and the test class.
Comment #6
marvil07 commented@Berdir, glad to see you around the issue queues again!
I have changed d7 test and related fixture, let us see if tests pass.
Re-visiting this, I see I have changed only d7 version because I did not have a d6 instance to see the structure.
Looking at the
Drupal\redirect\Plugin\migrate\source\PathRedirect, I see fragment is a database field in D6, and based onDrupal\redirect\Plugin\migrate\process\PathRedirect, I think it will be a good idea to actually introduce options to the process plugin, e.g. I could use$value[2]there, but then how to handle redirects with fragment but not query.I would say that is worth a follow-up ticket :-)
Comment #7
marvil07 commentedForgot the interdiff.
Comment #8
benjifisherI already reviewed the fix in #4. Now I am looking at the test.
The changes look as if they do the correct thing: add a fragment to the D7 source and check that it is added to the migrated Redirect object in D8.
I checked the source using
unserialize().The testbot approves.
I noticed a couple of things while looking at the test, although neither is related to this issue:
lookupDestinationID()on Line 54 should belookupDestinationId().'status_code' => 0,. I assume that 307 (temporary redirect) is the default.I had to check the List of HTTP status codes for that second point. I need to get up to date with HTTP 1.1!
Comment #9
berdirThanks!