Problem/Motivation

Steps to reproduce:

  • Install the Standard profile
  • Go to admin/structure/views
  • Disable the Content view (admin/content)

Expected result:
Working site

Actual result:
WSOD

-----

When you disable the admin/content view, it no longer uses the View as a route, but falls back to the listing page provided by node module.
However, shortcut doesn't get updated, and continues to reference the no-longer-existing route.

Proposed resolution

Decouple shortcuts from menu links by creating a new content entity type: shortcut.

Remaining tasks

Patch is done, needs reviews.

User interface changes

None.

API changes

  • shortcuts are no longer stored in the menu_links table, they have their own storage, and shortcut sets no longer act as semi-bundles for menu links but as actual bundles of the new 'shortcut' entity type
  • shortcut sets no longer keep the weird uuid reference to their shortcuts, because it's not needed anymore
Files: 
CommentFileSizeAuthor
#86 2021779-followup.patch843 bytesamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,493 pass(es).
[ View ]
#76 interdiff.txt913 bytesamateescu
#76 shortcut_rewrite-76.patch91.75 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 59,378 pass(es).
[ View ]

Comments

Status:Needs review» Needs work
Issue tags:+VDC

The last submitted patch, shortcut-WSOD.patch, failed testing.

amateescu’s picture

I investigated this a bit a couple of days ago, and it's bad, really bad :/

Shortcut links don't have any kind of relationship with the menu link they were originally created from, so we have no good way of updating their 'route_name' property when the original menu link changes.

One way to do it would be in a hook_entity_presave() for menu_link, but that means doing one more select for each saved menu link, and _menu_navigation_links_rebuild() is already slow as hell.

The only way forward that I can think of is to decouple shortcuts from menu links and convert them to a separate entity type with an entity reference field.

klonos’s picture

I was pointed to this issue here from #1895160-63: Convert admin/content to a View, keep a non-views fallback with no bulk operations...

STR:

  1. disable the views_ui.module
  2. disable the views.module
  3. Symfony\Component\Routing\Exception\RouteNotFoundException: Route "view.content.page_1" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 126 of /home/s4d78154f0ffa1ad/www/core/lib/Drupal/Core/Routing/RouteProvider.php).
amateescu’s picture

The patch just adds a test for this bug, it doesn't try to solve anything..

klonos’s picture

ok, thanx. I edited my comment above.

ibullock’s picture

In addition to #3:

Same error occurs if you try to enable Views again after disabling.

dawehner’s picture

I can't really imagine this to just be major to be honest.

dawehner’s picture

Sadly we can't really know what is updated but there is at least a method on MenuLink.php which just would have to be executed again, to get the proper route name: findRouteName(). So we could truncate all route names, on module disable and then lazy-rebuild them on usage?

catch’s picture

Priority:Major» Critical

Yeah this seems critical enough and I can't see it only happening if a module gets disabled/uninstalled - would be affected by any change in alters too no?

dawehner’s picture

I thought about introducing an event subscriber for module enable/update/disable/delete or router rebuild which updates all menu_links but this won't really work, as you can't mass-update menu links (and you shouldn't).

The question is: Is the entity storage the right place to store the route_name? Isn't that information which is just between the route system and menu links, and don't belong to the actual entity. If we store the mapping for example in a key value store, or maybe in some cache array, resetting the values on a router rebuild would be simple.

With a cache array you would also get lazy rebuilding of the information.

mgifford’s picture

I just replicated the problem in #3 on SimplyTest.me twice.

Can the router be re-built when modules are disabled?

tim.plunkett’s picture

The router is rebuilt when modules are disabled. This isn't the menu router, it's menu links, which aren't declarative.

Also, disabling a module is only one of a few ways to reproduce this. See the OP.

mgifford’s picture

Thanks Tim. I just ran into this issue when trying to address another one. Appreciate the info though.

Wim Leers’s picture

This happened to me while simply disabling the admin/content view at admin/structure/views, i.e. not even disabling modules.

dawehner’s picture

Why would you ever want that ;)

Feedback on the idea of #10 is still wanted.

amateescu’s picture

I still prefer the decoupling from menu links and turning shortcuts into a separate entity type with a reference property. This wasn't possible when shortcut.module was written (D7) but it should be really easy to do now.

dawehner’s picture

I somehow doubt that a) this can actually happen due to API freeze and b) this will happen, given that people are already quite busy.

amateescu’s picture

Assigned:Unassigned» amateescu

