Problem/Motivation
The Drupal 8 Forum module expect that the entity reference field used to link the forum nodes to the forum vocabulary is named 'taxonomy_forums'. But when we migrate Drupal 6 Forums to Drupal 8, this field is named whatever the Drupal 6 vocabulary name is. We then end up with two entity reference fields on forum nodes, with the right value being saved in the wrong field.
Proposed resolution
When we migrate vocabulary fields, check the Drupal 6 variable 'forum_nav_vocabulary' (where the forum vocabulary id is saved), and if it's the vocabulary used for forums, rename the field 'taxonomy_forums'.
Remaining tasks
Write a patch, Write tests, Review
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#63 | 2669030-63.patch | 17.04 KB | maxocub |
#63 | 2669030-63.patch | 17.04 KB | maxocub |
#58 | 2669030-53-for-8-5-x.patch | 17.98 KB | maxocub |
#58 | 2669030-53-for-8-4-x.patch | 17.98 KB | maxocub |
#58 | 2669030-53-for-8-3-x.patch | 17.98 KB | maxocub |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #5
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI also ran into this problem when testing a migration, so it's an issue as of D8.2.1
We have forum module enabled, and two migrations to comment entity destinations.
comment_forum
comment bundle provided by forum module.Interestingly, the error occurs during the migration of the NON-forum comments.
Line 261 is in
hook_ENTITY_TYPE_update() for comment entities
. It seems that$comment->getCommentedEntity()
is returning null.forum.module
implements severalhook_ENTITY_TYPE_foo()
hooks for both Node and Comment entity types. The ones specific to Node entities include a check against the node type using\Drupal::service('forum_manager')->checkNodeType($node)
, however the entity hook implementations for Comment entities do not include a check for the target node type. I wonder if that would be worth doing.My migrations use the
d6_comment
source, and haveentity_id: nid
processing, just like the migration templates from comment module.I didn't solve the problem - it only happened on --update migrations, not on a fresh migration into a site with no content.
Comment #6
joel_guesclin CreditAttribution: joel_guesclin as a volunteer commentedI am beginning to test migrations and have just tried a migration from D6 to D6 with a forum (just a single forum). On the D6 site, the forum was actually managed by the Advanced Forum module but I think that only handles displays by providing some extra Views (when I switch off the module on D6, the forum still displays as expected). At all events, here is what has happened.
As far as I can see, the actual content (the forum posts and their comments) has been correctly migrated. However:
1) If I go to forum/1056 (being the original number for the taxonomy term) the forum title is displayed, but no content.
2) On the other hand, if I display taxonomy/term/1056 then the forum content is properly listed as it would with a taxonomy term.
3) If I check the list of forums, there is indeed a forum called "General discussion" with an id of 1056
4) Whenever I access a forum post from the taxonomy listing I get a php error report with a location like this: http://d822.dd:8083/forum/1056/icc/3937/rules-forum and a referrer like this: http://d822.dd:8083/taxonomy/term/1056
and a VERY long error message of which I will post the beginning:
Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Entity\EntityStorageBase->getFromStaticCache() (line 139 of /Users/joelguesclin/Sites/d822/core/lib/Drupal/Core/Entity/EntityStorageBase.php) #0 /Users/joelguesclin/Sites/d822/core/includes/bootstrap.inc(548): _drupal_error_handler_real(2, 'array_flip(): C...', '/Users/joelgues...', 139, Array) #1 [internal function]: _drupal_error_handler(2, 'array_flip(): C...', '/Users/joelgues...', 139, Array) #2 /Users/joelguesclin/Sites/d822/core/lib/Drupal/Core/Entity/EntityStorageBase.php(139): array_flip(Array) #3 /Users/joelguesclin/Sites/d822/core/lib/Drupal/Core/Entity/EntityStorageBase.php(231): Drupal\Core\Entity\EntityStorageBase->getFromStaticCache(Array) #4 /Users/joelguesclin/Sites/d822/core/lib/Drupal/Core/Entity/EntityStorageBase.php(212): Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array)
Comment #7
joel_guesclin CreditAttribution: joel_guesclin as a volunteer commentedI changed this to "critical" because with 8.2.3 the migration of the forum just doesn't work. Here is what I did:
1) Created new Drupal 8.2.3 site
2) Enabled Forum and Book modules, plus the experimental migration modules
3) Kicked off the migration - content appears to have migrated successfully (at least I can see Book content and Forum content in the Contents list)
4) Navigate to forum/1056 (the id for the forum on the source site). It displays the headline ("General discussion forum") but nothing else.
5) Navigate to taxonomy/term/1056 - the content is listed.
I also notice that there are two Vocabularies both called "Forums", one of them empty and one containing the taxonomy term described above.
I notice sometimes ( but not systematically) when I look at a forum post, I get an enormous error message that starts with this:
Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Entity\EntityStorageBase->getFromStaticCache() (line 139 of /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Entity/EntityStorageBase.php) #0 /Users/joelguesclin/Sites/d823/core/includes/bootstrap.inc(548): _drupal_error_handler_real(2, 'array_flip(): C...', '/Users/joelgues...', 139, Array) #1 [internal function]: _drupal_error_handler(2, 'array_flip(): C...', '/Users/joelgues...', 139, Array) #2 /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Entity/EntityStorageBase.php(139): array_flip(Array) #3 /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Entity/EntityStorageBase.php(231): Drupal\Core\Entity\EntityStorageBase->getFromStaticCache(Array) #4 /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Entity/EntityStorageBase.php(212): Drupal\Core\Entity\EntityStorageBase->loadMultiple(Array) #5 /Users/joelguesclin/Sites/d823/core/modules/taxonomy/src/TermStorage.php(146): Drupal\Core\Entity\EntityStorageBase->load(NULL) #6 /Users/joelguesclin/Sites/d823/core/modules/forum/src/ForumManager.php(461): Drupal\taxonomy\TermStorage->loadAllParents(NULL) #7 /Users/joelguesclin/Sites/d823/core/modules/forum/src/Breadcrumb/ForumNodeBreadcrumbBuilder.php(29): Drupal\forum\ForumManager->getParents(NULL) #8 /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Breadcrumb/BreadcrumbManager.php(83): Drupal\forum\Breadcrumb\ForumNodeBreadcrumbBuilder->build(Object(Drupal\Core\Routing\CurrentRouteMatch)) #9 /Users/joelguesclin/Sites/d823/core/modules/system/src/Plugin/Block/SystemBreadcrumbBlock.php(72): Drupal\Core\Breadcrumb\BreadcrumbManager->build(Object(Drupal\Core\Routing\CurrentRouteMatch)) #10 /Users/joelguesclin/Sites/d823/core/modules/block/src/BlockViewBuilder.php(203): Drupal\system\Plugin\Block\SystemBreadcrumbBlock->build() #11 [internal function]: Drupal\block\BlockViewBuilder::preRender(Array) #12 /Users/joelguesclin/Sites/d823/core/lib/Drupal/Core/Render/Renderer.php(376): call_user_func('Drupal\\block\\Bl...', Array) #13
Comment #8
cilefen CreditAttribution: cilefen commentedOne broken aspect of an experimental module is not critical. See https://www.drupal.org/node/45111
Comment #9
mikeryanComment #11
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedhitting this one too .. will dig a bit as it's blocking my migration
Comment #12
Rodeo.be CreditAttribution: Rodeo.be as a volunteer commentedhave this issue as well :/
Comment #13
joel_guesclin CreditAttribution: joel_guesclin as a volunteer commentedShould I go ahead and test again using 8.3.x-dev? Or has the bug not been addressed yet?
Comment #14
Drupalized CreditAttribution: Drupalized commentedSame here. Any workaround for this?
Comment #15
NiklasBr CreditAttribution: NiklasBr commentedEncountered this error on a D6 site I tried to migrate. Have not found any workaround unfortunately.
Comment #16
robotjox CreditAttribution: robotjox commentedMe too - am I understanding this correctly: there is no way to migrate a D6 forum to D8 for now?
Comment #17
NiklasBr CreditAttribution: NiklasBr commentedI'm tempted to mark this as Migrate critical, anyone?
Comment #18
NiklasBr CreditAttribution: NiklasBr commentedComment #19
mikeryanTagging in case I don't get to it before DrupalCon...
Comment #20
maxocub CreditAttribution: maxocub commentedI'm testing this issue at DrupalCon Baltimore.
Here's what I have so far:
PHP Fatal error: Call to a member function getSetting() on null in core/modules/comment/src/Controller/CommentController.php on line 125
Comment #21
maxocub CreditAttribution: maxocub commentedI see two things that seems to be wrong after a migration:
taxonomy_forums
&forums
. The forum topic is migrated to theforums
and it's thetaxonomy_forums
that seems to be used and is empty, hence the forum node is not linked to the forum topic taxonomy term.comment_forum
but the forum module seems to be looking for a field namedcomments
.Comment #22
mikeryanre: comment field discrepancy, see #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics.
Comment #23
mikeryan@maxocub, are you actively looking at this?
Comment #24
maxocub CreditAttribution: maxocub commented@mikeryan: Yes, I am.
Comment #25
sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commented@maxocub - I need this for a site conversion; I have a large forum with several containers so if you want help testing a patch let me know, I'm glad to help test it on my site.
Comment #26
maxocub CreditAttribution: maxocub commented@sbogner: Thanks for the offer, it will be nice to test with an existing forum.
For now, the only thing I add time to do is to add some forum data in the fixtures and to build the base for the Kernel tests. I'll upload this WIP and keep working on a solution but I'll unassigned myself in case someone want to pick it up.
Comment #27
maxocub CreditAttribution: maxocub commentedJust adding a dedicated 'forums' vocabulary and a 'General discussion' forum to make it easier to test.
Putting it on NR just to see how many tests does that breaks.
Comment #29
maxocub CreditAttribution: maxocub commentedHere's the first part of the patch, that is the fix for the taxonomy.
There is still the comments to be fix.
Comment #32
maxocub CreditAttribution: maxocub commentedMoving the 'forum_vocabulary' process plugin to the taxonomy module to fix the failing tests.
Comment #34
maxocub CreditAttribution: maxocub commentedBack to NW for the forum comments.
Comment #35
sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commented@maxocub - I applied patch 32 and ran the upgrade on my actual forum data, but still didn't seem to assign the correct taxonomy (see screen snap); comments came across fine.
Comment #36
maxocub CreditAttribution: maxocub commented@sbogner: Thanks for testing the patch.
That is so weird, how come the comments could be OK since the patch don't do anything about them yet?
I also tested the patch manually and it worked for the taxonomy and not for the comments...
Comment #37
mikeryanWe have another issue for the comment type discrepancy at #2853872: Migration for forum and article comments: duplicate comment types and incorrect comment_entity_statistics - here, let's focus on the vocabulary discrepancy.
Comment #38
mikeryanThis is complete as far as the taxonomy goes, correct? So, unless there's a known gap here, I can review it this week...
Comment #40
maxocub CreditAttribution: maxocub commentedI think it's complete for the taxonomy, at least with my manual testing and with the short test included in the patch.
But @sbogner reported in #35 that it didn't worked for him.
If we do the comment part in the other issue, the issue summary needs an update.
Comment #41
sbogner@insightcp.com CreditAttribution: sbogner@insightcp.com commentedI started with a fresh install, applied 2669030-32.patch and still the same issue I reported in #35. Comments work, forum doesn't. It's really odd. I'm at Migrate version 8.3.2 if that matters.
Comment #42
maxocub CreditAttribution: maxocub commented@sbogner: I'm trying to reproduce what you are reporting but the patch from #32 does not apply to the 8.3.2 version. How are you applying the patch? It should work with the 8.3.x branch.
Comment #43
phenaproximaThis should be simplified --
$this->assertEquals(7, $node->taxonomy_forums->target_id)
Why do we need a process plugin for this? I think we could easily accomplish this with static_map.
Comment #44
maxocub CreditAttribution: maxocub commentedHere's adressing #43. I kept the process plugin because static_map won't work since we would need to get the value of a variable dynamically. I documented it instead, as discussed with @phenaproxima.
Comment #46
maxocub CreditAttribution: maxocub commentedMy last patch was against 8.4.x, so here's a reroll for 8.3.x.
I also added a link to this issue in the comments.
Comment #48
maxocub CreditAttribution: maxocub commentedJust an entity count failure...
Comment #49
maxocub CreditAttribution: maxocub commentedThere were new commits on 8.3.x, so this needed a re-roll, again.
Comment #50
maxocub CreditAttribution: maxocub commentedAs per #37, we decided to focus only on the taxonomy problem here, and deal with the comments problem in an other issue.
Updated the Issue title & summary accordingly.
Added the two related issues where the comments problem is being discussed.
Comment #51
quietone CreditAttribution: quietone as a volunteer commentedAssigning for review
Comment #52
quietone CreditAttribution: quietone as a volunteer commentedThis looks good, but I am not sure about the changes to the fixture and the tests.
I don't know much about d6 forum, so I made a d6 site with the test fixture from the patch in #49. I am surprised that it does not have any forums, but it does have forum content. The forum content can't be edited and saved because there are no forums. I think that needs to be changed so the test fixture is functional from the UI. And similarly, after migrating the fixture, the existing forum content can't be edited on the d8 site.
And the proposed resolution states that this patch will rename a field to taxonomy _forums. But I don't see a test for that.
Comment #53
maxocub CreditAttribution: maxocub commentedI had to do a re-roll in #46 because the fixtures were changed in another commit, I must have messed things there, now the forum in the fixtures should be usable.
I also added some tests for the four migrations that are affected by this patch, which are:
Comment #54
quietone CreditAttribution: quietone as a volunteer commented@maxocub, thanks it looks even better now. All my concerns addressed.
Comment #56
maxocub CreditAttribution: maxocub commentedCI error, batck to RTBC.
Comment #57
catchThat's not just an error, I did a retest against 8.5.x and the patch doesn't apply there, so needs a re-roll.
Comment #58
maxocub CreditAttribution: maxocub commentedRe-roll for 8.4.x and 8.5.x
Comment #59
maxocub CreditAttribution: maxocub commentedThe changes in the re-rolls were just in context lines, so I think this can be back to RTBC.
Comment #60
catchWe don't need an @see back to the issue, just git blame does enough. However it would be good to explain why the logic lives in taxonomy module and not forum module - I assume because there's no forum migration other than the variable?
Comment #61
maxocub CreditAttribution: maxocub commentedSince #2854878: Taxonomy vocabulary with name "Type" cannot migrate from d6 to d8 got in, this one needs a major re-roll.
Comment #62
maxocub CreditAttribution: maxocub commentedHmm, with #2854878: Taxonomy vocabulary with name "Type" cannot migrate from d6 to d8 in, this issue won't work anymore.
The forum module expects it's taxonomy field name to be 'taxonomy_forums', but with the other issue, the field name will be prefixed with 'field_'.
Comment #63
maxocub CreditAttribution: maxocub commentedHere's a reroll. Sorry, I wasn't able to create an interdiff because the previous patch and this one don't apply to the same branches. I also tried with the program interdiff, but didn't worked. If someone know how, it would be appreciated.
I mostly just removed the @see and modified the process pipelines.
Comment #64
quietone CreditAttribution: quietone as a volunteer commentedThe @see has been removed from the comments as requested in #60.
Comment #67
catchCommitted/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #68
catchComment #70
dillix CreditAttribution: dillix commentedDoes this patch worked for D7 to D8 migrations?
Comment #71
maxocub CreditAttribution: maxocub for Acquia commentedComment #72
maxocub CreditAttribution: maxocub as a volunteer and for Acquia commentedComment #73
maxocub CreditAttribution: maxocub as a volunteer commented@dillix: The D7 to D8 migration doesn't have this problem since the D7 forum field for taxonomy terms is already named correctly, that is taxonomy_forums.