Move _system_default_theme_features() to theme.inc

Committed/pushed to 8.1.x, thanks!

Making again it to RTBC

I don't think we should necessarily move this to a different file in 8.0.x in case some code/test/etc. was relying on it being available with module_load_include(). This is a good example of something that is an internal change that is safe for a minor, but might cause some disruption that we should avoid for a patch release.

I originally was going to just mark this back to fixed against 8.1.x, but the point about brittleness during bootstrap made me reconsider. @dawehner, can you clarify a bit about how you might encounter it?

Maybe a solution for 8.0.x would be to add a if !function_exists() to system.module and define the function conditionally there.

Also, for 8.1.x, isn't the function namespaced incorrectly now? So alternately, we could deprecate _system_default_theme_features() and make it call a new _theme_default_features() or such in 8.1.x?

@dawehner, thoughts?

All great points.

So how about we instead mark the function as deprecated, and inline the single use of it in theme_get_setting() with a comment that references ThemeHandler::$defaultFeatures?

Come to think of it, I wonder why there is not a getter for that property on the theme handler to begin with. The theme handler service is obviously available in theme_get_setting(). So maybe we could revert the earlier 8.1.x patch, commit the deprecation to both 8.0.x and 8.1.x so that both branches are more consistent, and inline the call in 8.0.x but move it to a getter on the theme handler in 8.1.x.

We could add a new interface and a new service, deprecating the old interface and service, or something like that.

Another way would be theme_get_setting() moves to a class, then move the property to a trait.

All of this feels like follow-up. I'd be fine with reverting the 8.1.x patch and inlining for now.

Sorry haven't read much, just spotted this issue and don't know the best solution around the architecture move, just passing through and thought I might fix #14.

+++ b/core/modules/system/system.module
@@ -1066,13 +1067,7 @@ function system_rebuild_module_data() {
 function _system_default_theme_features() {

Lets mark it also deprecated

Removes some duplication, provides minor direction to take in 9.x with @todo and deprecates the use of a private global function (which is private by _ convention)

+++ b/core/modules/system/system.module
@@ -1065,6 +1065,9 @@ function system_rebuild_module_data() {
+ * @deprecated in Drupal 8.0.x-dev, will be removed before Drupal 9.0.0.

Why dev stated here, can't only 8.0.x will work here.
I am pointing out because i haven't seen this in any documentation.

@snehi feel free to remove the -dev. It's probably not needed but there are 33 instances in core of it's use.

@dawehner so if the default features do not belong on ThemeHandler where do they belong and can we move them there rather than just making a public static property? Also can't a public static property be changed?

So if we had a ThemeFeatures thing should be ensure that it can also handle doing the theme_get_setting work?

Yay, explicitly sad patch!

Hmmm doesn't #32 prove we're missing test coverage?

Patch #32 has multiple test failures which proves it has some test coverage. @dawehner mentioned that there was problem with the test bot during that time which might have caused the positive false test result.

I think this fix needs to go into 8.1.x first and unfortunately the patch does not apply because the branches have diverged.

Let's fix this.

I've been thinking about the longer term aim of trying to refactor theme_get_settings(). One of the obvious places to move all of that code would be ThemeSettings - and actually I think that class would make a good home for this static method - ThemeSettings::defaultFeatures(). One less class to load. @dawehner what do you think?

Thanks @dawehner and @alexpott, makes sense to keep this in the ThemeSettings class.

I think we need to remove the ThemeFeatures class and use in ThemeHandler unless I'm missing something.

1. +++ b/core/includes/theme.inc
   @@ -96,16 +98,13 @@ function theme_get_registry($complete = TRUE) {
   + * @deprecated in Drupal 8.0.x, will be removed before Drupal 9.0.0.
   + *   Use \Drupal\Core\Theme\ThemeSettings::defaults() instead.
   

   Don't we want this to say deprecated in 8.2.x?

Nice spotting that @Cottser, thanks I've just cleaned-up the items from #46

Not sure what happened to this @catch, the commits look like some kind of mistake?

No I think I just failed to update the issue to fixed. Doing that now!

So there was a patch committed on #5 then the issue was re-opened for no good reason and work continued on a slightly different solution. The recent commit messages occurred when the new branch was opened. So the patch in #47 is for 8.3.x.

OK that makes more sense, but we should have had a new issue 50 comments ago, so moving back to closed (fixed). Opened #2804453: _system_default_theme_features() refactoring.