Afaik, API freeze does not apply to critical bugs. And b) you knew exactly what buttons to push to make me work on it, didn't you? :P

First of all, we need to do #2036629: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set' so we have the 'shortcut' namespace available here.

catch’s picture

API freeze doesn't apply to critical bugs when an API change is required to fix the bug in a sane way. #10 looks sane enough to me.

amateescu’s picture

The thing is, I'm doing #16, not #10... Let me know if I should stop :)

catch’s picture

Unless there's a particular reason not to do #10 let's do that please.

amateescu’s picture

Assigned:amateescu» Unassigned

Here are my reasons:

  1. #10 can be done regardless of #16
  2. my approach is more like "port shortcut.module to D8", since not a lot of people care about its fate and I've been burned a lot by it while working on the conversion of menu links to entities
  3. it is 80% done, for #10 we would need to find someone to do it
  4. @pwolanin wants to turn 'link_path' into a machine name for menu links, and that won't be unique as long as we keep shortcuts in there
dawehner’s picture

@amateescu
Just to be clear, does your port will solve it automatically or do you still need some similar logic as #10?

amateescu’s picture

It will solve it because shortcuts will not be saved as menu links anymore (basically duplicating the menu link with a different menu and link_title), but as standalone entities with a reference to a menu link.

tim.plunkett’s picture

See #1987762: Convert node_add_page() to a new style controller for another puzzling bug in shortcut/menu_links

amateescu’s picture

Issue tags:-VDC
StatusFileSize
new61.92 KB

Here's what I have so far. The new entity type seems to work, but we still need to update the old code that expected menu links and receive shorcuts now.

andypost’s picture

anyway this approach requires re-sync of links and references when shortcut module disable/enable and there's no code that listens for changes on menu-link entity

otoh moving from string translation to entity translation looks promising

