[meta] Attempt to upgrade several modules to Drupal 8 and see what breaks

Eh, let's make this a meta issue for all of them.

First up, CDN. Here's the analyze report + generated patch.

Output:

$ drush dmu-upgrade cdn
Indexing...done.
Error at 497:44: expected ;                                          [error]
Error at 497:44: expected ;                                          [error]

Need to dig into that more and figure out what that's about.

Now it's Purge's turn!

Output is clean:

$ drush dmu-upgrade purge
Indexing...done.

Next up, the venerable Mollom module.

Lots of these:

$ drush dmu-upgrade mollom
Indexing...done.
Error at 2673:70: invalid expression                                 [error]
Error at 2673:70: invalid expression                                 [error]
Error at 2673:70: invalid expression                                 [error]
Error at 2673:70: invalid expression                                 [error]
Error at 2673:70: invalid expression                                 [error]
Error at 2673:70: invalid expression                                 [error]

Need to figure that out later.

And last but not least, the fabulous Localization Client!

Moar errors:

$ drush dmu-upgrade l10n_client
Indexing...done.
Error at 497:33: expected ;                                          [error]
Error at 497:33: expected ;                                          [error]
Error at 497:33: expected ;                                          [error]
Error at 497:33: expected ;                                          [error]
Error at 497:33: expected ;                                          [error]

I'm going to start by digging into those errors, then try turning on the modules and see what blows up. ;)

As a first troubleshooting step, just tried composer update to get the latest version of All The Things™. However, this only brought down new copies of symfony/filesystem and symfony/finder; Pharborist was already pointing to dev-master. However, never hurts to have the latest version of stuff, so committed the update to composer.lock.

Created a pull request against Pharborist at https://github.com/grom358/pharborist/pull/198 so those error messages go from:

Error at 497:44: expected ;                                          [error]
Error at 497:44: expected ;                                          [error]

to:

Error at 497:44 in                                                   [error]
/Users/webchick/Sites/8.x/modules/cdn/cdn.basic.farfuture.inc:
expected ;
Error at 497:44 in                                                   [error]
/Users/webchick/Sites/8.x/modules/cdn/cdn.basic.farfuture.inc:
expected ;

...much easier. :)

So, CDN first. Relevant code is this in cdn.basic.farfuture.inc:

  $exists = file_exists($path);
  # Line 497...                            v character 44.
  \Drupal::logger('cdn')-> watchdog_notice : watchdog_critical('Nested HTTP request to generate %file: %result (URL: %url, time: !time).', array(
      '!time'   => (int) $_SERVER['REQUEST_TIME'],
      '%file'   => $path,
      '%url'    => $url,
      '%result' => $exists ? 'success' : 'failure',
    ));
  return $exists;

That does indeed seem to be a legit parse error.

The D7 code looks like this:

  $exists = file_exists($path);
  watchdog(
    'cdn',
    'Nested HTTP request to generate %file: %result (URL: %url, time: !time).',
    array(
      '!time'   => (int) $_SERVER['REQUEST_TIME'],
      '%file'   => $path,
      '%url'    => $url,
      '%result' => $exists ? 'success' : 'failure',
    ),
    $exists ? WATCHDOG_NOTICE : WATCHDOG_CRITICAL
  );
  return $exists;

So something's getting bungled when watchdog() is converted to Drupal::logger(). Filed an issue at #2425439: Conversion from watchdog() to \Drupal::logger() has problems when severity has logic for this one.

Next up, Mollom!

Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]
Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]
Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]
Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]
Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]
Error at 2673:70 in /Users/webchick/Sites/8.x/modules/mollom/mollom.module: invalid expression                                                                  [error]

