Problem/Motivation

Right now, the d6_filter_format and d7_filter_format migrations try to migrate the php_code filter during format migration. This filter doesn't exist in Drupal 8 (and rightfully so), but they try anyway, which results in PluginNotFoundExceptions being thrown.

Proposed Resolution

Both migrations should explicitly map php_code to filter_null. This will cause any node/comment/text field which uses php_code to display nothing until the user installs the PHP module. But I think that's preferable to throwing exceptions and potentially causing unpredictable side effects further down the line.

Remaining Tasks

  1. Write patch
  2. Add test coverage
  3. Review
  4. Commit

API & UI Changes

None.

CommentFileSizeAuthor
#21 Screen Shot 2015-09-11 at 1.46.44 PM.png61.95 KBwebchick
#21 Screen Shot 2015-09-11 at 1.45.33 PM.png21.16 KBwebchick
#18 2562073-18.patch253 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,860 pass(es). View
#14 2562073-14.patch246.41 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,819 pass(es). View
#7 interdiff-2562073-4-7.txt3.54 KBphenaproxima
#7 2562073-7.patch221.09 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,405 pass(es). View
#4 interdiff-2562073-4.txt1.66 KBphenaproxima
#4 2562073-4.patch217.41 KBphenaproxima
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,030 pass(es), 2 fail(s), and 6 exception(s). View
#4 2562073-4-FAIL.patch216.05 KBphenaproxima
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,258 pass(es), 7 fail(s), and 6 exception(s). View
#3 2562073-2.patch1.26 KBphenaproxima
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,952 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

Issue summary: View changes
phenaproxima’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
1.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,952 pass(es). View

Initial patch.

phenaproxima’s picture

Issue tags: -Needs tests
FileSize
216.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,258 pass(es), 7 fail(s), and 6 exception(s). View
217.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 110,030 pass(es), 2 fail(s), and 6 exception(s). View
1.66 KB

With tests. Sorry about the size of the patch -- I had to add a php_code format to both the D6 and D7 dump files, so a lot of extra crap was generated.

The last submitted patch, 4: 2562073-4-FAIL.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: 2562073-4.patch, failed testing.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
221.09 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,405 pass(es). View
3.54 KB

Derp.

mikeryan’s picture

So, out-of-the-box someone using the php_code filter is going to run the upgrade process and not see their content - major WTF. There needs to be communication... somewhere... that they need to either install the contrib PHP module or (better but unlikely) stop using PHP in content. Most likely there should be an alert up front in the upgrade process - but should there also be something during the migration - "This format will not work without the contrib PHP module"?

phenaproxima’s picture

I agree. Things were always broken with regard to php_code format handling -- it would just throw an exception in the migration and the user would later be greeted with the same WTF condition.

I propose that the Filter module should implement an event listener so that, before a row from either filter_format migration is saved, it sets a message or something explaining what happened. We can leverage the Migrate event system and keep everything nice and contained in the Filter module. But this should be a follow-up issue.

An alternate approach is to implement a process plugin which will either preserve the php_code filter (if the PHP module is installed), or set it to filter_null if not, with a message. Slightly more work, but also slightly less disruptive to the user. Either way, it's a follow-up.

mikeryan’s picture

+1 to the process plugin approach.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

One might quibble on extra cleanup in the role test, but fine with me...

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Needs a reroll.

phenaproxima’s picture

Status: Needs work » Needs review
FileSize
246.41 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,819 pass(es). View

Have at me, testbot.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2562073-14.patch, failed testing.

phenaproxima’s picture

Issue tags: +Needs reroll
phenaproxima’s picture

Status: Needs work » Needs review
FileSize
253 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,860 pass(es). View

Re-rolled.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

DrupalCI approves, so back to RTBC.

  • webchick committed 11cd1c5 on 8.0.x
    Issue #2562073 by phenaproxima, mikeryan: Filter format migrations need...
webchick’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
21.16 KB
61.95 KB

Ok, awesome. This patch looks a lot scarier than you'd think because it has to update the database 1400 times.

My one concern here was what would happen to an actual PHP node (like if PHP code would be suddenly visible to end users), and luckily I happen to have one: node/37.

The php_code notice that was previously appearing vanished. Here's what it ends up looking like:

When you go to edit:

Despite the truly TERRIFYING help text, nothing happens at all to the text when you save. It's just not visble on view. Could use minor novice issue.

With that, committed and pushed to 8.0.x. YEAH! :D

Status: Fixed » Closed (fixed)

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