D6/7->D8 migration: User role based block visibility settings doesn't filter missing roles [#2705977]

Problem/Motivation

Here's an example scenario. An 'editor' role is created. A block is created with visibility rules assigned to that role. The role is then deleted. Now block migrations into D8 fail because when the migrate plugin is called internally inside BlockVisibility, it cannot find a value from the role migration and the migration dies a painful death.

Proposed resolution

In the gathering of roles inside Block->prepareRow() source plugin, do a join against the roles table and only select roles that are still in existence. And filter out those that do not exist. The join should work equally well for D7 & D6. But we found this issue on a D6 site.

Remaining tasks

Attached patch seems to fix the issue but we should add a test to validate.

User interface changes

API changes

Data model changes

Comment	File	Size	Author
#17	drupal-d6_7_d8_block_role_visibility_migration-2705977-17-tests_only.patch	10.28 KB	heddn
#17	8.1.x: PHP 5.5 & MySQL 5.5 18,532 pass, 1 fail
#15	drupal-d6_7_d8_block_role_visibility_migration-2705977-15.patch	11.7 KB	heddn
#15	8.1.x: PHP 5.5 & MySQL 5.5 18,533 pass PHP 7 & PostgreSQL 9.1 18,575 pass 8.2.x: PHP 5.5 & MySQL 5.5 18,547 pass
#15	drupal-d6_7_d8_block_role_visibility_migration-2705977-15-tests_only.patch	7.93 KB	heddn
#15	8.1.x: PHP 5.5 & MySQL 5.5 Patch failed to apply
#13	drupal-d6_7_d8_block_role_visibility_migration-2705977-13-tests_only.patch	2.91 KB	heddn
#13	8.1.x: PHP 5.5 & MySQL 5.5 18,517 pass, 12 fail
#13	interdiff_10-13.txt	801 bytes	heddn
#10	drupal-d6_7_d8_block_role_visibility_migration-2705977-10-tests-only.patch	2.13 KB	heddn
#10	8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass PHP 5.6 & MySQL 5.5 18,531 pass 8.1.x: PHP 5.5 & MySQL 5.5 18,531 pass
#9	drupal-d6_7_d8_block_role_visibility_migration-2705977-9.patch	3.55 KB	edysmp
#9	8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass
#9	interdiff_8-9.txt	1.97 KB	edysmp
#8	drupal-d6_7_d8_block_role_visibility_migration-2705977-8.patch	2.37 KB	edysmp
#8	8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass
#8	interdiff-3-8.txt	1.54 KB	edysmp
#3	drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch	1.42 KB	heddn
#3	8.0.x: PHP 5.5 & MySQL 5.5 14,606 pass, 5 fail 8.2.x: PHP 5.5 & MySQL 5.5 18,526 pass, 5 fail

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

14 April 2016 at 01:28

heddn created an issue. See original summary.

Comment #2

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC, Forum One commented 14 April 2016 at 01:29

Issue summary:

View changes

Comment #3

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC, Forum One commented 14 April 2016 at 03:13

Version:	8.0.x-dev	» 8.2.x-dev
Status:	Active	» Needs review

File	Size
drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch	1.42 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,606 pass, 5 fail 8.2.x: PHP 5.5 & MySQL 5.5 18,526 pass, 5 fail

File

Size

drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch

1.42 KB

8.0.x:
PHP 5.5 & MySQL 5.5 14,606 pass, 5 fail

8.2.x:
PHP 5.5 & MySQL 5.5 18,526 pass, 5 fail

Comment #4

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 14 April 2016 at 03:18

Issue summary:	View changes
Issue tags:		+Needs tests

Comment #5

14 April 2016 at 03:53

Status:

Needs review

» Needs work

The last submitted patch, 3: drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch, failed testing.

Comment #6

14 April 2016 at 16:20

The last submitted patch, 3: drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch, failed testing.

Comment #7

14 April 2016 at 16:24

The last submitted patch, 3: drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch, failed testing.

Comment #8

edysmp

Spanish

