Problem/Motivation

When a D6 block with role-based visibility settings is migrated, the roles selected in the "Show blocks for specific roles" settings are not properly migrated. Currently, when a block with role-based visibility settings (in this case, "authenticated") is migrated, the resulting D8 configuration is:

visibility:
  user_role:
    id: user_role
    roles:
      - '2'
    context_mapping:
      user: user.current_user
    negate: false

When it should be:

visibility:
  user_role:
    id: user_role
    roles:
      authenticated: authenticated
    negate: false
    context_mapping:
      user: user.current_user

I'm thinking that the D6 core roles (anonymous, authenticated) should be handled in the BlockVisibility process plugin. Any D6 custom roles will need to be leverage the d6_user_role migration, I suppose?

Proposed resolution

Figure out how to properly handle core roles (anonymous, authenticated) and custom roles properly.

Remaining tasks

The work.

User interface changes

None.

API changes

None.

CommentFileSizeAuthor
#9 2415399-5-test-only.patch494.25 KBlokapujya
#9 2415399-5-REVIEW-do-not-test.patch10.73 KBlokapujya
#6 link not avaialble.png25.23 KBRavindraSingh
#5 2415399-5.patch496.91 KBultimike

Comments

chx’s picture

chx commented

You can look at how BlockPluginId / block_plugin_id uses ContainerFactoryPluginInterface to get plugin.manager.migrate.process. Note that the createInstance method call could be migrated into create. Could be because the whole thing is totally unnecessary and a separate issue should be filed to remove this from block_plugin_id since d6_custom_block keeps the ids (as do every single migrate_drupal migration) so this is superflous.

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented

chx,

