[PP-1] Add "Add content" link to empty table text at admin/content

Good idea!

We should be able to use "Empty Node Frontpage behavior" directly.

I add the "Add content" link after "No content available" text

@matteom #4

The patch is throwing an error

drupal[8.0.x*] $ git apply -v add_content_link2454309-4.patch
Checking patch core/modules/node/config/install/views.view.content.yml...
error: core/modules/node/config/install/views.view.content.yml: No such file or directory

Here is what I think the patch should be

Patch #7 is showing following error when tried to apply :

Checking patch core/modules/node/config/optional/views.view.content.yml...
error: core/modules/node/config/optional/views.view.content.yml: No such file or directory

@Shivam Agarwal I queued the patch from #7 for retesting. If it did not fail then, should also work for you.

@prics #11 But we can't always rely on bot because I have seen patches which are applied cleanly but are failed during testing by bot and vice versa too.

It looks a bit odd to have the <li> here ...

I think there was something wrong in my localhost. Patch worked good. +1 to you . Please find attached scrrenshot after applying patch.

#13 is not addressed plus I guess we should add a test. Also the title in views data looks wrong 'title' => t('Empty Node Frontpage behavior')

Working on it in Drupal dev days (mentoring sprint).

I've modified the 'Empty Node Frontpage behavior' to 'Empty Node List' (used both in frontpage and content views).
I wrote a test to check if the link "Add content" is present for the content view but, doesn't seems to work locally...
I don't really understand why node_listing_empty plugin is not present in the view result.
If someone can help me ? thanks

forget to change the status.

The patch applies fine but does nothing, as you've described. Changing status to continue work on it

I rewrite the tests to check if 'Add content' link is available when no content found, and removed if there is some content.
The link was not visible before because the user was not logged in with the correct user accesses.

Applies great against the latest head, however no sign of the link at /admin/content
Steps to reproduce:
- Log in as user #1
- Remove all content or a content from the given filter, i.e. "Type: Published"
- Go to /admin/content and expect to see the "Add content" link

Trying to figure out why it's happening

Anyone else to review?

Actually, if you apply the patch, and re-install your drupal8, the link will be available in /admin/content page.
If you already have an installed drupal8, you'll have to export your configuration, and apply the diff in views.view.content.yml :
- drush cex -y
- diff sites/default/files/config_eYNsEP31-zCz2iy3VPrjwlf5KZWqbzqCUTBkEz2BY-SiHIqf5Hv8tN_AQ7Vi1nrtPa6q9YQWyA/staging/views.view.content.yml core/modules/node/config/optional/views.view.content.yml and add the node_listing_empty part
- drush cim -y

Tested and confirmed

Screen shot:

I don't think there should be a list. Instead, the link should be its own sentence right after the message that there is no content yet, similarly to what we do in list builders (what you see when you disable views).

Changing status

Hello,

Here is the modified patch to have the link juste after the "no content" label.
Hopes everything is OK.

Regards

Very nice work on the patch.

