🇬🇧United Kingdom @Xano

The difference is less with your patch than with how core has been doing this for years. These are entities. They are primarily data containers, and shouldn't really contain too much logic. Ours do, and we violate our dependency injection principles everywhere. Seeing as the entity storage is actually one of the few services other ORMs inject into their entities exactly for the purpose of CRUD methods, I wanted to pitch (hence the less invasive do-not-test patch) if anyone else thought this would be a good place to improve this part of entity API. As fixing this was obviously not part of your scope, your patch doesn't address this issue. I figured if we're changing that part of the code anyway, could we introduce a possibly better approach for these dependencies?

Here's a tiny patch that hopefully illustrates my previous comment. My main problem with our entities is that there's too much logic on them for anyone's good (they're entities, data carriers, not services). This results in entity classes requiring many services, including the entity storage for what are probably the few sane functional bits we do want on entities: CRUD operations. I can't fix entities requiring all kinds of non-entity services, but perhaps this issue is the right place to introduce an improvement for those CRUD operations. For similar reasons, I'm not too happy with static entity storages on the Entity class. Too much coupling.

The patch is probably broken, incomplete, and it implements my idea rather crudely, so if anyone ends up liking this I'll put in some more work to make it pretty :)

Other frameworks often provide similar functionality, but to prevent global access, they inject their equivalent of our entity manager/storage into the entities when they are loaded through said entity manager. You'd get something like this:

$apple = \Drupal::entityTypeManager()->getStorage('apple')->create();
# AppleStorage then sets itself on the entity.
$apple->save();
# ::save() uses $this->storage rather than \Drupal::....

I like your patch because your ::getStorage() provides a really good place for BC-compatibility behavior, if we want it. We can make it return any injected storage if it exists, and fall back to calling \Drupal and trigger a deprecation error when it does, to encourage people to the entity storage rather than all kinds of global access shortcuts like Entity::create().

Re-roll.

We would indeed be able to do that. With generic command discovery (plugins?), every module (core & contrib) can provide commands.

In order to limit the disruption during beta.

How is this a need right now? Core does not ship with any Symfony console commands right now, so adding any command will most likely not be disruptive, but additive. On top of that I have seen no reason not to ship the first command with a generic console executable/application that can discover other commands in the future, so we'll at least have a single executable. Commands themselves would then be responsible for booting core, or not, for instance.

No, there will be one app per command for now and a shell script to wrap it.

If so, could you please explain the need to enforce one app per command?

@jhedstrom: Thanks for the example. I see your point and agree. I would like to ask we focus on building the small application in such a way that it will only support the single command, but is not specific to it, meaning that in 8.1.0 we can extend it to support multiple commands without having to break BC.

1. #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible → is a critical issue.
2. It requires the current shell script to be converted to a console command for reasons related to QA.
3. To do this, Symfony's Console component was added to Drupal core in #2493807: Add symfony/console component to core → .
4. Any Console command requires a console application. Such applications can handle multiple commands, and are not tied to any specific one of them.
5. As such, it makes sense to add the application with command discovery in a separate issue, such as the one you are currently reading, and then use #2447573: [meta] Make sure 8.x - 8.x hook_update_N() testing is possible → to add specific command for database dumps.

Following this reasoning, this issue is actually a blocker of a critical issue and should therefore be made active and reset to 8.0.x again.
Building the application does not mean all shell scripts should or can be converted to console commands for 8.0.x. It merely means the basis is in place to do this anytime we think is appropriate, such as for 8.1.x or later minor versions.

Thanks for bringing this back up! I had already forgotten about it.

In general I'm not sure we should make this a route controller, and considering the complexity and the fact the trait calls entityManager() which is not defined anywhere in the trait (and I know why), making this a trait sounds messy as well. Can we make this an entity handler base class? The code in its current form isn't usable for non-entities anyway. Besides, using a class rather than a trait for something this complex will allow us to build slightly more solid code (no dark magic entityManager() for instance).

In any case, here is a short review of the patch:

1. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  public function revisionShow($revision_id) {
   +    $entity = $this->entityManager()->getStorage($this->getRevisionEntityTypeId())->loadRevision($revision_id);
   

   We generally start methods with a verb.

2. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +    if (isset($page[$this->getRevisionEntityTypeId() . 's'][$entity->id()]['#cache'])) {
   

   unset() works fine if the value to unset does not exist yet, so we don't need a condition here.

3. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +   * Page title callback for an entity revision.
   

   One-line summaries should also start with a verb.

4. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  /**
   +   * Determines if the user has permission to revert revisions.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to check revert access for.
   +   *
   +   * @return bool
   +   *   TRUE if the user has revert access.
   +   */
   +  abstract protected function hasRevertRevisionPermission(EntityInterface $entity);
   +
   +  /**
   +   * Determines if the user has permission to delete revisions.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to check delete revision access for.
   +   *
   +   * @return bool
   +   *   TRUE if the user has delete revision access.
   +   */
   +  abstract protected function hasDeleteRevisionPermission(EntityInterface $entity);
   

   Did you check if this can be done through access controllers? I understand it's probably difficult at the least; I just want to make sure that approach has been researched.

5. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  abstract protected function buildRevertRevisionLink(EntityInterface $entity, $revision_id);
   +
   +  /**
   +   * Builds a link to delete an entity revision.
   +   *
   +   * @param \Drupal\Core\Entity\EntityInterface $entity
   +   *   The entity to build a delete revision link for.
   +   * @param int $revision_id
   +   *   The revision ID of the delete link.
   +   *
   +   * @return array
   +   *   A link render array
   +   */
   +  abstract protected function buildDeleteRevisionLink(EntityInterface $entity, $revision_id);
   

   The same goes for these link builders and entities' list builder classes.

6. +++ b/core/lib/Drupal/Core/Revision/RevisionControllerTrait.php
   @@ -0,0 +1,212 @@
   +  abstract protected function getRevisionDetails(EntityInterface $revision, $is_current = FALSE);
   

   Maybe rename this to getRevisionDescription(), or something similar that indicates the output is human-readable?

7. +++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
   @@ -0,0 +1,33 @@
   + * Defines an interface for entities with timestamped revisions.
   

   This interface is not in an entity namespace, so we should not mention entities anywhere in the file. There is no technical reason for this interface to be entity-specific in its current form, except for my next comment.

+++ b/core/lib/Drupal/Core/Revision/TimestampedRevisionInterface.php
@@ -0,0 +1,33 @@
+   * @return \Drupal\Core\Entity\EntityInterface
+   *   The called entity.

Should be @return $this.

Can you please update the issue summary so it includes the reasons for adding this, which problems it solves, etc?

Also, this looks like Chosen. Do they really do similar things and if so, how do they differ?

I agree with what has been said before. Having a generic base class that will only be used for node revisions by default offers developers a good starting point for making their own revision UI when they want to.

I cleaned up the directory structure and code comments. I also added a cache clear command and tests.

I'd like to add at least one command that belongs to a module so we know the services finder does not only *.console.services.yml for core, but for modules as well. What about the user cancellation command, which belongs to Drush, but is fairly simple? The only module-specific command in core is run-tests.sh, which is too big for this issue to port to a command.

There is a binary file in here, so use --binary when creating patches.

This patch includes the vendor packages. I did not make any other changes yet.

Hi, can we talk on IRC, just to hash a few things out?

Re-roll.

1. There does not seem to be a generic way to expose Symfony Console commands. Symfony's FrameworkBundle uses a common discovery method, but that relies on bundles, which are a concept that does not exist within Drupal.
2. Do we have good use cases for using a console-specific container?
3. Why do we register the console application as a service in the console-specific container instead of building it in the console file? Testability?
4. Is there anything else we need to do in order to provide a good basis for this to replace Drush?

So the conflict here is tha we have view access/permissions for entities, but they don't define what exactly people can see. That all depends on whatever display modes the site builder configured, but theoretically the permissions and access check let you see everything that's in an entity.

If you only have view access, then you do not know what values the entity actually contains.

Not really. You're allowed to view an entity, but whether you get to see everything that's contained within it, is up to the site builder.

It should definitely not be edit. What if I don't want anyone to mess with my entities, but I'm fine with them seeing the contents?

I do understand your concern. Maybe we must leave this open to the individual entity access controllers?

RTBC if the tests pass.

Re-roll.

Cool. Thanks for the work, guys!

I only changed UI texts, with the exception of the actual operation so it inherits from the default list builder and access controller (which is what the patch is about). There is a lot more that will need to be changed, among which are route and form names.

I decided not to change any of Views' internals where I didn't have to, as not to make this patch too complex. I'd rather do all that in a follow-up.

Re-roll.

15: drupal_2201137_15.patch queued for re-testing.

Re-roll.

Classifying this as a task, as it simply consolidates the different duplicate/clone operations in core.

10: drupal_2201137_10.patch queued for re-testing.

I just talked to @dawehner and he was okay with converting Views' clone operation to duplicate in the UI, so that's what this patch adds.

Re-roll.

Some sort of $this->t?

Done.

I see your route uses a permission, rather than entity access, which defeats the purpose of this issue :) My patch does not only add the operation link, but also adds access control for the duplicate operation.

I changed the order of the checks when building entity operation links for two reasons:

• Checking whether the required link path must always be done, but is faster than checking for entity access. Therefore if an entity does not support duplication, there is less of a performance hit in this order.
• BlockAccessController does not support checking access for this operation and this order prevents it from being called unnecessarily, which prevents the test failures.

I have split the patch in two. After #363951: Reintroduction of MENU_ITEM_GROUPING → gets submitted I'll post a new patch taking care of access control here. This will make benchmarking easier.

The attached patch also performs access control and should hide the item if a user has no access to child items. All issues pointed out in #29 are dealt with. I haven't yet tested the patch thoroughly, but it seems to work. Will do more testing as soon as I'm done with my other work here.

To do: display a message if there are no children to list.

The code responsible for the access check:

/**
 * Determine if a user should have access to a menu item.
 *
 * Users should only have access if all of the following cases are true:
 * - The user has access to at least one of this item's children.
 * - The custom access callback (if any) returns TRUE.
 *
 * @param $path string
 *   The path of the menu item to check access to.
 * @param $callback string
 *   A custom access callback to check as well.
 * @param $arguments array
 *   Arguments to pass on to the access callback.
 *
 * @return boolean
 */
function menu_item_grouping_access($path, $callback, $arguments) {
  if (is_string($callback)) {
    $custom_access = call_user_func_array($callback, $arguments);
  }

  $item = array('path' => $path);
  $items = system_admin_menu_block($item);
  foreach ($items as $child) {
    _menu_check_access($child, array());
    if ($child['access']) {
      if ($custom_access) {
        return TRUE;
      }
      else {
        return FALSE;
      }
    }
  }

  return FALSE;
}

The attached patch reintroduces MENU_ITEM_GROUPING as a menu item type. Such menu items automatically get a page callback that lists all the item's children, or redirects the user if there is only one child.

1. To do: Hide the menu item if there are no children. I must say I'm not sure if this is the way to go, since it could cause some links not to be visible anywhere, although they have been enabled at admin/build/menu.
2. To do: Check if the user has permission to access any children, otherwise hide the item.
3. Custom access callbacks should be used in parallel with the access callbacks from points 1 and 2.

See this patch in action at /node/add. See anything different? No? Exactly!

By the way: node_add_page() and similar obsolete functions have not yet been removed to keep this initial patch simple.

While working at #362834: Move statistics out of the administration pages and add permissions → I noticed that MENU_ITEM_GROUPING has not been added to the new menu system in D7. As a result, we now have several functions that do pretty much the same. node_add_page() and system_admin_menu_block_page(). I think node_add_page() is a good candidate for a new MENU_ITEM_GROUPING. It needs some changes, but then it will do fine (See the patch in #362834).

However, that patch is not complete yet, since it displays a MENU_ITEM_GROUPING parent item even if there are no child items or if the user has no permission to access any of its child items. This is the point where chx told me to check out this issue.

1. I believe the names system_admin_menu_block() and system_admin_menu_block_page() are improper. They don't describe what the functions do in a short and simple fashion, nor do they make the functionality appear available to non-admin pages, which should be.
2. I suggest I suggest putting MENU_ITEM_GROUPING back in and let menu.inc automatically set menu_item_grouping() (as seen below) as the page callback. Menu.inc should also set special access callback that returns FALSE if there are no children or if the user has no permission to access them.
3. The access callback as described in #4 only checks if there are no child items, but it should also check if the user has permission to access those children. This could perhaps be done the easiest by creating a user_access_multiple() (issue), so multiple permissions can be checked at once.

/**
 * List a menu item's children.
 *
 * @return string
 */
function menu_item_grouping() {
  $item = menu_get_item();
  $items = system_admin_menu_block($item);
  // Bypass the listing if only one child is available.
  if (count($items) == 1) {
    $item = array_shift($items);
    drupal_goto($item['href']);
  }
  return theme('menu_item_grouping', $items);
}

/**
 * Theme a list of menu items.
 *
 * @param $items
 *   The items as returned from system_admin_menu_block().
 *
 * @return string
 */
function theme_menu_item_grouping($items) {
  $output = '';

  if ($items) {
    $output = '<dl class="menu-item-grouping">';
    foreach ($items as $item) {
      $output .= '<dt>' . l($item['title'], $item['href'], $item['localized_options']) . '</dt>';
      $output .= '<dd>' . filter_xss_admin($item['description']) . '</dd>';
    }
    $output .= '</dl>';
  }
  return $output;
}