Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Needs review » Postponed

All of the feedback I've been getting for #1894972: Provide a bulk form for user operations is valid here too, and the code is almost identical. I might as well wait until that one is polished.

klonos’s picture

klonos’s picture

tim.plunkett’s picture

This has nothing to do with that, its for /admin/content

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
48.5 KB

Resurrecting this.

This will fail LocaleContentTest, we don't have any filter handlers for node.translation

tim.plunkett’s picture

Title: Provide a bulk form for node operations » Convert admin/content to a View, keep a non-views fallback with no bulk operations

Retitling.

tim.plunkett’s picture

Okay, this should be done now.

tim.plunkett’s picture

FileSize
482 bytes
49.21 KB

Missed a line.

tim.plunkett’s picture

Issue tags: +VDC

Tagging.

dawehner’s picture

+++ b/core/modules/node/lib/Drupal/node/Form/DeleteMultiple.phpundefined
@@ -0,0 +1,130 @@
+   * The entity manager.

... This constructs an object ...

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.phpundefined
@@ -0,0 +1,77 @@
+    }, module_invoke_all('node_operations'));
...
+      $operations = module_invoke_all('node_operations');

Can we use \Drupal::moduleHandler()?

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.phpundefined
@@ -0,0 +1,77 @@
+      foreach (array_intersect_key($this->view->result, $selected) as $node) {
+        $node = $this->get_entity($node);

I guess name it $row would clear it a bit better.

+++ b/core/modules/node/tests/modules/node_test/lib/Drupal/node_test/NodeTestStorageController.phpundefined
@@ -0,0 +1,25 @@
+    // Allow test nodes to specify their created time.

Afaik it's about ->changed, but it feels a bug of the NodeStorageController that you can't set it manually at all.

+++ b/core/modules/node/tests/modules/node_test_views/test_views/views.view.test_node_bulk_form.ymlundefined
@@ -0,0 +1,45 @@
+disabled: '0'

should be status 1

tim.plunkett’s picture

ModuleHandler cannot be injected right now, that's cropped up in several issues, I guess I should open an issue about that.

Still, we can use Drupal::moduleHandler(), for easier conversion later.

About $node->changed, this is weird:
In #722688: Allow programmatic setting of node->changed, this was fixed by wrapping it in a conditional.
Then #322759: Allow hook_node_presave() to alter other node fields undid this, with the reasoning that someone could implement hook_node_pre_save() to override this behavior.

That's exactly the approach I've taken here, except using a controller and not a hook.

tim.plunkett’s picture

dawehner’s picture

FileSize
10.14 KB
18.44 KB
11.04 KB

Commented on that issue :)

Posting some screenshots, to make people more comfortable with the patch.

Before:

before.png

After:

after.png
I guess we maybe need some follow up to improve the visual clutter on this page.

After without views:

after_no_views.png

tim.plunkett’s picture

FileSize
49.28 KB
482 bytes

Adjusted the parent tab to be a default as well.

klonos’s picture

In the "after" screenshot there are three "apply" buttons. I realize that the two are duplicate of the same thing, but still should we do something about it?

The button in the filter differs in style from the other two plus it is inline with the status/type filter drop-down menus while the other one wraps under the "With selection". It would perhaps be better to have that one have a label like "filter" instead of "apply".

Also, the "With selection" label and the "do something with selected content" feels redundant. How about we change the label to something like "Action:" or reduce the drop-down entries to be only verbs like "unpublish", "delete" and so on instead?

irinaz’s picture

I applied the patch to a standard Drupal 8 install and I cannot see filters. What do I need to do activate views?
Thanks, Irina

irinaz’s picture

It would be great to have an option to filter by title also
content view
I am adding a yml file with the view code

tim.plunkett’s picture

You would have to reinstall while the patch was applied to see it.

The current D7 page has no title filter, and this issue is a straight port.

We can discuss improvements to this view and all admin views in a follow-up once the basic port is done.

tim.plunkett’s picture

FileSize
655 bytes
49.53 KB

Okay, I changed the top button to say "Filter".

tim.plunkett’s picture

#20: admin-content-1895160-20.patch queued for re-testing.

dawehner’s picture

Two small nitpicks. This is looking great.

+++ b/core/modules/node/lib/Drupal/node/Form/DeleteMultiple.phpundefined
@@ -0,0 +1,130 @@
+  /**
+   * The entity manager.
+   *
+   * @var \Drupal\Core\Entity\EntityManager
+   */
...
+   * @param \Drupal\Core\Entity\EntityManager $manager
+   *   The entity manager.
...
+    $this->manager = $manager;
...
+      $this->manager->getStorageController('node')->delete($this->nodes);

