Follow-up to #2549801: Improve source provider missing exception message.

Problem/Motivation

DrupalSqlBase doesn't include requirements in exception. Requirements can be used by code catching exceptions to interact with the missing requirements in a structured manner.

Proposed resolution

Re-add requirements to the requirement exception in DrupalSqlBase and assert that we can see them without munging the exception message.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

neclimdul created an issue. See original summary.

neclimdul’s picture

Status: Active » Needs review
Issue tags: -Barcelona2015, -Novice, -DevDaysMilan +Quickfix
FileSize
2.36 KB

I moved the setExpectedException as well because the only real benefit over the annotation is that we can assert where the exception happens by calling the assertion directly before the method call. So if we're going to use it lets do that.

mikeryan’s picture

Is it worth checking getRequirementsString() as well?

neclimdul’s picture

We could though we're not testing the exception so much as the fact that the requirements exist on the exception so I'm not sure if its needed.

mikeryan’s picture

Status: Needs review » Reviewed & tested by the community

OK!

fgm’s picture

Slightly related, since it is a dependency/requirements issue, there is (at least in 8.2.0-beta3) a logic inversion: DrupalSqlBase is part of migrate_drupal, but migrate depends on it: most (all ?) of the migrate Source plugins inherit from DrupalSqlBase, so it should be moved to migrate, otherwise no migration can run without enabling migrate_drupal, although non-D2D migrations don't need it.

mikeryan’s picture

@fgm: No, DrupalSqlBase is only needed when migrating from Drupal and does belong in migrate_drupal. The reason non-Drupal migrations currently need to have migrate_drupal enabled is being addressed in #2560795: Source plugins have a hidden dependency on migrate_drupal.

fgm’s picture

Terrific issue title :-)

  • xjm committed 6f7f400 on 8.3.x
    Issue #2790111 by neclimdul, mikeryan: DrupalSqlBase doesn't include...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Nice, improved debugging and a test to boot. Committed 6f7f400 and pushed to 8.3.x and 8.2.x. Thanks!

  • xjm committed 199b414 on 8.2.x
    Issue #2790111 by neclimdul, mikeryan: DrupalSqlBase doesn't include...

Status: Fixed » Closed (fixed)

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