1. +++ b/core/modules/node/src/Plugin/views/area/ListingEmpty.php
   @@ -64,17 +64,11 @@ public static function create(ContainerInterface $container, array $configuratio
   +        $link = \Drupal::l($this->t('Add content.'), Url::fromRoute('node.add_page'));
   

   Let's inject link_generator service and use $link_generator->generate($text, $internal_url); here.

2. +++ b/core/modules/node/src/Plugin/views/area/ListingEmpty.php
   @@ -64,17 +64,11 @@ public static function create(ContainerInterface $container, array $configuratio
        $account = \Drupal::currentUser();
   

   We can also inject this now after fixing #1.

3. +++ b/core/modules/node/src/Plugin/views/area/ListingEmpty.php
   @@ -64,17 +64,11 @@ public static function create(ContainerInterface $container, array $configuratio
   +        $element = array('#markup' => ' ' . $link);
   

   Nice.

4. +++ b/core/modules/node/src/Tests/Views/ContentPageTest.php
   @@ -0,0 +1,52 @@
   + * Contains \Drupal\node\Tests\Views\ContentPageTest.
   

   Why can't we use an existing NodeAdminTest::testContentAdminPages() for this page? Delete all nodes in that test and assert. Let's not add one more test if we can avoid adding it ;-)

Hello, here is the corrected patch including your asked modifications.
Any feedbacks are welcome !

Thanks for fixing the feedback this is almost ready.

1. +++ b/core/modules/node/src/Tests/NodeAdminTest.php
   @@ -193,6 +193,16 @@ function testContentAdminPages() {
   +    $this->assertResponse(200);
   

   This failed.

2. +++ b/core/modules/node/src/Tests/NodeAdminTest.php
   @@ -193,6 +193,16 @@ function testContentAdminPages() {
   +    $this->assertTrue(count($this->cssSelect('.view-content .views-empty a[href="/node/add"]')) == 1);
   

   We can add assert false as well when table is not empty.

Strange... I tested it on my machine successfuly before making the patch.
I'll take a look ASAP.

Hello @jibran,

I've added the case when the link mustn't be present, I thought it was not worth it because it's more testing that the ".view-empty" div is not present but ok.
I still have the problem with the test failing on the QA, and passing on my local installation.

Do you think it has something to do with the server configuration ? I'm not sure but I already had some problems on my symfony apps when using Xpath, and got different results on different servers... but got no clue about the origin of this issue...

Thanks for adding the new assert.

+++ b/core/modules/node/src/Tests/NodeAdminTest.php
@@ -193,6 +193,19 @@ function testContentAdminPages() {
+    $this->assertResponse(200);

Just remove it. We don't have to check this at all and please upload the test only patch to confirm the fail and then we are good for RTBC.

Here is the modified patch, with interdiff and test only patch.
Hopes this will pass the test now as I don't know how to correct this if not...

I'll take a look on https://www.drupal.org/project/testbot - to try to check why it passes locally but not on the test bot. If you already have some idea, I'm eager to know them :)

Update:

Running tests like this :

php scripts/run-tests.sh --url "http://drupal8.loc" --verbose --class "Drupal\node\Tests\NodeAdminTest"

I don't have the error on my local installation... still investigating

I will look at it later today or tomorrow.

Thanks for your help, Zaporylie

Change is rather subtle but let's see what testbot thinks about it.

Is there a way to test the behavior of the testbot by an other way than updating patches on drupal.org ?

I guess it's all about PHP version. I'm testing it with php 5.5.9, testbot has php 5.4.39 (read more). I will try to look at it closer but first I need to take a rest after ddd :) You can ping me about it on IRC tomorrow.

I have to check but my configuration was the same for PHP version.
I think it's related to xpath configuration but I'm not sure.
I'll investigate too when I got some time. Take a good rest, the DDD was really fun but also a bit tiresome ;)

Ok, let's give testbot another chance :)

Hello Zaporylie, good job for the test ! Why does it work by adding a dollar after href ? What was the issue ?

href$="" means that href should ends with value (in our case "node/add"), doesn't matter if path is index.php?q=node/add, localhost/sitefolder/node/add or localhost/node/add, while href="" is looking for perfect match.

According to testbot specification, testbot webroot is in a subdirectory and clean URLs are disabled so link to "Add content" looks like that: <a href="checkout/?q=node/add">Add content</a> while we were looking for perfect match to <a href="/node/add">Add content</a>

I think we are ready for RTBC now. It would be great to add beta evaluation and print screens for "before" and "after" to issue summary.

Before and After Screenshots

Before and After Screenshots

No need for beta eval this is simple usability bug. This is ready thanks @zaporylie for sticking with it.

This changes how the front page looks when there is no content. Is this by design? If so it's not mentioned in any of the comments. And there are no screenshots of this part of the change on the issue.

@alexpott is right.

Frontpage before the patch

Frontpage after the patch

Current patch fixes above issue.

The list item always looked a bit overdone to me. Seems fine to change this link from a list item to an inline link, because that makes it consistent with the 'empty' pattern we use in tables.

+++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
@@ -0,0 +1,99 @@
+        $link = $this->linkGenerator->generate($this->t('Add content.'), Url::fromRoute('node.add_page'));
+        $element = array('#markup' => ' ' . $link);

What about just using #type link here?

I am removing the Novice tag from this issue because there are no clear action items in this task.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

Working on #72.

@dawehner how about this? It's same as block content.

+++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
@@ -0,0 +1,106 @@
+        '#cache' => [
+          'contexts' => $access_result->getCacheContexts(),
+          'tags' => $access_result->getCacheTags(),
+          'max-age' => $access_result->getCacheMaxAge(),
+        ],

Is there no api for that, maybe BubbleMetadata::fromObject->applyTo() or that?

+++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
@@ -87,13 +87,20 @@ public static function create(ContainerInterface $container, array $configuratio
+      $access_result = $this->accessManager->checkNamedRoute('node.add_page', array(), $this->currentUser, TRUE);
+      $element = [
+        '#markup' => ' ' . $this->linkGenerator->generate($this->t('Add content.'), Url::fromRoute('node.add_page')),
+        '#access' => $access_result->isAllowed(),
+        '#cache' => [
+          'contexts' => $access_result->getCacheContexts(),
+          'tags' => $access_result->getCacheTags(),
+          'max-age' => $access_result->getCacheMaxAge(),
+        ],
+      ];

$access_result = …;
$element = [
  '#type' => 'link',
  '#title' => $this->t('Add content'),
  '#url' => Url::fromRoute('node.add_page'),
  '#access' => $access_result->isAllowed(),
];
$access_result->applyTo($element);

Changed the type to link. But can't do $access_result->applyTo($element); because of Fatal error: Call to undefined method Drupal\Core\Access\AccessResultAllowed::applyTo()

Addressed #77 and #78. Thanks @dawehner for the suggestion and thanks @Wim Leers for the help.

+++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
@@ -0,0 +1,106 @@
+ * Contains \Drupal\node\Plugin\views\area\AdminListingEmpty.
...
+   * Constructs a new ListingEmpty.

Nitpick: doesn't match.

Thanks for the review @dawehner and nice catch.

+++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
@@ -0,0 +1,106 @@
+      /** @var \Drupal\Core\Access\AccessResultInterface|\Drupal\Core\Cache\CacheableDependencyInterface $access_result */
+      $access_result = $this->accessManager->checkNamedRoute('node.add_page', array(), $this->currentUser, TRUE);
+      $element = [
+        '#type' => 'link',
+        '#title' => $this->t('Add content'),
+        '#url' => Url::fromRoute('node.add_page'),
+        '#access' => $access_result->isAllowed(),
+        '#prefix' => ' ',
+      ];
+      CacheableMetadata::createFromObject($access_result)->applyTo($element);

Can someone explain this a bit? Is this what is needed to create a link from code without breaking caching? Is there documentation for this kind of stuff somewhere?

Is this what is needed to create a link from code without breaking caching?

@benjy here is your answer http://wimleers.com/talk-making-drupal-fly-fastest-drupal-ever-near/#/3/5?

I don't see any mention to CacheableMetadata? Also, is there not a documentation page on drupal.org for this?

dawehner is working on allowing to assign an AccessResultInterface object ($access_result in the above) to #access, so that that silly dance is no longer necessary. That'll take the painfulness away.

Documentation is at https://www.drupal.org/developing/api/8/render/arrays/cacheability. This particular example isn't documented there because the painfulness will be removed.

This particular example isn't documented there because the painfulness will be removed.

That's what I was hoping someone would say :)

Verified the patch in #83, the Add content link is not inline on homepage and Add Content link is not displayed on Content page.

Re-rolled didn't apply for 8.2.x.

I tested this on a new installation of 8.2.x and the Add content link is on admin/content however in an already installed site, even rebuilding cache the link doesn't show up. What is the intended behavior on case of upgrading?

Probably unrelated.

As per given patch in comment #94 changes are not reflecting attached screen shot please check.

The patch is making site slogan miss align.

Re-roll patch created.

Most probably this should fix the failed test case.

I reviewed the patch #112 and found that it will only work when a user freshly installs Drupal 8.9.x, It will not work for them who upgrade from the lower version to 8.9.x. So need to write an update hook to import/update that (in patch) nodes configurations.

Updated patch to add the update hook to import content view configurations. It also includes changes recommended by the code sniffer.

Updated patch.

Created a patch to update the node configurations.

Steps:
1). Apply patch.
2). Run drush cache rebuild command.
3). Run drush update db command.

@sja112 Patch #124 looks fine for me. I tested it with fresh drupal installation & with the existing installed drupal site and it works well for both. Code also looks fine for me.

I so thought this was fixed years ago. Nice work!

It looks like the patch does not apply to 9.1.x, so we will need a reroll for that. Thanks!

Quick re-roll and few fixes
- default view still needs work (moved to post update hook and added TODO)
- keep constructor slimmer and string translation is one of deficiencies via trait

Still needs work for upgrade hook and upgrade tests as the changed view primary for listing content

+++ b/core/modules/node/node.post_update.php
@@ -14,3 +14,16 @@ function node_removed_post_updates() {
+  // TODO update 'views.view.content' view.

the todo to fix, views now using some new way to write upgrades

I've updated the patch for TODO item.

Not clear which helper views updates should use now... that's why I changed this to todo in previous patch

+++ b/core/modules/node/node.post_update.php
@@ -14,3 +17,20 @@ function node_removed_post_updates() {
+  $optional_install_path = $module_handler->getModule('node')->getPath() . '/' . InstallStorage::CONFIG_OPTIONAL_DIRECTORY;
...
+  $config_storage->write('views.view.content', $source->read('views.view.content'));

is no-go as upgrade path because user can have the view customized already.
So the right way is load its config and process

@andypost,

I have updated the patch to update the views config. I have loaded the views config and processed it.

Please review.

Looks good so only upgrade path tests left

Mostly some nits

1. +++ b/core/modules/node/node.post_update.php
   @@ -14,3 +14,38 @@ function node_removed_post_updates() {
   +    // Go through all the entity fields on each display and find ones currently using 'date' as the plugin.
   

   c/p left over I'm guessing?

2. +++ b/core/modules/node/src/Plugin/views/area/AdminListingEmpty.php
   @@ -0,0 +1,75 @@
   +        '#prefix' => ' ',
   

   Hmmm. I understand why, but this makes this pretty weird to use without another plugin outputting text before it.

3. +++ b/core/modules/node/tests/src/Functional/NodeAdminTest.php
   @@ -207,6 +207,20 @@ public function testContentAdminPages() {
   +    $this->assertTrue(is_array($link) && empty($link), 'Did not find "Add Content" link in view.');
   

   Why not just assertEmpty()?

4. +++ b/core/modules/node/tests/src/Functional/NodeAdminTest.php
   @@ -207,6 +207,20 @@ public function testContentAdminPages() {
   +    $this->assertTrue(is_array($link) && !empty($link) && count($link) == 1, 'Found "Add Content" link in view.');
   

   why not just assertCount()?

5. +++ b/core/modules/node/src/NodeViewsData.php
   @@ -192,6 +192,14 @@ public function getViewsData() {
   +      'help' => $this->t('Provides a link to the node add overview page for admin.'),
   

   'for admin.' => 'for users with the correct permissions.'
   I don't know, something like that to make it clear what is being checked.

6. +++ b/core/modules/node/config/schema/node.views.schema.yml
   @@ -4,6 +4,10 @@ views.area.node_listing_empty:
   +  label: 'Node link'
   

   This can be a bit more descriptive

7. +++ b/core/modules/node/tests/src/Functional/NodeAdminTest.php
   @@ -207,6 +207,20 @@ public function testContentAdminPages() {
   +    $link = $this->cssSelect('.view-content .views-empty a[href="/node/add"]');
   ...
   +    $link = $this->cssSelect('.view-content .views-empty a[href$="/node/add"]');
   

   The negative and positive assertion don't match selectors, so the negative assertion will always fail now.

@lendude, thanks for the review.

I have updated the patch to fix #135.1, #135.3, #135.6, #135.7 except,

#135.2

this pretty weird to use without another plugin outputting text before it.

#135.4, #135.5 about the use of assertCount and assertEmpty. Because here I am performing '&&' operation I am getting results in boolean that's why I have used assertTrue instead of these methods.

Did not find "Add Content" link in view.
Failed asserting that a boolean is empty.

@sja112
You should try this

Because a[href$="node/add"], It will match any links that end in given format.
I am assuming that's why your test cases is getting failed.

Hi @rahul,

just reroll patch #136, kindly review.

Changes:

-    $link = $this->cssSelect('.view-content .views-empty a[href="/node/add"]');
+    $link = $this->cssSelect('.view-content .views-empty a[href$="/node/add"]');

Thanks,
Shakti

+++ b/core/modules/node/tests/src/Functional/NodeAdminTest.php
@@ -207,6 +207,20 @@ public function testContentAdminPages() {
+    $this->assertTrue(is_array($link) && empty($link), 'Did not find "Add Content" link in view.');
...
+    $this->assertTrue(is_array($link) && !empty($link) && count($link) == 1, 'Found "Add Content" link in view.');

@sja112 Re:#135.4/.5 Yes I see why you need assertTrue if you do the && thing, but why would you do the && thing? We don't care that it is an array, we only care that it's empty. Testing if cssSelect () returns an array is testing the test framework, which is not the scope of this test.
Same goes for the second assertion, we only care about the count being 1, we don't care about any of those other things since they will also lead to assertCount to fail if they are not true for some weird reason.

The && leads to extra complexity which we don't need, hence, we should take them out.

@shaktik why the reroll? It applies fine.

Back to NW for 'Needs upgrade path tests'

After applying patch it's working fine. I have modified the if conditions in NodeAdminTest.php

I modified the below code in NodeAdminTest.php.

// Verify that the 'Add content' link is not available when content has been created.
    $link = $this->cssSelect('.view-content .views-empty a[href="/node/add"]');
+  $this->assertTrue(empty($link), 'Did not find "Add Content" link in view.');

    // Remove all content.
    foreach (\Drupal::entityTypeManager()->getStorage('node')->loadMultiple() as $entity) {
      $entity->delete();
    }

    // Verify that the 'Add content' link is available when no content has been created.
    $this->drupalGet('admin/content');
    $link = $this->cssSelect('.view-content .views-empty a[href="/node/add"]');
+    $this->assertTrue(count($link) == 1, 'Found "Add Content" link in view.');

All the test cases are passed on local which are failing in #140

root@b1dad303faf2:/var/www/html# phpunit --configuration=core --filter testContentAdminPages core/modules/node/tests/src/Functional/NodeAdminTest.php
PHPUnit 8.5.4 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\node\Functional\NodeAdminTest
.                                                                   1 / 1 (100%)

Time: 5.85 minutes, Memory: 8.00 MB

OK (1 test, 86 assertions)

HTML output was generated
http://localhost:81/sites/simpletest/browser_output/Drupal_Tests_node_Functional_NodeAdminTest-1-52944308.html
http://localhost:81/sites/simpletest/browser_output/Drupal_Tests_node_Functional_NodeAdminTest-2-52944308.html

Patch for test case failure.

Updating the correct comment id.
Patch for test case failure.

Fixed test, Please review.

@vsujeetkumar Please review this patch i have changed some test case code.

Still needs the upgrade path test.....

This is actually not a bug, but task and also a blocker for #2453733: Create a general 'add new {entity_type}' area plugin and fix the empty behaviour for nodes, comments and files to match UX guidelines (or indirect) #2454311: Add "Add a file" link to empty table text at admin/content/files

Core has no support for this kind of action links in views, so it's regression from D7 era (which is still supported)

If we call it regression then it's bug, if feature parity then task...

Anyway it needs to update summary and current state of the patch

@Lendude Sorry for the #153 the patch is not tested "Add content" link on table I apologies for the same and also fixed the test case for "Add content" link inside the table.

I don't see how this is a blocker for #2453733: Create a general 'add new {entity_type}' area plugin and fix the empty behaviour for nodes, comments and files to match UX guidelines?

Why would we create a node specific handler first? Why not create the generic handler first and then implement that for node? If we do the node specific one first, we are going to have to do the whole @deprecated dance with it to replace it with the generic handler once that lands.

I would mark this 'duplicate' and put our efforts into #2453733: Create a general 'add new {entity_type}' area plugin and fix the empty behaviour for nodes, comments and files to match UX guidelines

Yes, it makes more sense to create the generic plugin valid for every entity (which means this issue is a duplicate of the other one.)
It would make sense to have two issues if the classes implementing the handlers were different and not extending the same base class, but I think this is not the case.

I don't also think that implementing the handler for nodes is faster than creating the handler for every entity. If it were faster, I could understand the decision to implement the handler for nodes first.

Seems this should be postponed on #2453733: Create a general 'add new {entity_type}' area plugin and fix the empty behaviour for nodes, comments and files to match UX guidelines