Thanks - benjy pointed me to a bit of similar code in the FilterFormatPermission process plugin (that doesn't use createInstance). In either case, that's exactly what I'm looking for.

Thanks,
-mike

chx’s picture

chx commented

$container->get('plugin.manager.migrate.process')->createInstance('migration', array('migration' => 'd6_filter_format'), $migration) sure it does. It actually does what I suggested "the createInstance method call could be migrated into create".

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented

Ah - yeah, I see now. I was looking in the transform method and not create.

I'll open a new issue and create a patch to fix BlockPluginId.

Thanks,
-mike

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented
Status: Active » Needs review
StatusFileSize
new 2415399-5.patch496.91 KB

Patch attached, using the same method as in #2410623 to handle the d6.gz binary file. This patch also includes the new core/modules/migrate_drupal/src/Tests/.gitattributes file, and will likely have to be re-rolled once #2410623 is committed (or vice-versa). For now, let's see what the testbot says.

The Blocks.php D6 table dump has some unrelated "weight" changes in it, likely due to weight changes in other blocks related to tests in this patch. The unrelated weight changes are all for blocks that don't have an associated region, and therefore have no automated tests, so they shouldn't be an issue.

-mike

RavindraSingh’s picture

RavindraSingh commented
StatusFileSize
new link not avaialble.png25.23 KB 
+++ b/core/modules/migrate_drupal/src/Tests/Table/Variable.php
index b245ec9b5af38b38b36ca992a471a6529778a697..5be76f7fdb3d6d4cb8675bff4bc3d393a233b421 100644
GIT binary patch
literal 211247
zcmV)6K*+xziwFSb)XG!<1MIzNcN{s6DEj&JuZY*@o@;xaBr7*9_VnEGR%LhDvagq_
z={Y(lhnY!bMkz8PD;CMBI{NRA0QU&Sl6y+lOtmGE5eyIjK>!4S{eADl&6ofC-Wz4p
znb$fzXmvdAwV$N1Kk#}7y@O6?&%20XFG{|%-Xrg^-#IyY^0)nc_Mi7QO44Uu5DiH%
zVZVCp|6Y@n3`jzsd84EL)E~zrrSRzfLjMPEexdOdjXn0i#E-)NhXeJuzdil8|F?VC
zI&67g-hB1`^W*#1{a0^ae17rj>zmJg*3^IX{@t6iumAYZ|8zBb|Mv8G?FgUWeEIhN
z>zBe1qP=X4kMG~V`1<bS8T;+*?9D6q6^3v$7e_dI@!`!EFH0{@%)X8u{p-6AZ~FiJ
z@$3y7QvF_hRzLRW|2sT<c6j(mKXcgX>hZpvz5Bo4-mr<kdG!m-)cSkRv%3#V2KnjZ
z=Qr<uKI{MT=3jcGV&k6Yc<`_Ujf^MzA3nZ@dCFh+Jk|K<>|_7Mx33@jzrJ|S;A2?*
z?>C=68k20ba$}u+{QBmL7z}^sqw!`Agkb8gWDrn~t$=KpW-&$XPqFQP$Z$xL<bUA7
zzrFta@ss!U#mo0^ymvo&Z~p7u7se9uiTsQbaz#h|VbUK4J`Hi9c>nPJ^FN0Z5|d#{
z<9<R@*;zn{^QnJmHhJ~=&5N&Z1TYc=@A2O}_J=?EAI}fd$F0^Ak4@G)`}Y35hp2mZ
z#@_qD7Q-GqI3>yD|9Dp<hL1gN_j<<TuiyOif^od}=n+1e-ppvjN0XGK8G|MD!yAC2
z>w5GMpZ)at-G>*S|K%~>@E#-FPyY7Advo^lJI2ND!Z3RMQoRD}ks;<EfPrSXDp0^6
z_-iKm{BZ1znCL2__WO^oe&Gn#MD~9B{O;=;xgZV?y%+D9h{%)S^}8?Fz~*I$jMB&7
zoY_x*J2O7rj7L)@CTp0KKeAme<uDj*TEkI?F7E5J%bAbuO2L2(o3izcfj^}Teg;$!
zh2xZdPu0Wt#E2%t*q;I54WZpdrZ(6k$JK%&HEb=OD)9BYvoCKxfA!v-ef?-Wh64JF
z_xRB;nob#v{aH2${AAMSPdwU_9s0N8BkaFVyv>tOKiShmW?_Rt9S^bpzR6Z=;4m7|
z{*+wOev<mb%NzX)^oSqs2iSifSi3g^eTaAFhRnj+L5q)A!^&<w@GOp|QA*uI7CHf)
zyp4~SM&3mnG5(p6F*OFw<I5Vff<`c?9tN1yJ%-)G{`<lja&I%TRn5)O&HnpLr;!*5
zW;S(B&%qG6kt_K{fte+2T@IKMeA)MB`eA4mKODX6zn3KR-hJ}OXcRNa??o5hFpFbY
z#`tP(X#BNeaJwIdhvwXnwTpUHyH6iPUHuM(kL|!NgmB~)ro$(&(EM>oM^&2B&Wd<V
z1DBZRQ*I*a`IC*+7~`GgxoJF(>6oNZ+#fPKava4sm0aa`e5>5$fQNJ0u&G*}`f1Qq
z*|yXZw+4f<VgYp=;C%wk+#tkO3EN}w%huVxj9F~<chRJJ8MxkPx*S~Zu5wviP+Bc$
zbC2%GazI%4Qn+Za6ueg-Kj2TMxA;>M-*~^!n`hp)(ElT2m|=r??_M7~nrFr6A-}>4
zu>GbJK9(3Li-UQF`S$reC{npWVtT=L_laYM#(L9(yp66IBq<GHsNsaN9USN$(rZ3T
zKc&-Tj}7J}bQp!BM0^Gxr7lD|Kl7e|*5^+J?ygDVi4oN2#FtnP!}520m>KianfPGf
zhYjp11IY|6%)kQvxTb@Ijc#K6IcVQB;kp_${_h}t4xjkn<Mg>l0}7fs%sPkNYKd3%
z^+%5q&v@GjfYDzA2jj^xt%Tr3=;1?NOlOQC*f1c%a>LM+jORh>8#YRM!=%F4d;9h4

Why this misc content is here?

I see there are some block setting getting changed on your system.

See the screenshot for the reference.

block setting

ultimike’s picture

ultimike
Primary language English
Location Florida, USA
commented

@RavindraSingh,

The binary portion of the patch is for the d6.gz database dump that is used for the Migrate in Core automated tests.

The block settings being changed by my system are explained by the second paragraph of my comment 5 above.

Thanks,
-mike

benjy’s picture

benjy commented

I think we need a more reviewable patch without the database diff.

lokapujya’s picture

lokapujya
Primary language English
Location Boston
commented
StatusFileSize
new 2415399-5-REVIEW-do-not-test.patch10.73 KB
new 2415399-5-test-only.patch494.25 KB

One for review, and one for seeing the test fails.

So I tested this locally. Changed a block to anonymous, changed one to authenticated, and created a custom block with user role visibility. They all migrated.

Status: Needs review » Needs work

The last submitted patch, 9: 2415399-5-test-only.patch, failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review

Thanks for the review patch! Restoring status: #9 is a test and a review only patch and #5 is still up for review.

benjy’s picture

benjy commented
Status: Needs review » Reviewed & tested by the community 
  1. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/process/d6/BlockVisibility.php
@@ -7,18 +7,50 @@
+   * The migration plugin.
+   *
+   * @var \Drupal\migrate\Plugin\MigrateProcessInterface
+   */
+  protected $migrationPlugin;
...
+    $this->migrationPlugin = $migration_plugin;

    Not a huge for of this name but it's the same in the FilterFormatPermission plugin so lets roll with it. 

  2. +++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d6/Block.php
@@ -85,7 +85,7 @@ public function prepareRow(Row $row) {
-    $row->setSourceProperty('permissions', $roles);
+    $row->setSourceProperty('roles', $roles);

    Good catch.

Rest looks good to me, RTBC.

  • alexpott committed ef2a4a0 on 8.0.x 
    Issue #2415399 by lokapujya, ultimike: D6->D8 migration: User role based...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Migrate is not subject to beta. Committed ef2a4a0 and pushed to 8.0.x. Thanks!

@RavindraSingh there is no need to upload a image to do code snippets. Use https://dreditor.org/ for this.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.