Convert node_add_page() to a new style controller

I'll grab this one too.

Adding the actual services file, and adding the controller to use helper ALOT.

+++ b/core/modules/node/lib/Drupal/node/NodeAddController.phpundefined
@@ -0,0 +1,66 @@
+      return new RedirectResponse('node/add/' . $type->type);

Don't you actually need url('node/add' . $type->type, array('absolute' => TRUE)); on there?

removing the hook_menu entry also feels wrong.

I guess it's sort of okay to reuse this access checker.

#8: drupal-1987762-8.patch queued for re-testing.

@dawehner Maybe it would be better to move from NodeAddAccessCheck contoller to routing.yml requirements param:

+    if (user_access('administer content types')) {
+      // There are no content types defined that the user has permission to create,
+      // but the user does have the permission to administer the content types, so
+      // grant them access to the page anyway.
+      return TRUE;
+    }

requirements:
  _permissions: 'administer content types'

Good idea. This works fine here, as the access system uses an OR conjunction at the moment.

#12: drupal-1987762-12.patch queued for re-testing.

Meh, seems random.

#12: drupal-1987762-12.patch queued for re-testing.

Yeah, sorry, that is an actual legit failure.

Wow, I can't really debug the failure, but I guess once this is a real route this might work out better.

#20: 1987762-node_add_page-routing-20.patch queued for re-testing.

Rerolled, and remove ControllerInterface implementation for NodeAddController. We don't need that if we have no deps atm. Or did you put it there in preparation? even so, if we don't need it yet, let's not use it.

#25: 1987762-25.patch queued for re-testing.

