Port to Drupal 8

Issue summary update.

Pinged :) timezones are hard. I probably won't be online tonight (roughly 12 hours from now) and will be travelling a good chunk of tomorrow.

I realized I said I'd post patches and I haven't posted a patch yet, and people have been pinging me about this.

Here's a very minimal port (no settings support, it's either on or off, I commented out things to be addressed later). Patch is against 7.x-1.x-dev (7.x-1.1).

The patch does all those things, for the record :) thanks and glad it works for you.

@jorgegc thanks for creating the branch, looks like I missed your IRC ping by a few hours :( timezones--

I'm looking at porting the settings and permissions now.

Work in progress (obeys permissions and integrates with theme settings forms), not sure how correct the CMI stuff is but it seems to work :)

One thing I think is technically available again (in addition to the UI) is setting these values via a settings.php/settings.local.php file but still need to test it.

Good catch @jorgegc :) fixed that and a couple other things here. It looks like as soon as you add fields to the theme settings form they are saved with the theme settings whether you like it or not so the whole third_party_settings thing seems unnecessary and redundant at this point, removed.

Yes, tests are up next for sure, I think that's the only thing left.

In the meantime here's the syntax to test the settings.php overrides. We're not doing anything special to get this, just a built-in CMI feature:

# Enable Browsersync globally (themes can still override this so it's not really a global 'ON' switch).
$config['system.theme.global']['browsersync_enabled'] = TRUE;

# Enable Browsersync for a specific theme.
$config['bartik.settings']['browsersync_enabled'] = TRUE;

# Override port number for a specific theme.
$config['bartik.settings']['browsersync_port'] = 3002;

# Override host globally.
$config['bartik.settings']['browsersync_host'] = 'localhost';

Oh and there's probably no better time to convert to short array syntax!

And also it dawned on me that hook_form_system_theme_settings_alter() is technically only for themes although it emulates a hook_form_FORM_ID_alter(). See color_form_system_theme_settings_alter() and #943212: hook_form_system_theme_settings_alter() vs. THEME_form_system_theme_settings_alter() name-clash.

Short version: Config schema added back, D7 test ported. I added one more basic assertion and this could be expanded later to add more tests (like for the UI and other settings).

Didn't expect this but I think that the test is showing that we need to have a config schema after all. I think there is actually a core issue because of this. The theme settings form shouldn't just save everything that's added to its form, otherwise the theme settings config schema will end up incomplete/mismatched. I'll try to find an issue for that or create one.

Without adding back the config schema when you try to save the config in the test:

    // Enable Browsersync globally.
    \Drupal::configFactory()->getEditable('system.theme.global')
      ->set('browsersync_enabled', TRUE)
      ->save();

You get this:

Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for system.theme.global with the following errors: system.theme.global:browsersync_enabled missing schema in Drupal\Core\Config\Testing\ConfigSchemaChecker->onConfigSave()

@jorgegc I didn't see any PHP warnings before so if this reintroduces them please paste them in here. I think I have all the error reporting cranked up but could be something I was doing differently too :)

Oops :)

@jorgegc no worries it happens! Thanks for the update.

By the way this is how to do the settings.php overrides with the latest code:

# Enable Browsersync globally (themes can still override this so it's not really a global 'ON' switch).
$config['system.theme.global']['third_party_settings']['browsersync']['enabled'] = TRUE;

# Enable Browsersync for a specific theme (Bartik for these examples).
$config['bartik.settings']['third_party_settings']['browsersync']['enabled'] = TRUE;

# Override port number for a specific theme.
$config['bartik.settings']['third_party_settings']['browsersync']['port'] = 3002;

# Override host globally.
$config['bartik.settings']['third_party_settings']['browsersync']['host'] = 'localhost';

You can also enable Browsersync for all users, very cool :) <3 CMI

$config['user.role.anonymous']['permissions'][] = 'use browsersync';
$config['user.role.authenticated']['permissions'][] = 'use browsersync';

Edit: Hmm maybe nevermind on that one.

@joelpittet 100% agree but thought it was out of scope. Can be addressed once we have a commit on 8.x-1.x IMO :) Thanks!

@jorgegc yes I think I've done all I can without stepping on core's toes, so this should be ready to go. To prevent the redundant data from being saved into theme settings the core bug at #2623146: theme_settings_convert_to_config() shouldn't save unknown keys into theme configuration objects needs to be fixed and @joelpittet seems to agree on the path forward there and it's really a generic core issue with anyone altering the theme settings form. I'll probably work on a patch and tests for that bug in the near future if someone else doesn't beat me to it.

So in short, this is ready to roll from my perspective. Once the core bug is fixed we shouldn't need to change anything, either.

Thanks, good news! Commit authorship (in git) would be appreciated next time but no big deal. We haven't yet gotten a chance to catch up on IRC but my offer for co-maintaining the 8.x branch still stands :)

@jorgegc looks good thanks! I thought I had just caught you in IRC but didn't look closely enough at the timestamp.