Improve theme compatibility error message

Seems to be an improvement for me. Do you mind adding a quick beta evalutation?

Let's also make sure the documentation gets updated.

I'll add the beta evalutation.

I've updated the documentation:
core: 8.x is required - but this documentation was already there.
Content region is required - added this to the documentation.

#2349827: Defining a theme with an .info.yml file

@Adam Clarey no not yet. Thanks for the bump, here's a quick thing that can be addressed.

+++ b/core/modules/system/system.admin.inc
@@ -413,7 +413,7 @@ function template_preprocess_system_themes_page(&$variables) {
+        $current_theme['incompatible'] = t('This version is not compatible with Drupal !core_version and should be replaced. Check the .info.yml file contains the correct \'core\' value and that the theme includes a region called \'content\'.', array('!core_version' => \Drupal::CORE_COMPATIBILITY));

Maybe wrap the string with double quotes so you don't have to escape the single quotes. See https://www.drupal.org/coding-standards#quotes

Direct change.No disruption.Only documentation change.
RTBC +1

The second sentence didn't quite read right. It was missing prepositions.

I removed the "...and should be replace." part of the first sentence. It doesn't make sense to first say the theme should be replaced, and then say check for compatibility issues. The first sentence is implying the theme is D7 or older, which we're no longer assuming.

Shouldn't we use short array syntax ?

@joshi, sure. If changing it, I would probably change all the array syntax the same function, so it is consistent. We don't have solid rules for this, though.

Updated the array syntax in the function. Hopefully, no typos. This is part of the problem with changing the array syntax. We start making out of scope changes that distract us from the issue at hand. This shouldn't be too much of a change though, so I think we're ok.

+1 for this last patch.

@sqndr, feel free to review and RTBC it then. ;)

All the short array stuff for the parts that aren't being touched is out of scope here IMO. If I had to pick one I would take #13, especially because it's 1/4 the size of the latest patch.

Is this really a bug?

No, there is no software bug and the current comment isn't incorrect. We're adding additional info to make clearer, so I'd call that a task.

For me its good

'!core_version' should be replaced with '@core_version', see #2506427: [meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand

I agree with @Cottser (#19) that the array() / [] stuff is out of scope and should be addressed in another issue.

Based on the comments #19 and #23 about the array stuff, I worked off #13. We will add the array stuff if most think differently

Also, I believe its pointless to replace !placeholder with @placeholder is one place and not deal with all occurrences with in the entire file.

@dcmul thanks for the patch! I don't think we should be bulk replacing !placeholder with @placeholder, IMO that's really no different from the mass short array conversion we had. It makes the patch hard to review, it's harder to see what the actual relevant change is and makes it less likely that this type of issue will get committed.

Also, I believe its pointless to replace !placeholder with @placeholder is one place and not deal with all occurrences with in the entire file.

If this were my own project, in terms of version control I would separate those changes into two commits. First make the actual functional change which is the reason for this issue, then do any bulk search/replace things in a separate commit. Applying that same philosophy to core means a separate issue, which in this case already exists and is being worked on.

Ok, so reroll #13 and replace the array() syntax with the short [] syntax in just that line. Should we also replace the placeholder in that line, but just that line?

I think so.

Please review.

There should be two different errors, one for the region and one for the core version, or it is oddly confusing.

The message says "This version is not compatible with Drupal 8.x. Check that the .info.yml file contains the correct 'core' value and the theme includes a region called 'content'." It should actually read "This theme is not compatible ...".

It is easy to write a test for this so I am doing that.

Looking at this now with Chris. I agree. These should be two different checks.

Looks good! The patch is RTBC but we still need some information to beta-evaluation. There is also Needs Documentation tag, does anyone know what for?

I assume the theming guide. This page already mentions that 'content' region is required. https://www.drupal.org/node/2349827

Fixed beta evaluation. Otherwise this is all good.

Committed f1e618e and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation. I agree this is a change that benefits theme developers and the benefits outweigh the disruption of committing the patch.