Only emit Contextual Links data- attributes when actually necessary

Hmm yes, it seems a little silly to generate markup when the current user has no access to it.
Patch attached fixes this.

Interesting fails, even though I think the fix is right since contextual_page_attachments() does the same check, so we should fix the tests here.

So what exactly is the work now ? :)

Oops!

+++ b/core/modules/contextual/contextual.module
@@ -107,6 +107,11 @@ function contextual_help($route_name, RouteMatchInterface $route_match) {
+  // No use in adding contextual markup if the user has no access.
+  if (!\Drupal::currentUser()->hasPermission('access contextual links')) {

Nit: Let's omit the comment, that's quite obvious?

More importantly, the corresponding cache context is missing:

$variables['#cache']['contexts'][] = 'user.permissions';

1. sure, makes sense
2. ok, but we'll probably want test coverage for this then I guess, or is this enough ?

Ok scratch that patch, looked at the referenced issue where the context needs to be, new patch coming up

And go

You can't test for the presence of user.permissions because it's always present. It's just a best practice to be explicit about it.

(At some point, we should start adding the ability to tests with no required cache contexts, so that code that's written entirely correctly can indeed test for this. But that will only start making sense when 9.x is on the horizon, because only in 9.x will we be able to remove at least user.permissions as a required cache context.)

RTBC as far as I'm concerned, but I'd like a front-end person to approve it.

Front-ender here. Wasn't sure which branch to test against. The patch works on 8.0.x branch. 8.1.x seems broken for other reasons. Marking as RTBC.

Got clarity on which branch to test against. Should be 8.1.x because that's what the issue is set to. Changing back to needs review. Sorry for the noise.

I can confirm what @zakiya found. The patch is good, but 8.1.x is borked right now (can't even install Drupal), so we can't properly test it there. However, we both applied the patch to HEAD, tested it there and all is working perfectly.

Given the state of this branch, is it ok to mark RTBC?

Queued for retesting. Patch applies cleanly locally.

I guess #17 & #19 are saying Drupal 8.1 HEAD itself is currently broken?

Yup. Core itself is borked. Specifically:

( ! ) Warning: require(/Users/jacine/Sites/drupal/vendor/autoload.php): failed to open stream: No such file or directory in /Users/jacine/Sites/drupal/autoload.php on line 14`

That's not a bug. Since a few days ago, Drupal 8.1 doesn't include the vendor libraries (Symfony etc.) anymore. See https://www.drupal.org/node/2648064.

In the command line, go to your Drupal 8 install and do this: composer install. Now you can install Drupal 8.1 again.

@Jacine you need to run composer install in the root directory of drupal from now on if you checkout from git. (tarballs are find IIRC)

ah, typing the same thing [won't comment now anymore ;)]

O.M.G. Thanks guys.

Okay tested #13 on 8.1.x and it works. Marking RTBC.

Bringing #13 back to the top of the list.

I.... do not know what just happened to the files summary. Trying to hide earlier files.

Committed/pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Nice, was just coming to commit this. Thanks all :D