Rerolled after the RedirectResponse patch.

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.phpundefined
@@ -23,6 +25,8 @@ class ListingEmpty extends AreaPluginBase {
+    $node_access_checker = new NodeAddAccessCheck();

@@ -32,7 +36,8 @@ public function render($empty = FALSE) {
+        '#access' => $node_access_checker->access(new Route('/node'), \Drupal::request()),

Just wondering whether we should better inject the node add access service/request object?

Maybe the failure is related with #2021779: Decouple shortcuts from menu links but honestly no idea.

Stealing this, I have a couple thoughts.

Okay, injected all the things.

I think we could remove the last of the procedural functions there and use the url generator. Also Since #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation happened, the createAccess just wants the bundle type I think.

Sorry, I had the patch applied, so this seems easier!

Nice!

#35: 1987762-35.patch queued for re-testing.

My patch was clearly inferior!
@damiankloip++

Ha, I just got lucky this time... :)

Anyway, looks like a schoolboy error in my patch. I guess that's the source of all failures.

#41: 1987762-41.patch queued for re-testing.

ok, I found #1987760: Convert node_add() to a new style controller which is for the node_add conversion but was closed as a dupe of this. I think it now makes sense to roll what that patch was meant to be into here too, as they will conflict quite a bit anyway, and adding node_add to this patch isn't massive.

We could extend the NodeAddAccessCheck to check if a node type is present in the request? Then we don;t need another access checker. It would be cool to be able to use _entity_access: 'node.create' but this isn't possible as we wouldn't have a node slug in the path to get upcasted... or can we do some other trickery on the route config like adding 'node: something' or whatever we would need, I doubt that is a. good and b. would work though.

The EntityCreateAccessCheck was added specifically because we can't use _entity_access: 'foo.create'

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,154 @@
+    $node = entity_create('node', array(

Let's also use the storage controller.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.phpundefined
@@ -0,0 +1,41 @@
+    foreach (node_permissions_get_configured_types() as $type) {

Somehow we can inject this or convert to some helper function in nodeManger. This is just a thought. It is out of scope obviously.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,153 @@
+      return new RedirectResponse($this->urlGenerator->generate('node_add'));

Why not $this->urlGenerator->generate('node_add', array('node_type' => $type))? Am I missing something?

Thanks, made those changes. @jibran, sorry I missed your suggestion form the interdiff, but the actual patch has it in. You're totally right, it needs the node_type.

Also we have a bunch of failing forum tests as forum alters the node form for paths like 'node/add/forum/1' etc... by using arg(3) to set a default forum ID. We can't really use arbitrary arguments on the path anymore so I've converted this to use a forum_id parameter in the query string instead. This should fix those tests again.

What about creating nodeTypeManger and move node_permissions_get_configured_types and other helper functions to it? If someone agrees to the idea I can create an issue for this.

@jibran, having these helpers contained in a class sounds like a good idea to me. It will have to be done at some point anyway. +1

Oops, that needed to be $type->type... :?

Hopefully this fixes those fails.

+++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.phpundefined
@@ -0,0 +1,41 @@
+   * {@inheritdoc}

Hmmm I don't know. Can we use @inheritdoc for attributes as well?

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.phpundefined
@@ -0,0 +1,169 @@
+    drupal_set_title(t('Create @name', array('@name' => $node_type->name)), PASS_THROUGH);

Let's inject stringTranslationManger as well.

thanks, here are those changes. I think that just leaves the one fail in ShortcutLinksTest, which I'm not sure about the cause of atm.

Debugging this, the menu_link for node/add is being built wrong:

Drupal\menu_link\Plugin\Core\Entity\MenuLink::__set_state(array(
   'menu_name' => 'shortcut-default',
   'bundle' => 'shortcut',
   'mlid' => '110',
   'uuid' => '60b03dbe-bc79-411f-ad30-48cde382d896',
   'plid' => '0',
   'link_path' => 'admin/config',
   'router_path' => 'admin/config',
   'link_title' => 'Add content',
   'options' => 
  array (
  ),
   'module' => 'menu',
   'hidden' => '0',
   'external' => '0',
   'has_children' => '0',
   'expanded' => '0',
   'weight' => '-20',
   'depth' => '1',
   'customized' => '0',
   'p1' => '110',
   'p2' => '0',
   'p3' => '0',
   'p4' => '0',
   'p5' => '0',
   'p6' => '0',
   'p7' => '0',
   'p8' => '0',
   'p9' => '0',
   'updated' => '0',
   'route_name' => 'node_add_page',
   'routeObject' => 
  Symfony\Component\Routing\Route::__set_state(array(
     'path' => '/node/add',
     'host' => '',
     'schemes' => 
    array (
    ),
     'methods' => 
    array (
    ),
     'defaults' => 
    array (
      '_controller' => '\\Drupal\\node\\Controller\\NodeController::addPage',
    ),
     'requirements' => 
    array (
      '_permission' => 'administer content types',
      '_node_add_access' => 'TRUE',
    ),
     'options' => 
    array (
      'compiler_class' => '\\Drupal\\Core\\Routing\\RouteCompiler',
      '_access_checks' => 
      array (
        0 => 'access_check.permission',
        1 => 'access_check.node.add',
      ),
    ),
     'compiled' => NULL,
  )),
   'langcode' => 'und',
   'entityType' => 'menu_link',
   'enforceIsNew' => NULL,
   'newRevision' => false,
   'isDefaultRevision' => true,
   'path' => 'admin/config',
   'load_functions' => '',
   'to_arg_functions' => '',
   'access_callback' => 'user_access',
   'access_arguments' => 'a:1:{i:0;s:27:"access administration pages";}',
   'page_callback' => 'system_admin_config_page',
   'page_arguments' => 'a:0:{}',
   'fit' => '3',
   'number_parts' => '2',
   'context' => '0',
   'tab_parent' => '',
   'tab_root' => 'admin/config',
   'title' => 'Configuration',
   'title_callback' => 't',
   'title_arguments' => '',
   'theme_callback' => '',
   'theme_arguments' => 'a:0:{}',
   'type' => '6',
   'description' => 'Administer settings.',
   'description_callback' => 't',
   'description_arguments' => '',
   'position' => '',
   'include_file' => 'core/modules/system/system.admin.inc',
   'm_route_name' => '',
   'router_item' => 
  array (
    'weight' => '-20',
  ),
   'in_active_trail' => false,
   'href' => 'admin/config',
))

#2045453: menu_item_route_access() does not catch ResourceNotFoundException helps a bit.

This probably isn't 100% related to #2021779: Decouple shortcuts from menu links, but I bet it would fix this.

#61: 1987762-61.patch queued for re-testing.

Here's a trivial reroll with a trivial fix on top for the account vs _account change that was upsetting the node test cases. Fixes some things, probably not all.

The _node_add_access property in the route needed some minor tweaks.

[edit: oops, I failed to reload the page before posting this patch]

reroll

Cleaning up the NodeController.

1. +++ b/core/modules/node/lib/Drupal/node/Access/NodeAddAccessCheck.php
   @@ -0,0 +1,43 @@
   +      return $access_controller->createAccess($request->attributes->get('node_type')->type);
   ...
   +        return TRUE;
   ...
   +    }
   +  }
   +
   +}
   

   This needs to return either static::ALLOW or static::DENY

2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -0,0 +1,142 @@
   +  protected $urlGenerator;
   ...
   +  protected $entityManager;
   ...
   +  protected $languageManager;
   ...
   +  protected $moduleHandler;
   ...
   +    $this->entityManager = $this->container->get('plugin.manager.entity');
   ...
   +    $this->urlGenerator = $this->container->get('url_generator');
   +    $this->languageManager = $this->container->get('language_manager');
   +    $this->moduleHandler = $this->container->get('module_handler');
   

   These are all on ControllerBase, remove them

3. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -0,0 +1,142 @@
   +    $this->nodeAccess = $this->entityManager->getAccessController('node');
   +    $this->nodeTypeStorage = $this->entityManager->getStorageController('node_type');
   

   I'm not sure if this will work since setContainer won't have been called.
   Either use create(), put them in protected methods, or just make the calls to entityManager inline.

4. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -0,0 +1,142 @@
   +      return new RedirectResponse($this->urlGenerator->generate('node_add', array('node_type' => $type->type), TRUE));
   

   $this->redirect()

5. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -0,0 +1,142 @@
   +    $node = $this->entityManager->getStorageController('node')->create(array(
   

   Either inject this like the rest, or don't

6. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -0,0 +1,142 @@
   +    drupal_set_title($this->t('Create @name', array('@name' => $node_type->name)), PASS_THROUGH);
   

   use ['#title']

7. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.php
   @@ -20,6 +24,52 @@
   +   * @param array $configuration
   

   Missing oneliner

8. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.php
   @@ -32,7 +82,8 @@ public function render($empty = FALSE) {
   +        '#access' => $this->nodeAccessChecker->access(new Route('/node'), $this->request),
   

   Use AccessManager::checkNamedRoute() please

9. +++ b/core/modules/node/node.routing.yml
   @@ -12,6 +12,21 @@ node_page_edit:
   +  requirements:
   +    _permission: 'administer content types'
   +    _node_add_access: 'node'
   

   This needs _access_mode: 'ALL'

addressing everything but checkNamedRoute. Couldn't find an example of this being done elsewhere, and too late to dig into it now.

Manually testing helps :)

#2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck would help here for NodeAddAccessCheck

+++ b/core/modules/node/node.routing.yml
@@ -12,6 +12,23 @@ node_page_edit:
+    _node_add_access: 'node'
...
+    _node_add_access: 'node:{node_type}'

Not sure about this change, the node add access checker doesn't care about the values of these?

Oh, I didn't even see that added already. That is the syntax for the issue I just mentioned in #76.

Fair, makes sense. thanks!

Since we're using code syntax from that patch, here's a combined to see if it improves tests at all. Future developers, DO NOT use this patch for development. Patch in #75 is the correct one.

fixing some issues introduced when I converted this to ControllerBase (incorrectly). Manual testing of creating nodes locally works, so this should pass, or at least be close. It may still need #2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck to pass all tests.

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

reroll!

#88: drupal8.node-module.1987762-88.patch queued for re-testing.

Rerolled the patch, but didn't actually fix anything

1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -7,44 +7,74 @@
   +    $account = $request->attributes->get('_account');
   

   use currentUser() here

2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -7,44 +7,74 @@
   +    $langcode = $this->moduleHandler()->invoke('language', 'get_default_langcode', array('node', $node_type->type));
   ...
   +      'langcode' => $langcode ? $langcode : $this->languageManager()->getLanguage()->id,
   

   seems better to use language manager, related #1966436: Default *content* entity languages are not set for entities created with the API

3. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.php
   @@ -32,7 +71,7 @@ public function render($empty = FALSE) {
   -        '#access' => _node_add_access()
   +        '#access' => $this->accessManager->checkNamedRoute('node_add_page'),
   
   +++ b/core/modules/node/node.module
   @@ -940,21 +940,6 @@ function _node_revision_access(EntityInterface $node, $op = 'view', $account = N
   -function _node_add_access() {
   -  return \Drupal::service('access_manager')->checkNamedRoute('node.add_page');
   

   dot was lost

4. +++ b/core/modules/node/node.routing.yml
   @@ -39,9 +40,11 @@ node.view:
   -    _entity_form: 'node.delete'
   +    _title: 'Add page'
   +    _content: '\Drupal\node\Controller\NodeController::addPage'
   ...
   -    _entity_access: 'node.delete'
   +    _permission: 'administer content types'
   +    _node_add_access: 'node'
   

   any reason for?

1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -27,19 +28,65 @@ public function contentOverview() {
   +      return $this->redirect($this->urlGenerator()->generate('node_add', array('node_type' => $type->type), TRUE));
   

   redirect() will call generate() so this will not work. You just need to pass route name, and params into redirect.

2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -27,19 +28,65 @@ public function contentOverview() {
   +    $account = \Drupal::currentUser();
   +    $langcode = $this->moduleHandler()->invoke('language', 'get_default_langcode', array('node', $node_type->type));
   ...
   +    $form = $this->entityManager()->getForm($node);
   

   This is still mixing how the services are used.

Also, created : #2138073: Remove module_load_include() call from NodeController::revisionOverview()

101: 1987762-node-add-page-101.patch queued for re-testing.

Re-roll. Cleaned up a couple of code style issues.

1. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
   @@ -19,19 +20,63 @@
   +   * @param \Drupal\Core\Entity\EntityInterface $node_type
   ...
      public function add(EntityInterface $node_type) {
   

   In a perfect world (a follow up) we could typehint for the node type interface.

2. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.php
   @@ -19,19 +21,57 @@
   +            'title' => t('Add new content'),
   

   We should have $this->t() available.

+++ b/core/modules/node/lib/Drupal/node/Controller/NodeController.php
@@ -57,20 +57,20 @@ public function addPage() {
-  public function add(EntityInterface $node_type) {
+  public function add(NodeInterface $node) {

This change is wrong, should be NodeTypeInterface

Thank you!

Test failures seem unrelated:

Drupal\rest\Tests\Views\StyleSerializerTest	109	1	0
Drupal\views\Tests\Plugin\DisplayTest	53	1	0
Drupal\views\Tests\Plugin\PagerTest	70	1	0
Drupal\views_ui\Tests\CachedDataUITest	59	1	0

back to rtbc

Committed and pushed to 8.x. Thanks!

(Merging "node system" and "node.module" components for 8.x; disregard.)