Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, empty.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
5.33 KB

A quick start at this. Of course contextual links lack title replacement patterns (for now), to be added back in #2120235: Regression: routing / tabs / actions / contextual links lack way to attach replacement arguments to UI strings so another core dependent todo added.

Status: Needs review » Needs work

The last submitted patch, 2: contextual-links.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.18 KB
3.04 KB

Fixed several code problems, missing use statements, misnamed class, attaching contextual link to the translation route instead of the base route, etc. Still no success in making it appear :/ At least there are less fails :) Can someone take a look? :) I need to drop this for today...

Status: Needs review » Needs work

The last submitted patch, 4: contextual-links-4.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
902 bytes
6.18 KB

No, we do need to use the actual route of the translation page, so rolled that line back :) This will not work yet. I suspect there is something with the group identifier, but the group is not (well) documented at all. If I use 'views_edit_ui' as a group, then I'm starting to get other errors (about routes resolving to paths), so I suspect there may need to be some group matching.

Gábor Hojtsy’s picture

Title: Contextual links update » Config translation should work with new contextual links API

Status: Needs review » Needs work

The last submitted patch, 6: contextual-links-6.patch, failed testing.

Gábor Hojtsy’s picture

I looked at the code behind the lookup / grouping system more and indeed, we cannot just define a group like it is now, we'd need to alter the build arrays as well. Or figure out existing groups. Or both. I think this is a sizable DX step-back, or (hopefully) I'm just missing something :) [#8156943-142] is where I posted what I found.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
6.61 KB
953 bytes

Going to go with disabling contextual links testing for now. The current new implementation generates the contextual links but not in the proper group. This will allow us to move forward at least on other issues with a green HEAD (which is not the case now given the removed contextual support from hook_menu).

Gábor Hojtsy’s picture

FileSize
953 bytes

Committed #10, so HEAD will keep passing with 8.x core. Still need to fix so the actual contextual links show up again. They don't.

Status: Needs review » Needs work

The last submitted patch, 11: contextual-links-11.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
4.53 KB

Here is a stop-gap for the ones I know appear in core. Block, menu and views. Added plenty @todos to figure this out once it get better in core. As per @dawehner in #2084463-144: Convert contextual links to a plugin system similar to local tasks/actions, the same problem occurs for Views which is why views contextual links don't work in core now.

Gábor Hojtsy’s picture

Status: Needs review » Fixed

Yay, passed. Committed!

dawehner’s picture

We could just introduce that the entity related group should always be called $this->entityType. For config translation this would than
just be a derivative plugin that defines one contextual translate link for all available translatable entity types.

Gábor Hojtsy’s picture

Yeah that is an option yes :) I think in our case views was the exception for some reason. I think since views only have a UI with views_ui, calling the contextual group views_ui_* does not have a lot of advantage. OTOH Views has multiple groups, so maybe the rule needs to be "the most important / default frontend contextual group" or something like that :)

Gábor Hojtsy’s picture

@dawehner: opened #2134841: Contextual link group names are not predictable for that suggestion :)

Status: Fixed » Closed (fixed)

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