Deprecate automatic template_preprocess discovery and 'includes'/'file' support from hook_theme()

@catch this may still valid in 8.1.x but not 8.0.x. Maybe you have another way of dealing but I'm reposting the patch for our current 8.0.x

Bumping

3k failed tests looks promising!

I don't think we can get rid of 'file' easily as there are plenty of usages in core modules but there only seems to be one use of 'includes' that we can refactor away.

Turns out Views uses it as well, but we might be able to remove that too.

1291 failures, I think this is all because of

      // For the views module we ensure views.theme.inc is included.
      if ($def['provider'] == 'views') {
        if (!isset($hooks[$def['theme']]['includes'])) {
          $hooks[$def['theme']]['includes'] = [];
        }
        if (!in_array('views.theme.inc', $hooks[$def['theme']]['includes'])) {
          $hooks[$def['theme']]['includes'][] = $module_dir . '/views.theme.inc';
        }
      }

Nope, there is a bigger issue: 'file' sets up 'includes' for later cached calls:

        if (isset($info['file'])) {
          $include_file = isset($info['path']) ? $info['path'] : $path;
          $include_file .= '/' . $info['file'];
          include_once $this->root . '/' . $include_file;
          $result[$hook]['includes'][] = $include_file;
        }

We can still deprecate 'includes' from being declared in the first place though.

#3495943: Handle module preprocess functions as OOP hooks should make this pretty straight-forward. Once we converted the remaining preprocess functions in .inc files in core like system and views, we should be able to trigger deprecations if this is set.

ThemeCommonElements uses it too, but that should be able to be cleaned up too.

Still postponed, but wanted to have a try, all we have to do now is trigger a deprecation if one of the two keys is set, started a MR based on #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent which removes those in system module and a quickfix for authorize report that I might move to #3518822: Convert template preprocess hooks in core/includes/theme.inc, there shouldn't be too much left after that.

Blocker has been committed

Still postponed on at 50% of #3504381: [meta] Convert Template Preprocess hooks to OOP equivalent, template_preprocess is actually the main thing here, module preprocess aren't really in those files. I'm aggregating the relevant ones from the meta to check what's left, but keeping at postponed.

This is still postponed and contains 4 or so other issues and a bunch of manual fixes, but with all that, it is now green. We might need to add a BC test though for this. could be combined with a BC test for template_preprocess.

This postponed on one last issue: #3346394: Replace BlockContentController::add with EntityController::addPage via the AdminHtmlRouteProvider route provider.

The deprecation for template_preprocess is in place and is correctly picking up some bits we missed. There's one in field_ui that looks converted because it's already a one-liner calling the service, but it's a redirection to another preprocess, this can now actually directly use that.

And one in views was missed too, that seems completely untested as the old function had been deprecated.

This months theme is finishing 2011 issues.

This can finally be reviewed.

Updated the issue summary

I think this is almost ready, just one question on testing the last deprecation path.

I can add includes, would require another hook definition and include file. The only argument to not do it is the fact that we didn't have any actual test coverage in core for it. It wasn't used at all, tests or otherwise. Directly at least, having a file key is just a shorthand for having an include.

Fair enough, I'll review again without it.

I think this is almost ready then, one last question.

I see you are only deprecating if it's not already deprecated. At first I thought this was a nice little optimization, but can we end up in a bit of a bind.

E.g. we are removing this support in 12 someone could deprecate their template preprocess and try adding support for 12 and this would not notify them.

Without that check it's a double deprecation, but that might be preferable unless I'm missing something.

Other than that.
Took another look, all deprecations line up.

We usually don't test deprecation pathways but this is unique enough it is warranted.

Everything else is just a couple that were still in the include file or were using the deprecated fallback.

50 convinced me we don't need a test for the third pathway.

Whoops, didn't mean too rtbc before that question was addressed.

The intention of the deprecated check isn't to avoid double deprecation, I don't really care about that and they have a different target audience (one is for the code defining the theme hook and one is for those using it).

The reason is just to avoid having to convert those already deprecated template preprocess functions into OOP, so we end up having a deprecated functional BC function calling into an also deprecated OOP method.

It's IMHO unlikely to effect anything else or cause problems with different versions, even if we deprecate more template preprocess functions, they're already converted now in core.

> The only argument to not do it is the fact that we didn't have any actual test coverage in core for it. It wasn't used at all, tests or otherwise

This is wrong, it was used in the maintenance page template, I'm not sure if we have deprecation test coverage for that though. I'm open to adding a deprecation test example for that

> We usually don't test deprecation pathways but this is unique enough it is warranted.

I would actually invert it. We test deprecated pathways *unless* they are trivial. It just so happens that many of them are in fact trivial.

The test coverage is primarily about making sure that the code, while deprecated, still works, because it's still heavily used in contrib (template_preprocess functions). A kernel test can only partially guarantee that because the includes are not only for discovery but also rendering the templates, which we could only test with a functional test and doing that on a different request.

Either way, if you or a committer thinks it would be valuable to extend the coverage then I'm happy to do so.

Had a brief chat in slack:

We're going to add test coverage for include, but leave the deprecation exclusion for already deprecated functions.

Done.

I took a pass at updating the CR and IS

Cr could use another review but I think this is ready now.