+++ b/core/modules/shortcut/shortcut.installundefined
@@ -22,6 +22,98 @@ function shortcut_uninstall() {
function shortcut_schema() {
+  $schema['shortcut'] = array(
+    'description' => 'Stores shortcut items.',
...
+  $schema['shortcut_property_data'] = array(
+    'description' => 'Stores shortcut properties.',

another abstraction on top of menu link? suppose translation become worth

+++ b/core/modules/shortcut/shortcut.installundefined
@@ -22,6 +22,98 @@ function shortcut_uninstall() {
function shortcut_schema() {
+  $schema['shortcut'] = array(
...
+      'menu_link' => array(

maybe better use route_name here?

amateescu’s picture

Issue tags:+D8MI, +language content

anyway this approach requires re-sync of links and references when shortcut module disable/enable

I guess you missed the critical issue about removing module enable/disable functionality from D8? :)

And we can't reference a route name, because we need to reference an actual path (that would be route name + params), but it's totally not worth it because menu links will already do it for us.

P.S. Could you edit your comment to remove those table schemas? Keep the first row of the declaration, but the rest doesn't bring anything for someone trying to scan this issue :)

andypost’s picture

So the only translation of shortcut title is questionable.
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is not in core yet.

PS: so shortcut becomes content entity? why not use link field for that?

amateescu’s picture

It's not questionable, it's an improvement at the moment, because menu links are not yet translatable.

Yeah, removal of module disable is not in core yet, but it's critical, so it certainly will be.

And yes, shorcuts become content entities, and I'm not using a configurable field because this is kind of a "hidden" property, it's not even shown in the shortcut form, so I think that would be overkill.

amateescu’s picture

Title:Updating a route-based menu link doesn't update its shortcut link» Decouple shortcuts from menu links
Assigned:Unassigned» amateescu
Status:Needs work» Needs review
StatusFileSize
new80.2 KB
FAILED: [[SimpleTest]]: [MySQL] 58,505 pass(es), 122 fail(s), and 71 exception(s).
[ View ]

Here's the current state of things. It's almost there, just need a couple fixes and an upgrade path.

And, of course, I need to fix the initial problem, update the route name of the shortcut when the route is changed, but that's just copying the code that does it for menu links.

Status:Needs review» Needs work

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

dawehner’s picture

Status:Needs work» Needs review
amateescu’s picture

StatusFileSize
new10.45 KB
new81.92 KB
FAILED: [[SimpleTest]]: [MySQL] 58,697 pass(es), 74 fail(s), and 4 exception(s).
[ View ]

Getting there.

Status:Needs review» Needs work

The last submitted patch, shortcut_rewrite-34.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new6.32 KB
new86.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,754 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Now with upgrade path.

Status:Needs review» Needs work

The last submitted patch, shortcut_rewrite-36.patch, failed testing.

jibran’s picture

I believe it is an API change. Issues summary is not in proper format and I think we should list all the api changes.

amateescu’s picture

Status:Needs work» Needs review
Issue tags:-Needs issue summary update, -API change, -language content
StatusFileSize
new1.47 KB
new87.21 KB
FAILED: [[SimpleTest]]: [MySQL] 58,367 pass(es), 11 fail(s), and 0 exception(s).
[ View ]

A bit more progress. One of the fails will be fixed by #2029569: Convert node_admin_nodes to a new-style Controller.

amateescu’s picture

Nope, I didn't want to remove those tags.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -API change, -D8MI, -language content

The last submitted patch, shortcut_rewrite-39.patch, failed testing.

klonos’s picture

...adding back tags that got removed again :/

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new12.21 KB
new88.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,260 pass(es), 128 fail(s), and 15 exception(s).
[ View ]

Keeping up with HEAD and a tentative fix for aliases.

Status:Needs review» Needs work

The last submitted patch, 43: shortcut_rewrite-43.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new87.89 KB
FAILED: [[SimpleTest]]: [MySQL] 59,870 pass(es), 10 fail(s), and 0 exception(s).
[ View ]
new2.77 KB

Turns out drupal_schema_get_field_value() had to be fixed, not removed from the storage controller.

Status:Needs review» Needs work

The last submitted patch, 45: shortcut_rewrite-45.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new88.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,845 pass(es).
[ View ]
new1.57 KB

And this fixes the last failures, spoke to @dawehner and it turns out getPathFromRoute() is not really deprecated after all. This should be green so I'll write a new issue summary.

amateescu’s picture

Issue summary:View changes
Issue tags:-Needs issue summary update
StatusFileSize
new89.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,664 pass(es).
[ View ]
new1.35 KB

Update the issue summary and let's also add the original test from Tim :)

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {

    @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +  public static function findRouteNameParameters($link_path) {

    As written below, this should not really be a static function.

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkInterface.php
    --- a/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php
    +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php

    +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php
    @@ -8,6 +8,7 @@
    +use Drupal\Core\Routing\UrlMatcher;

    @@ -65,11 +66,11 @@ public function testFindRouteNameParameters() {

    -    $this->assertEquals(array('view.frontpage.page_1', array()), MenuLink::findRouteNameParameters('node'));
    -    $this->assertEquals(array('node_view', array('node' => '1')), MenuLink::findRouteNameParameters('node/1'));
    -    $this->assertEquals(array('node_edit', array('node' => '2')), MenuLink::findRouteNameParameters('node/2/edit'));
    +    $this->assertEquals(array('view.frontpage.page_1', array()), UrlMatcher::findRouteNameParameters('node'));
    +    $this->assertEquals(array('node_view', array('node' => '1')), UrlMatcher::findRouteNameParameters('node/1'));
    +    $this->assertEquals(array('node_edit', array('node' => '2')), UrlMatcher::findRouteNameParameters('node/2/edit'));

    -    $this->assertEquals(array(), MenuLink::findRouteNameParameters('non-existing'));
    +    $this->assertEquals(array(), UrlMatcher::findRouteNameParameters('non-existing'));
       }

    We should try to move the test given that this does not belong into the class anymore.

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
    @@ -0,0 +1,37 @@
    +    if ($shortcut_set = shortcut_set_load($entity->bundle())) {
    ...
    +    if ($shortcut_set = shortcut_set_load($entity_bundle)) {

    Should we just use the entity manager, which is already part of the EntityAccessController

  4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php
    @@ -0,0 +1,180 @@
    +    $form_state['redirect'] = 'admin/config/user-interface/shortcut/manage/' . $entity->bundle();
    ...
    +    $form_state['redirect'] = array('admin/config/user-interface/shortcut/link/' . $this->entity->id() . '/delete');

    It would be great to already use redirect_form here.

  5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPathValue.php
    @@ -0,0 +1,52 @@
    +use InvalidArgumentException;

    Afaik we don't use classes which are in the root namespace.

  6. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
    @@ -16,15 +22,60 @@
    +   * @var \Drupal\Core\Entity\EntityManager
    ...
    +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
    ...
    +  public function __construct($entity_type, array $entity_info, ConfigFactory $config_factory, StorageInterface $config_storage, QueryFactory $entity_query_factory, UuidInterface $uuid_service, EntityManager $entity_manager) {

    We do now have an interface for the entity manager.

  7. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
    @@ -55,15 +55,14 @@ function testShortcutLinkAdd() {
    +        'title' => $title,
    +        'path'  => $test['path'],

    It would be great if we could drop that odd spacing here.

  8. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
    @@ -108,30 +109,43 @@ function testShortcutLinkChangePath() {
    +  function testShortcutLinkChangeRoute() {

    Let's mark it public if we create a new function.

  9. +++ b/core/modules/shortcut/shortcut.install
    @@ -5,23 +5,104 @@
    +  $schema['shortcut_property_data'] = array(

    node is using node_field_data instead of _property_data ... do you know which one is the recommended one?

  10. +++ b/core/modules/shortcut/shortcut.install
    @@ -101,6 +182,143 @@ function shortcut_update_8001() {
    +/**
    + * Migrate shortcuts into their own storage tables.
    + */
    +function shortcut_update_8003() {

    I am wondering whether we need to put the update into a batch process given that some sites might use shortcuts for every user.

  11. +++ b/core/modules/shortcut/shortcut.module
    @@ -400,19 +334,41 @@ function shortcut_valid_link($path) {
    +  $shortcut_links = NULL;

    Why not default to an empty array?

  12. +++ b/core/modules/shortcut/shortcut.module
    @@ -400,19 +334,41 @@ function shortcut_valid_link($path) {
    +
    +  $shortcuts = \Drupal::entityManager()->getStorageController('shortcut')->loadByProperties(array('shortcut_set' => $shortcut_set->id()));
    +  foreach ($shortcuts as $shortcut) {
    +    $links[] = array(
    +      'title' => $shortcut->label(),
    +      'href' => $shortcut->path->value,
    +    );
    +  }

    theme_links should be able to render links using the route_name and parameters, so we could already set it to drop additional work in the future.

  13. +++ b/core/modules/shortcut/shortcut.module
    @@ -434,10 +390,11 @@ function shortcut_preprocess_page(&$variables) {
    +    if (!($route_info = UrlMatcher::findRouteNameParameters($link))) {

    I really think that this should be part of the service, not backed into yet another static function.

  14. +++ b/core/profiles/standard/standard.install
    @@ -63,24 +63,21 @@ function standard_install() {
    +    'title' => t('Add content'),
    ...
    +    'title' => t('All content'),

    Just a general question: Do we really want to translate the title here and not while rendering the output?

Berdir’s picture

  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
    @@ -0,0 +1,204 @@
    + *   base_table = "shortcut",
    + *   data_table = "shortcut_property_data",
    + *   translatable = TRUE,

    Yes, I think this should be shortcut_field_data.

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
    @@ -0,0 +1,204 @@
    +    foreach ($entities as $entity) {
    +      $route_parameters = $entity->route_parameters->value;
    +      $entity->route_parameters->value = unserialize($route_parameters);
    +    }

    I have no idea what this is doing now that route_parameters is a map_field... Seems like the unserialize should happen earlier, before it's assigned?

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
    @@ -16,15 +22,60 @@
    +  /**
    +   * Constructs a FieldStorageController object.
    +   *

    wrong class name.

  4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
    @@ -16,15 +22,60 @@
       protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
         parent::attachLoad($queried_entities, $revision_id);

         foreach ($queried_entities as $id => $entity) {
    -      $links = menu_load_links('shortcut-' . $id);
    -      foreach ($links as $menu_link) {
    -        $entity->links[$menu_link->uuid()] = $menu_link;
    +      $shortcuts = $this->entityManager->getStorageController('shortcut')->loadByProperties(array('shortcut_set' => $id));
    +      foreach ($shortcuts as $shortcut) {
    +        $entity->links[$shortcut->uuid()] = $shortcut;
           }
         }

    FYI: A lot of attachLoad stuff in storage controllers is moving to entity classes in #2095283: Remove non-storage logic from the storage controllers

amateescu’s picture

StatusFileSize
new92.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,797 pass(es).
[ View ]
new19.63 KB

Great reviews, thanks guys! Fixed everything except the following:

Should we just use the entity manager, which is already part of the EntityAccessController

Sadly enough, the entity manager is not part of EntityAccessController.

I am wondering whether we need to put the update into a batch process given that some sites might use shortcuts for every user.

I wouldn't worry about it, this will be converted to a migration soon enough.

theme_links should be able to render links using the route_name and parameters, so we could already set it to drop additional work in the future.

I looked at theme_links() and it's checking for a 'path' property, so it doesn't look able to use route names and parameters.

Just a general question: Do we really want to translate the title here and not while rendering the output?

Until we introduce the mechanism for default content entities (with translation support), this is the best we got for having those titles translatable on l.d.o

amateescu’s picture

StatusFileSize
new92.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,820 pass(es).
[ View ]
new3.65 KB

Noticed a problem in the form controller, we didn't use the shortcut set storage anywhere and the parent class needs the entity manager.

amateescu’s picture

StatusFileSize
new92.36 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut_rewrite-53.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

Status:Needs review» Needs work

The last submitted patch, 53: shortcut_rewrite-53.patch, failed testing.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new92.18 KB
PASSED: [[SimpleTest]]: [MySQL] 60,373 pass(es).
[ View ]

And again.

amateescu’s picture

StatusFileSize
new92.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,006 pass(es).
[ View ]

And again. Sad to see this one still in NR.

dawehner’s picture

Status:Needs review» Needs work

And again. Sad to see this one still in NR.

Let's see what we can do.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
@@ -7,26 +7,63 @@
+   *
+   * @var \Drupal\Core\Entity\EntityManager

We should typehind for the interface instead.

Note: This was not an actual review, but to tease a bit.

Cottser’s picture

Issue tags:+Needs reroll

Looks like this needs a reroll, patch applies to 14336d94b8da95f2c2f26edb7ffb5c12d58a1212.

amateescu’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new92.59 KB
PASSED: [[SimpleTest]]: [MySQL] 59,012 pass(es).
[ View ]
new897 bytes

Rerolled and fixed the review from #57.

amateescu’s picture

StatusFileSize
new92.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,073 pass(es).
[ View ]

Yet Another Reroll. Critical bug crying for reviews here :)

dawehner’s picture

This looks really rock-solid.

public function findRouteNameParamters() {
}

Still not sure whether the url matcher is the right place. Can we replace that functionality by just using \Drupal::service('router')->match? I see why it is here ...

protected $entityQueryFactory;

We don't use the query factory as far as I can see.

$path = \Drupal::urlGenerator()->getPathFromRoute

We could use \Drupal::url() directly.

Status:Needs review» Needs work

The last submitted patch, 60: shortcut_rewrite-60.patch, failed testing.

herom’s picture

Status:Needs work» Needs review

d.o, what failed testing?

amateescu’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,078 pass(es).
[ View ]
new1.99 KB

Still not sure whether the url matcher is the right place. Can we replace that functionality by just using \Drupal::service('router')->match? I see why it is here ...

As far as I see.. we can't. Here's what I get for \Drupal::service('router')->match('node/1');

Symfony\Component\Routing\Exception\ResourceNotFoundException: None of the routers in the chain matched this request GET node/1 HTTP/1.1 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Accept-Language: en-us,en;q=0.5 Host: localhost User-Agent: Symfony/2.X in Symfony\Cmf\Component\Routing\ChainRouter->doMatch() (line 202 of D:\xampp\htdocs\drupal-8-dev\core\vendor\symfony-cmf\routing\Symfony\Cmf\Component\Routing\ChainRouter.php).

We don't use the query factory as far as I can see.

That's right, fixed.

We could use \Drupal::url() directly.

Nope, we can't.. something to do with aliases.

@herom the patch failed initially and then I asked for a retest directly on qa.d.o

amateescu’s picture

StatusFileSize
new92.25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,078 pass(es).
[ View ]

The real patch would be nice to have.

alexpott’s picture

Without this patch we can not import shortcut sets (configuration) entities since they have a hard dependency on the menu_link (content) entities because they sort the UUIDs for them in their configuration file.

alexpott’s picture

andypost’s picture

Looks RTBC except few nitpicks

  1. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
    @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {
    +  public function findRouteNameParameters($link_path) {
    ...
    +      $result = \Drupal::service('router')->matchRequest($request);

    It makes UrlMatcher depends on container/service?
    is it ok?

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
    @@ -34,21 +34,22 @@ class ShortcutSetController extends ControllerBase {
    -        drupal_set_message(t('Added a shortcut for %title.', array('%title' => $link['link_title'])));
    ...
    +        drupal_set_message($this->t('Added a shortcut for %title.', array('%title' => $name)));
    ...
    -        drupal_set_message(t('Unable to add a shortcut for %title.', array('%title' => $link['link_title'])));
    ...
    +        drupal_set_message($this->t('Unable to add a shortcut for %path.', array('%path' => $link)));

    Any reason to mix title/path?

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
    @@ -0,0 +1,37 @@
    +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    +    if ($shortcut_set = shortcut_set_load($entity_bundle)) {

    $entity_bundle optional? maybe better to use here storage controller directly?

  4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPath.php
    @@ -0,0 +1,42 @@
    +class ShortcutPath extends StringItem {
    ...
    +        'class' => '\Drupal\shortcut\ShortcutPathValue',

    It would be nice to have special PathValue in core :)

amateescu’s picture

StatusFileSize
new93.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch shortcut_rewrite-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.75 KB

Thanks for reviewing!

  1. Not sure about this one, I'll ask later today in the WSCCI meeting.
  2. Fixed.
  3. That's how the parent class sets it :) And injected the shortcut_set storage.
  4. I guess if we introduce it here, we'll just need a followup issue to move it around. It's probably the issue about adding internal path support to the link field.
xjm’s picture

alexpott’s picture

69: shortcut_rewrite-69.patch queued for re-testing.

The last submitted patch, 69: shortcut_rewrite-69.patch, failed testing.

amateescu’s picture

I forgot to post back in this issue: opened #2153891: Add a Url value object for #68.1

amateescu’s picture

StatusFileSize
new91.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,364 pass(es).
[ View ]
new7.14 KB

Or, since this patch is pretty much RTBC at this point, we can just go ahead here with a @todo (see interdiff) and make #2153891: Add a Url value object a non-critical followup?

catch’s picture

I don't think we should postpone this. And if the Url value object doesn't block anything, could just be major yeah.

amateescu’s picture

StatusFileSize
new91.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,378 pass(es).
[ View ]
new913 bytes

Proper fix for #68.2.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Awesome! I see no other nitpicks to stop the critical

catch’s picture

Title:Decouple shortcuts from menu links» Change notice: Decouple shortcuts from menu links
Category:Bug report» Task
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Great to see this one green. Original shortcut issue to add that module discussed a separate entity at one point, in case anyone wants to read back and wish they had a time machine.

Committed/pushed to 8.x, thanks!

amateescu’s picture

Woot! I did a self-review on the patch in #76 and opened this followup: #2156923: Various code cleanups for the new shortcut entity

amateescu’s picture

Title:Change notice: Decouple shortcuts from menu links» Decouple shortcuts from menu links
Category:Task» Bug report
Priority:Major» Critical
Status:Active» Fixed

Wrote a change notice here: https://drupal.org/node/2156931

xjm’s picture

Title:Decouple shortcuts from menu links» [HEAD BROKEN] Decouple shortcuts from menu links
Status:Fixed» Reviewed & tested by the community

Sadly this has broken HEAD. :(

git revert d443c1

amateescu’s picture

It seems this is a general problem with creating translatable content entities during installation. Opened an issue for it here #2156945: Clean up installer entity defaults following bugfix

I agree with the temporary revert :(

Edit: can someone unpublish the change record node?

xjm’s picture

Looking at the verbose results, the failed assertions happen on an installer page where there's an exception displayed at the bottom:

There are no fields available to insert with.

Excitingly thrown from core/lib/Drupal/Core/Database/Query/Insert.php. :) So something is going funky when installing standard in the installer test. I did double-check and standard installs fine when I install it for real in a browser or via drush si.

xjm’s picture

I unpublished the change record. Soon we will be able to revert it to a draft state instead. :) #2151041: Add a "draft" status for change records

amateescu’s picture

I did double-check and standard installs fine when I install it for real in a browser or via drush si.

It breaks only when installing in another language than English. But we can continue this in the issue I opened for it :)

amateescu’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new843 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,493 pass(es).
[ View ]

This should fix it. Thanks to @Berdir for the tip in #2156945: Clean up installer entity defaults following bugfix.

jibran’s picture

jibran’s picture

Oh nvm. We don't have the revert yet.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

Confirmed that the change fixes the testfail, we have the other issue to try and improve it.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Cool, thanks for the fast turnaround, folks!

Committed and pushed #86 to 8.x.

webchick’s picture

Title:[HEAD BROKEN] Decouple shortcuts from menu links» Decouple shortcuts from menu links
Category:Bug report» Task
Priority:Critical» Major
andypost’s picture

@xjm or @webchick please publish back change notice https://drupal.org/node/2156931

xjm’s picture

Category:Task» Bug report
Priority:Major» Critical

Fixed. :)

Status:Fixed» Closed (fixed)

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

yched’s picture