(Incidentally, I am not sure why it feels compelled to tell me the same error multiple times, so filed #2425447: Pharborist errors reported multiple times about that.)

This one is confusing because...

/**
 * Log a Mollom system message.
 *
 * @param $log
 *   @todo A list of message parts. Each item is an associative array whose keys are
 *   log message strings and whose corresponding values are t()-style
 *   replacement token arguments. At least one part is required.
 *
# Line 2673...                                           position 70 v
 * In essence, this is a poor man's YAML implementation (give or take some
 * details, but note especially the indentation for array sub-elements).
 */

(I put some debugging code in that died right after the parse error happened to see if this was a situation where the real problem line just moved around, but that's indeed what it's choking on.)

Pharborist should basically be ignoring that entire line, because it's part of a comment. And even if it were somehow parsing that line, it's choking on a space, not any of the other special characters in that line. I tried deleting the space and re-adding it, in case there was some weird unicode character there or whatever, but git diff didn't show any changes.

Anyway, no clue. My best guess is something got flummoxed somewhere and caused it to feed the wrong line/position number into the exception, but for now spun off #2425475: Running DMU on Mollom causes Pharborist errors on a PHP comment line.

Ok, now let's play "What's up with l10n_client"?

Error at 497:33 in /Users/webchick/Sites/8.x/modules/l10n_client/l10n_client.module: expected ;                                                                 [error]
Error at 497:33 in /Users/webchick/Sites/8.x/modules/l10n_client/l10n_client.module: expected ;                                                                 [error]
Error at 497:33 in /Users/webchick/Sites/8.x/modules/l10n_client/l10n_client.module: expected ;                                                                 [error]
Error at 497:33 in /Users/webchick/Sites/8.x/modules/l10n_client/l10n_client.module: expected ;                                                                 [error]
Error at 497:33 in /Users/webchick/Sites/8.x/modules/l10n_client/l10n_client.module: expected ;                                                                 [error]

function l10n_client_save_string() {
  # Line 497       character 33 v
  $user = \Drupal::currentUser(), $language;

Um, yep, that'd do it. :)

Original code was:

function l10n_client_save_string() {
  global $user, $language;

Need to make replacement of global $user smarter. Filed #2425483: Fix parse errors caused by multiple global variable declarations on the same line. for that.

New l10n_client patch with #2425483: Fix parse errors caused by multiple global variable declarations on the same line. applied.

And new patch for CDN with #2425439: Conversion from watchdog() to \Drupal::logger() has problems when severity has logic now fixed.

Still do not get what's up with Mollom, so I'll skip that one for now.

Next step is to actually try installing all of these and see what burns. ;)

Purge... Enables cleanly!

CDN:

 Fatal error: Declaration of Drupal\cdn\Form\CdnAdminGeneralSettingsForm::buildForm() must be compatible with Drupal\Core\Form\FormInterface::buildForm(array $form, Drupal\Core\Form\FormStateInterface $form_state) in /Users/webchick/Sites/8.x/modules/cdn/src/Form/CdnAdminGeneralSettingsForm.php on line 14

That looks like a good one to fix.

And since that just broke my local, will test Mollom and Localization Client in a few. ;)

Spun off #2425615: Declaration of buildForm must conform to FormInterface error due to settings form conversion to investigate that more.

Ok, that one's gone (thanks, phenaproxima!) Now CDN fails on enable with:

 Fatal error: Can't use method return value in write context in /Users/webchick/Sites/8.x/modules/cdn/src/Form/CdnAdminOtherSettingsForm.php on line 282

Line 282 is the closing } of the class definition. WAT? I tried Googling the error but get a bunch of references to functions like empty() and isset() and unset(), none of which are in that file anywhere. Here's a copy of the file, which looks fine to me at first glance.

For now I'm going to chalk it up to something in CDN being goofy (since a DMU upgrade of Diff worked just fine) and move onto something else.

Mollom goes boom during installation on:

 Fatal error: Cannot use isset() on the result of a function call (you can use "null !== func()" instead) in /Users/webchick/Sites/8.x/modules/mollom/src/Form/MollomAdminConfigureForm.php on line 26

Line 26 is:

# This one here.
      if (!isset($form_state->getStorage())) {
        $form_state->setStorage('select');
      }

Hm, interesting. I actually wonder if the CDN failure is related now, since that sounds awfully similar to what was said in http://stackoverflow.com/questions/1075534/cant-use-method-return-value-....

In Drupal 7, this used to be:

    if (!isset($form_state['storage']['mollom_form'])) {
      $form_state['storage']['step'] = 'select';
    }

...which was not a problem because isset() was checking a variable, not the result of a function. Spun off #2426635: When $form_state is converted, various code that checks a form state variable fails about this.

AWESOME! Please keep feedback like this coming!