Hello;

I am a Drupal user and front-end developer. I recently switched to Drupal 8.0.2 and I'm having some issues with the contextual links.

With permissions to use contextual links active only for admin, I still can see the DIVs generated on my markup when the page is accesed anonymously.

i.e, I can see this DIV:

<div data-contextual-id="block:block=views_block__noticias_en_portada_block_1:langcode=es|entity.view.edit_form:view=noticias_en_portada:location=block&amp;name=noticias_en_portada&amp;display_id=block_1&amp;langcode=es"></div>

Caches were cleared, both the browser's and Drupal.

This is an issue for designers because when using flexbox the presence of unwanted siblings like those divs throws away the design (since they're used for calculations - "justify-content: space-between" and the like - even when empty or height and width = 0"

I think this is a bug and not a feature. However, this is my first time filing a core issue so please help me if I am putting this in the wrong place...

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

idiaz.roncero created an issue. See original summary.

idiaz.roncero’s picture

Just for the record: obviously drush pm-uninstall quickedit and contextual solves the problem...

idiaz.roncero’s picture

I found some other people reporting the same issue at StackOverflow

http://stackoverflow.com/questions/34073778/why-is-data-contextual-id-sh...

swentel’s picture

Version: 8.0.2 » 8.0.x-dev
Status: Active » Needs review
FileSize
744 bytes

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

Status: Needs review » Needs work

The last submitted patch, 4: 2650246-4.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.89 KB

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.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +D8 cacheability
Wim Leers’s picture

Title: Contextual links shown for anonymous users » Only emit Contextual Links data- attributes when actually necessary
Related issues: +#2528498: Only emit Quick Edit data- attributes when actually necessary
swentel’s picture

So what exactly is the work now ? :)

Wim Leers’s picture

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';
swentel’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
2.88 KB
807 bytes

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

swentel’s picture

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

swentel’s picture

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 11: 2650246-11.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
zakiya’s picture

Status: Needs review » Reviewed & tested by the community

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.

zakiya’s picture

Status: Reviewed & tested by the community » Needs review

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.

Jacine’s picture

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?

Wim Leers’s picture

Queued for retesting. Patch applies cleanly locally.

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

Jacine’s picture

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`
Wim Leers’s picture

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.

swentel’s picture

@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)

swentel’s picture

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

Jacine’s picture

O.M.G. Thanks guys.

zakiya’s picture

Status: Needs review » Reviewed & tested by the community

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

xjm’s picture

Bringing #13 back to the top of the list.

xjm’s picture

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

  • catch committed 92161a9 on 8.2.x
    Issue #2650246 by swentel, Wim Leers: Only emit Contextual Links data-...

  • catch committed ea9eff3 on 8.1.x
    Issue #2650246 by swentel, Wim Leers: Only emit Contextual Links data-...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

Cottser’s picture

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

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.