Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Gábor Hojtsy’s picture

Priority: Normal » Critical

Local tasks still seem to work for now, but it is highly unlikely that the module would be committed without this conversion.

Gábor Hojtsy’s picture

Watch this issue for how we can do this: #2032309: Use local tasks derivatives to provide local tasks for views - we need to do derivative generation as well based on the mappers. It is not as simple as dropping in a few classes.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned

Ok, I think watching the views issue progress is the best we can do ATM. :)

dawehner’s picture

There are many probles we have to figure out first, before that one can be implemented:

Nevertheless, this is a start to show how it should look like.

vijaycs85’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, config_translation-2044737-5.patch, failed testing.

Gábor Hojtsy’s picture

@dawehner: thanks for doing this! Amazing!

dawehner’s picture

Just want to say, that once the menu API changes allow us to do that I will come back. This issue and probably field_ui/views will proove that the API does actually cover the usecases.

Gábor Hojtsy’s picture

@dawehner: yay! Thanks a lot!

YesCT’s picture

Issue summary: View changes

not sure why that didn't link to the issue

YesCT’s picture

YesCT’s picture

Issue summary: View changes

ah, because it was a change record

YesCT’s picture

Issue summary: View changes

with template and missing issue

YesCT’s picture

Issue summary: View changes

oops same issue

YesCT’s picture

@tim.plunkett mentioned in irc that we don't want annotated plugins for local tasks. we want yaml.
and @dawehner pointed to #2050919: Replace local task plugin discovery with YamlDiscovery

Gábor Hojtsy’s picture

Yeah this is still in flux... :/

dawehner’s picture

Title: Provide annotated plugin for local tasks » Provide local tasks as plugins
Status: Needs work » Needs review
FileSize
11.22 KB

Yeah #2076283: Allow local task plugins on paths with a dynamic value (like a node) is in!

  • This is some basic work which at least lets the tabs appear on site-information, text-formats but not on node_type edit, as this one is not converted yet to the local task plugins.
  • The active tabs on text formats are not working properly

Status: Needs review » Needs work

The last submitted patch, config_translation-2044737-14.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Yay, thanks for your work on this.

I've extracted the id introduction only, so this can be committed ASAP. Let's see how this passes/fails.

Status: Needs review » Needs work

The last submitted patch, config-translation-id.patch, failed testing.

dawehner’s picture

Every call to drupalPost should be converted to drupalPostForm() now ... so this is kind of unrelated to the patch.

One thing the patch is using is the following snippet:

+      $routes = \Drupal::service('router.route_provider')->getRoutesByPattern('/' . $config_translation_plugin['base_path']);
+      $route_names = array_keys($routes->all());
+      $route_name = array_shift($route_names);

This bits could be replaced if the config_translation plugins would also contain the route name instead of just the base path.

Gábor Hojtsy’s picture

@dawehner: right, the drupalPost change is in #2093007: drupalPost => drupalPostForm() in tests and other core fails. We have been waiting on a core fix so we don't need to uncomment tests to make our stuff pass :) That issue should shortly come back green and get committed :)

As for route names, are all menu callbacks now converted to routes, so we can rely on that? Well, probably those that we use were converted. Mostly config entities and main site settings.

dawehner’s picture

Status: Needs work » Needs review

#16: config-translation-id.patch queued for re-testing.

Gábor Hojtsy’s picture

Ok, committed the ID patch with docs for the $id property at http://drupalcode.org/project/config_translation.git/commitdiff/0ec881a1... (although I used an autogenerated silly commit message, sorry :D)

Here is a quick edited version of #14 without those hunks.

Status: Needs review » Needs work

The last submitted patch, config_translation-2044737-14.patch, failed testing.

Gábor Hojtsy’s picture

Fails with

Undefined index: > Notice LocalTaskManager.php 215 Drupal\Core\Menu\LocalTaskManager->getLocalTasksForRoute() Not sure what would that come from...

dawehner’s picture

This is caused by having not converted all the other existing local tasks for the edit tabs. Here is a todo

YesCT’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, config_translation-2044737-24.patch, failed testing.

Gábor Hojtsy’s picture

@dawehner: do you have examples of those?

dawehner’s picture

#2095139: Checking for the active tab should use raw variables. is needed to convert any of the config entity local tasks, so I don't know of an example which already has an edit tab.

tim.plunkett’s picture

See #2095269: Remove add_edit_tab, core should have default tabs for some similar work, the issue @YesCT asked me to look at. Didn't know about this one.

Gábor Hojtsy’s picture

@tim.plunkett: yeah that one was about to just remove the edit tab autogeneration. Which issue should we consolidate this then? The less issues, the less rerolls needed :) That one? This one?

tim.plunkett’s picture

That matters much less than just getting #2095271: Add default tabs for routes expected by config_translation in first, IMO. I will read through this approach again and see how to best merge them.

Gábor Hojtsy’s picture

@tim.plunkett: yeah, sure, thanks!

Gábor Hojtsy’s picture

Status: Needs work » Closed (duplicate)

I incorporated the pieces that are still needed from this to #2095269: Remove add_edit_tab, core should have default tabs, where some overlapping code was already there. Thanks @dawehner! Learned a lot from this patch.

Gábor Hojtsy’s picture

Committed #2095269: Remove add_edit_tab, core should have default tabs and attributed dawehner on it :) Thanks again!

Gábor Hojtsy’s picture

Issue summary: View changes

copy and paste mistake before.