Nicaragua

CreditAttribution: edysmp at MTech, LLC commented 14 April 2016 at 18:09

Status:	Needs work	» Needs review
Issue tags:	-Needs tests

File	Size
interdiff-3-8.txt	1.54 KB
drupal-d6_7_d8_block_role_visibility_migration-2705977-8.patch	2.37 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass

1 file was hidden/shown/deleted

File	Size
drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch	1.42 KB
8.0.x: PHP 5.5 & MySQL 5.5 14,606 pass, 5 fail 8.2.x: PHP 5.5 & MySQL 5.5 18,526 pass, 5 fail

File

Size

drupal-d6_7_d8_block_role_visibility_migration-2705977-3.patch

1.42 KB

8.0.x:
PHP 5.5 & MySQL 5.5 14,606 pass, 5 fail

8.2.x:
PHP 5.5 & MySQL 5.5 18,526 pass, 5 fail

Fixed test.
Change table users_roles to role.

Comment #9

edysmp

Spanish

Nicaragua

CreditAttribution: edysmp at MTech, LLC commented 14 April 2016 at 21:44

File	Size
interdiff_8-9.txt	1.97 KB
drupal-d6_7_d8_block_role_visibility_migration-2705977-9.patch	3.55 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass

2 files were hidden/shown/deleted

File	Size
interdiff-3-8.txt	1.54 KB
drupal-d6_7_d8_block_role_visibility_migration-2705977-8.patch	2.37 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass

Other tests.

Comment #10

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 15 April 2016 at 01:40

File	Size
drupal-d6_7_d8_block_role_visibility_migration-2705977-10-tests-only.patch	2.13 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass PHP 5.6 & MySQL 5.5 18,531 pass 8.1.x: PHP 5.5 & MySQL 5.5 18,531 pass

File

Size

drupal-d6_7_d8_block_role_visibility_migration-2705977-10-tests-only.patch

2.13 KB

8.1.x:
PHP 5.5 & MySQL 5.5 18,531 pass

Comment #11

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 15 April 2016 at 02:35

Version:	8.2.x-dev	» 8.1.x-dev
Priority:	Normal	» Major
Issue tags:		+Migrate critical

Looks like the tests don't fail yet.

Moving to 8.1.x

Comment #12

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 15 April 2016 at 22:07

We need to test BlockVisibility that it returns the right data. But the existing test of that class is doing that in a strange manner. If someone else know more about how to test these source plugins, please take a stab. Due to client hour constraints we might not be able to fix this right now.

Comment #13

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 17 April 2016 at 04:10

File	Size
interdiff_10-13.txt	801 bytes
drupal-d6_7_d8_block_role_visibility_migration-2705977-13-tests_only.patch	2.91 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,517 pass, 12 fail

This should cause lots of failures. Seems our array comparisons in MigrateTestCase are bit lax.

Comment #14

17 April 2016 at 04:56

Status:

Needs review

» Needs work

The last submitted patch, 13: drupal-d6_7_d8_block_role_visibility_migration-2705977-13-tests_only.patch, failed testing.

Comment #15

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 19 April 2016 at 04:46

Status:

Needs work

» Needs review

File	Size
drupal-d6_7_d8_block_role_visibility_migration-2705977-15-tests_only.patch	7.93 KB
8.1.x: PHP 5.5 & MySQL 5.5 Patch failed to apply
drupal-d6_7_d8_block_role_visibility_migration-2705977-15.patch	11.7 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,533 pass PHP 7 & PostgreSQL 9.1 18,575 pass 8.2.x: PHP 5.5 & MySQL 5.5 18,547 pass

5 files were hidden/shown/deleted

File	Size
interdiff_8-9.txt	1.97 KB
drupal-d6_7_d8_block_role_visibility_migration-2705977-9.patch	3.55 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass
drupal-d6_7_d8_block_role_visibility_migration-2705977-10-tests-only.patch	2.13 KB
8.2.x: PHP 5.5 & MySQL 5.5 18,531 pass PHP 5.6 & MySQL 5.5 18,531 pass 8.1.x: PHP 5.5 & MySQL 5.5 18,531 pass
interdiff_10-13.txt	801 bytes
drupal-d6_7_d8_block_role_visibility_migration-2705977-13-tests_only.patch	2.91 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,517 pass, 12 fail