Let's store the storage controller. The manager will be container aware or sort of container aware, so let's avoid where it's possible.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeAdminTest.phpundefined
@@ -39,38 +46,42 @@ function setUp() {
+    \Drupal::state()->set('node_test.storage_controller', TRUE);

Should we use $this->container->get('state') instead?

+++ b/core/modules/node/node.admin.incundefined
@@ -308,8 +124,8 @@ function node_mass_update($nodes, $updates) {
+ * @param $node
+ *   A node to update.

@@ -318,8 +134,7 @@ function node_mass_update($nodes, $updates) {
+function _node_mass_update_helper($node, $updates) {

Can we typehint if we change the parameter?

dawehner’s picture

Before I forget: my manually testing showed that #1116326: Support admin overlay in exposed forms does also exist on this issue.

xjm’s picture

Status: Needs review » Needs work

NW for #22. About to test manually. :) simplytest.me choked for some reason.

xjm’s picture

The no-view fallback seems to work great. I didn't test it with generated content yet. The view works great too. :) Code review later.

xjm’s picture

Yeah, I noticed #1116326: Support admin overlay in exposed forms while I was recording my videos.

irinaz’s picture

Tim, thanks, got your point. I will continue testing and hope that title filter will make it into D8 :)
Thanks, Irina

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.65 KB
49.98 KB

This addresses #22, and also contains some further clean-ups based on how other code has gone in D8 recently.

Status: Needs review » Needs work

The last submitted patch, node-1895160-28.patch, failed testing.

tim.plunkett’s picture

EntityBCDecorator--

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
51.38 KB

Thanks to Berdir for guidance on the fix.

ParisLiakos’s picture

how about changing
Status: yes/no
to
Published: yes/no

+++ b/core/modules/node/lib/Drupal/node/Form/DeleteMultiple.phpundefined
@@ -0,0 +1,136 @@
+      drupal_goto($this->getCancelPath());

wondering if we should use RedirectResponse here

tim.plunkett’s picture

_form does not allow returning other responses yet.

Also I might be mistaken, but I think the current listing does Status, so that's what we're doing.

ParisLiakos’s picture

yes it does status, but like:
Status: published/not published

Status: yes/no makes no sense to me

besides that patch seems ready to fly!

tim.plunkett’s picture

pcambra’s picture

Is there any reason to create our own views form for the bulk operation instead of using the Action module? I think we could just add the "Action: Bulk update" field and configure the actions in the view.

Edit: dawehner pointed me to #1846172: Replace the actions API so I guess that's going to change a lot how we face the actions here.

tim.plunkett’s picture

Issue tags: -VDC

#31: node-1895160-31.patch queued for re-testing.

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

The last submitted patch, node-1895160-31.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
52.08 KB

Rerolled. We have an issue with overriding existing callbacks with views right now, I will open a new issue.

Status: Needs review » Needs work

The last submitted patch, admin-content-1895160-39.patch, failed testing.

tim.plunkett’s picture

FileSize
9.12 KB
53.02 KB

This contains #2011006: Default local tasks provided by Views are broken, but I want to see if that's the only failure.

Rerolled on top of the actions commit.

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, node-1895160-41.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.37 KB
53.1 KB

Some stragglers from the merge.

xjm’s picture

Priority: Normal » Major
tim.plunkett’s picture

FileSize
45.25 KB

This no longer has any blockers.
No changes, just a reroll.

msonnabaum’s picture

Status: Needs review » Reviewed & tested by the community

Looks quite good to me.

ParisLiakos’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
95.27 KB

Somethings not quite right here... some missing handlers... both with standard and minimal + views + views_ui

Content__Content____Site-Install.png

Also if you goto to admin/structure/views and then click on the path for the content view you get a page not found.

dawehner’s picture

These two missing handlers are the translation link and the langcode filter one. As written in #1966424: Change notice: Allow Views handlers to be optional we needed some kind
of way to tell views that a certain handler is optional (so it should not fail if it does not exist). We don't see a better way to extend an existing view depending whether a certain module is enabled or not.

Beside from the motivation I think we should give a better message on the view page (if not even remove the link and just display some text) if it's optional and the link does not exist, though it seems to be sort of out of scope of this issue?

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Title: Convert admin/content to a View, keep a non-views fallback with no bulk operations » Change notice: Convert admin/content to a View, keep a non-views fallback with no bulk operations
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 32697f2 and pushed to 8.x. Thanks!

Let's get stuff done... those are pre-existing issues.

dawehner’s picture

Yeaaaaaaaaaaaaaaah!!!!!!

andypost’s picture

FileSize
15.21 KB

There's no border at left side of the table, needs follow-up
empty_content.png

dawehner’s picture

Pol’s picture

FYI: I just tested D8 with simplytest.me, standard profile installation and the view is not working because of #49.

xjm’s picture

@Pol, could you give a more specific explanation for "not working"?

Pol’s picture

I just tested a few minutes ago on simplytest.me, when I try to access to /admin/content/node, it says:

The requested page "/admin/content/node" could not be found.

tim.plunkett’s picture

xjm’s picture

We definitely need to put the removal of admin/content/node in the change notice since people keep asking about it.

xjm’s picture

Issue summary: View changes

.

klonos’s picture

I tried disabling views_ui.module first and then views.module in order to test the fallback and got this while on the modules page:

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "view.content.page_1" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 126 of /home/sc98e1faf7be689e/www/core/lib/Drupal/Core/Routing/RouteProvider.php).

This error pops up when trying to get to /admin/modules. The page shows up fine in overlay (/#overlay=admin/modules).

Different issue?

amateescu’s picture

klonos’s picture

Thanx for the link to the issue Andrei!

So, has anybody actually tested the fallback to the non-views no bulk operations page or should we revisit this once that other issue is resolved?

amateescu’s picture

You can easily test it by disabling the shortcut module :)

klonos’s picture

Right. I would have never guessed that since I had absolutely no idea that shortcut is used for that view.

Anyways, I also want to say that in #2020167: Add a name and email search field to the admin/people view we added a field to search on user name and e-mail. Should we do the same here and search title and body? Did anybody file an issue for that?

tim.plunkett’s picture

klonos’s picture

Thanx Tim.

ibullock’s picture

Assigned: tim.plunkett » ibullock

going to write the API change notification.

Edit: Wrong chrome tab, wrong issue. :S

ibullock’s picture

Assigned: ibullock » Unassigned
pcambra’s picture

Wim Leers’s picture

The problem in #62 still has not yet been fixed over at #2021779: Decouple shortcuts from menu links.

I found the above through another regression: #2043403: admin/content View does not have "new"/"update" indicators.

Finally: this was committed on June 11, today is July 16, still no change notice.

claudiu.cristea’s picture

webwarrior’s picture

Assigned: Unassigned » webwarrior
Status: Active » Needs review

Proposed change notification:

Summary:
Various admin pages and core pages, as listed below, that lists entities was converted to use Views. As a fallback, if Views is not installed/enabled, a non-view will be used, but will not have any bulk operations available.

This change notification incorporates the following issues, as requested:
#1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations
#1851086: Replace admin/people with a View
#1806334: Replace the node listing at /node with a view

Pages changed:

  • admin/content
    admin/content/node was removed.

    In core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.php, class NodeBulkForm added.

    In core/modules/node/node.admin.inc removed functions:
    node_filters
    node_build_filter_query
    node_filter_form
    node_filter_form_submit
    node_admin_content
    node_admin_nodes
    node_admin_nodes_submit
    node_multiple_delete_confirm
    node_multiple_delete_confirm_submit

    Functions with changed parameters:
    node_mass_update
    Before:
    function node_mass_update($nodes, $updates, $langcode = NULL)
    After:
    function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load = FALSE)

  • admin/people
    admin/people/list

    API changes:
    In core/modules/user/user.admin.inc :

    router admin/people/people removed

    removed functions:
    user_admin
    user_filter_form
    user_filter_form_submit
    user_admin_account_submit
    user_admin_account_validate

    user_admin_account function changed to remove bulk operations.

  • Convert the main /node listing to a View

    API changes
    In core/modules/node/lib/Drupal/node/Plugin/views/area/ListingEmpty.php, class ListingEmpty added.

    Router /node removed from core/modules/node/node.module

    Removed functions:
    node_page_default

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Let's add #1823450: [Meta] Convert core listings to Views in related issues. Change notice looks good.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies

jibran’s picture

Issue tags: -Needs reroll

It was RTBC for change notice patch was already committed in #52. Sorry for the confusion. @webwarrior please go ahead an create a change notice and suggestion in #75 to it.

webwarrior’s picture

Sorry, I'm a bit confused.
What do you mean "Patch no longer applies" in #76? Do I need to change something in notice at #74?
Or can I go ahead and create a change notice?

tim.plunkett’s picture

@webwarrior please go ahead and create the change notice.

xjm’s picture

Haha @webwarrior, you are a victim of @alexpott's new patch-checking automation which missed the mark in this case. Disregard #76. I'll review the change notice in a bit. :)

xjm’s picture

Assigned: webwarrior » xjm
Status: Needs work » Needs review
catch’s picture

webwarrior’s picture

Change notification added. Should we remove the tag?

https://drupal.org/node/2084727

star-szr’s picture

Title: Change notice: Convert admin/content to a View, keep a non-views fallback with no bulk operations » Convert admin/content to a View, keep a non-views fallback with no bulk operations
Assigned: xjm » Unassigned
Status: Needs review » Fixed
Issue tags: -Needs change record

Yes, thanks for working on the change notification @webwarrior!

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

Anonymous’s picture

Issue summary: View changes

Adding a Related issues section.

xjm’s picture

Component: node.module » node system
Issue summary: View changes
klonos’s picture