Comment #16

19 April 2016 at 04:53

The last submitted patch, 15: drupal-d6_7_d8_block_role_visibility_migration-2705977-15-tests_only.patch, failed testing.

Comment #17

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 19 April 2016 at 05:10

File	Size
drupal-d6_7_d8_block_role_visibility_migration-2705977-17-tests_only.patch	10.28 KB
8.1.x: PHP 5.5 & MySQL 5.5 18,532 pass, 1 fail

Comment #18

19 April 2016 at 05:50

Status:

Needs review

» Needs work

The last submitted patch, 17: drupal-d6_7_d8_block_role_visibility_migration-2705977-17-tests_only.patch, failed testing.

Comment #19

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 19 April 2016 at 12:32

Status:

Needs work

» Needs review

Patch in #15 is ready for review.

Comment #20

benjifisher

he/him or they/them

English

Boston area

CreditAttribution: benjifisher at NorthPoint Digital commented 8 May 2016 at 19:00

It looks as though the patch from #15 was tested against 8.1.x, so I just queued a test for 8.2.x. I hope that was the right thing to do. If not, then which patch should be reviewed?

Comment #21

quietone CreditAttribution: quietone as a volunteer commented 9 May 2016 at 08:03

How do the changes to d7/FieldInstanceTest.php, d7/FieldTest.php, d6/FilterFormatTest.php, d7/FilterFormatTest.php, MigrateTestCase.php and d6/NodeTest.php relate to the issue? What am I missing?

Shouldn't this have tests added to MigrateBlock test, where a role being used by a block is deleted and then the migration is run and tested?

Comment #22

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan at Virtuoso Performance commented 14 May 2016 at 17:38

@heddn - just to be clear, when you say

An 'editor' role is created. A block is created with visibility rules assigned to that role. The role is then deleted.

these are steps taken on the source site before migration? So this is triggered by a D6 (at least) data integrity bug (leaving dangling references to a non-existent role)?

Comment #23

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 16 May 2016 at 15:10

re #22, yes, it is a problem of referential integrity (or lack thereof) in at least some version of D6 & maybe D7 (???)

Comment #24

quietone CreditAttribution: quietone as a volunteer commented 20 May 2016 at 03:12

Issue tags:

+migrate-d6-d8, +migrate-d7-d8

Adding tags for Drupal source and destination version.

Comment #25

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan at Virtuoso Performance commented 6 June 2016 at 19:33

The fix itself looks good, as well as the block test changes, I'd just like to echo quietone's question on the changes to Field and FilterFormat tests - are those in scope here?

Comment #26

heddn

English

Nicaragua

CreditAttribution: heddn at MTech, LLC commented 6 June 2016 at 19:53

Re #21 / #25

It is a symptom of retrievalAssertHelper being a lot more strict. Those values are in the db dump. So we need to make sure they are also in the "expectation" of the various tests.

Comment #27

mikeryan

he/him

English

Murphysboro, IL, USA

CreditAttribution: mikeryan at Virtuoso Performance commented 6 June 2016 at 20:02

Status:

Needs review

» Reviewed & tested by the community

Ah, I see, the new call to assertArrayEquals()... Good enough for me!

Comment #28

catch

he/him

English

CreditAttribution: catch at Third and Grove commented 15 June 2016 at 10:14

Status:

Reviewed & tested by the community

» Fixed

This is the kind of issue that makes me really glad we added configuration dependencies to 8.x, even if we're still dealing with the fallout of never having them before.

ommitted/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Comment #29

15 June 2016 at 10:14

catch committed b313cdc on 8.2.x

Issue #2705977 by heddn, edysmp: D6/7->D8 migration: User role based...

Comment #30

29 June 2016 at 10:14

Status:

Fixed

» Closed (fixed)

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