Move permission "view any unpublished content" from Content Moderation to Node

Just FYI: there are some very minor code style violations in the comments (space after the "//" and end the comment in a full-stop (period)).

Moving this feature request to Drupal 7. For Drupal 5, this feature can be set up with the View Unpublished module.

--Andrew

is this possible in d6?

This would be a very nice feature to have, as I couldn't believe it wasn't in D6 core.
Someone made a D6 port of a now unmaintained D5 module here #304173: Make it to work in Drupal 6.

I would really like to see this in D7 :x

The only available permission now is "View own unpublished content".
This could be expanded with "View all unpublshed content"
more fine grained control could allow for "View all unpublished nodes"

Any input on this ?

sub

I was really surprised this wasn't available in D6.
Really hope it makes it way to D7...

As this doesn't fit into the feature exceptions, it's going to have to wait until D8.

This would be extremely useful.

The view_unpublished module is good if you only need basic functionality.
But if you need to be able to view/edit unpublished revisions or use file
attachments on unpublished nodes or anything more than the most basic use
you run into problems.

Because node_access checks node status, you run into a lot of problems very quickly
if you are trying to get around this outside of core.

This would definitely be something that would be good to get into drupal 7.
It would certainly make it worth waiting for.

err.. correct me if I'm wrong, bug D7 already has 'view own unpublished content'
maybe that's not what is being requested, but it seems pretty much similar

This issue is also for "view any unpublished content" or something similar.

Thanks for taking the time to report this issue.

However, I think that the most common use-cases are already covered by #452538: Enable Node Grants for Unpublished Nodes and thus marking as duplicate.

Reopening this issue as #452538: Enable Node Grants for Unpublished Nodes didn't solve it

That issue took care of node grants for unpublished and view own unpublished so
this issue is for "view any unpublished" or possibly "view NODE-TYPE unpublished"

Subscribing.... BIG +1 for this feature!

Subscribe

Subscribing.

This is a new feature. Far too late for D7. See #927792: Please explain what is still considered for D7

+1

+1.
Such a shame this did not get into D7 - the various threads started early enough. I have my own simple patches for 'view unpublished TYPE content' for D6 and D7 if anyone is interested.

Jonathan

@jonathan1055 / #21: interesting, could you upload them as a zip file here in this thread?

Yes certainly. They are not zipped, no need really. D6 created against 6.20 and D7 against 7.0, not against the latest dev, but they should both apply ok.

Changing status

For those searching, I've posted a D7 and new D6 version of the View Unpublished module on the project page: http://drupal.org/project/view_unpublished

I try to add views unpublished support, please see http://drupal.org/node/773784#comment-4680180

tagging

Patch that adds a 'view unpublished content' permission, makes the necessary changes to the access checks and adds a small test for the new permission.

Wow. That's great. This is exactly what I'm looking for!

I installed your patch, but I didn't get it to work.

I'm trying to allow users to see their own unpublished nodes within views. They can see their nodes by accessing the node's page directly, but it does not show up in any views. It seems like the permission 'View own unpublished content' should take care of this, but unpublished nodes do not show up in views. Are talking about the same thing?

I'm trying to follow along what you did here in this patch. I see you added a 'View unpublished content' permission to the node section, but that seems odd. It is not clear what is meant by that and how it relates to 'View own unpublished content'. I'm not sure exactly what the intention is. Maybe it should read "'View own unpublished content in views'?

Hmmmm....

@#29 not fully reviewed, but first chunk seems to change indentation unnecessary
also, permission's name seems misleading (see #13 for a more appropriate suggestion)

Reroll that renames the permission to 'view any unpublished content' and fixes the indentation issue.

Note that the recent block from node module calls node_get_recent() which does the same thing with these permissions.

Hi Eric_A
Could you explain what you mean, I don't understand. Maybe I'm missing something, sorry.
Jonathan

Hmm, I would really like this permission seperate for each Content Type and not globally for all content types in D7.

+1 for #35

Hmm, I would really like this permission seperate for each Content Type and not globally for all content types in D7.

If you mean you want drupal core permission changes in drupal 7 it probably won't happen.
Changes like that generally won't happen until the next major version.

If you mean you want it in drupal 8, unlike how the view own unpublished works in drupal 7, then I agree.

Maybe this issue could be done to get the view any unpublished nodes permission in and then a follow up issue could be done for both the view own unpublished and the view any unpublished, to move them to content type specific perms.

+1 for #37
nevertheless, permission table gets bigger and bigger,
it would be nice to have configurable options to enable such permissions "for each" content type
but as rooby said @#37, lets start by having them globally at least

We just got bit by this too. Very strange that this functionality was removed from administer content without a suitable replacement ever being offered. Lets please try to get this in soon (and hopefully backport to D7).

Can we get a re-roll of this patch? It no longer applies.

I've just run into an issue with unpublished content on the admin/content page. I would think any kind of editorial workflow would allow users to edit other user's unpublished content. Currently this requires a contributed module to do. If D8 were to add in extra permissions however this would make things a whole lot simpler.

The way I see it there are three options ranging from simplest to most complex:

1. Add in a 'View all unpublished content' permission
2. Add in a 'View all unpublished $node->type' permission for extra granularity based on content type
3. Extend #2 by adding in a complimentary 'View own unpublished $node->type' permission rather than the standard 'view own unpublished.

Assigning to myself to reroll a patch based on #2/#3 in the first instance. If a week goes by without a patch feel free to unassign!

I like his proposal and I think it plays nicely with the issue mentioned here #26552: Allow users with access to unpublished nodes to create unpublished books.

@joel_osc:
In relation to books and unpublished access, this issue is also relevant: #520786: menu_tree_check_access: only filter by status = 1 for non content admins
However I assume menu_tree_check_access() might be changing due to all the menu changes in D8.

[EDIT] Actually I see now that they are partly duplicate issues, so I have mentioned it in the unpublished books issue.

OK - here's a little patch that addresses #1 in comment 40 so people don't think I've forgotten this issue.

I'm going to extend further and add in view unpublished per content type shortly.

Ok - here's a more granular patch with view unpublished permissions per content type.

#45: 273595-45-view_unpublished_per_type.patch queued for re-testing.

#45: 273595-45-view_unpublished_per_type.patch queued for re-testing.

Rerolled patch for latest D8 HEAD

Marked #1388674: Add "View unpublished {node_type}" permission as a duplicate of this issue.

I'm definitely liking this so far. Especially happy with the more granular options provided as well. Great work folks. Will allow others to continue testing before RTBC.

#50 no longer applies. This is a reroll of #50.

Subscribing. This is looking awesome, is there any chance some form of this will also be implemented in d7?

Ishmayl, I kind of doubt it, though if this gets into D8, we can certainly tag it for backport and see. Doesn't look like it'd be too hard to backport. Would certainly be nice. ;-)

If you've tested this - the best and fastest way to get it into D8 would be to mark as RTBC. This way the maintainers will be made aware of it and inform us whether or not it's likely to make it into this release.

#53: 273595-53-unpublished-node-access.patch queued for re-testing.

#53 no longer applies. This is a new re-roll of #50

Unassigning from me so anyone may review.

Is this essentially adding what the contrib module View Unpublished does?

Functionality looks pretty similar actually but baking it into core will fix what I consider to be an anti-pattern in the way Drupal handles access for unpublished content.

59: 273595-59-unpublished-node-access.patch queued for re-testing.

Completely agree, just wanted to make sure I was on the same page after reading thru the issue history.

Updating patch with re-roll. Please review.

Patch #68 stil applies fine.

I did the following manual tests:

1. Created a node/1 article with user 1 as author, left it unpublished.
2. Assigned the permission 'View any unpublished content' to authenticated users.
3. Visited the node/1 page as an authenticated normal user, I could see the node.
4. Assigned the permission 'View own unpublished content' to authenticated users, removed 'View any unpublished content' from authenticated users.
5. Changed the author to the normal user of node/1
6. Visited the node/1 page as the authenticated normal user, I could see the node.

So the permission works, at least from an enduser perspective. Setting this to RTBC, unless someone thinks of other things that need testing =)

This all looks good to me.

Assigning to @agentrickard as node access maintainer - it'd be nice to get a +1 from a node access maintainer.

lol

Two points, one of which can likely be ignored, the other is a point of debate:

1) I don't like that part of the access check is in the controller and part in node_node_access(). This seems inconsistent to me. The implicit design is that "view any" retuning TRUE cannot be overridden by other modules the way that NODE_ACCESS_ALLOW can when sent from node_node_access().

This applies to the current "view own unpublished" as well. I don't see why that should be in the checkAccess() method.

If this is intentional, we should document it as such.

2) This has no effect on list queries, which I suspect many people will see as a bug, but that can possibly be addressed in a followup, or ignored on the grounds that we don't track node access for unpublished content.

Ref. comment #72:

"2) This has no effect on list queries, which I suspect many people will see as a bug, but that can possibly be addressed in a followup, or ignored on the grounds that we don't track node access for unpublished content."

This can has been kicked down the road since the start of Drupal. This very issue was filed in 2008, but we have maaaaany such issues long before that, in various contrib modules as well as for core.

I think it is high time it is dealt with in a fundamental way that does not make Drupal stand out any worse than other solutions.

IMO, it is never ok to architectually risk exposing data to roles for which the system explicitly prohibits access to the content in question, just because it is a "list" or whatever is the reason why some page renders does not check the access control. That is neither good design, nor what is expected of a system that provides access control in the first place.

Now that Views is in core in D8, this must be dealt with by core itself.

Per: https://www.drupal.org/core/beta-changes
We're now in beta. New features can go into 8.1.x. We can unpostpone this when that branch is opened for development.

@Leeteq re: #75, I agree with you, but it's a bit out of scope to discuss on this issue, I think. To that point, though, I would agree that filtering listings also needs to be a part of the permission's behavior when we reroll this for 8.1.x.

Hi, I dug this up from the issue queue because I need the feature and quickly refactored (core has changed so much that it was a lot more than reroll but the logic is the same) the old patch to current D8.

In case this version does not apply to 8.0.1 (didn't try it out yet), I will make a such version for my own needs, just in case anyone else needs it too.

For the record, patch #77 applies and works on 8.0.1.

I have hard time figuring out why that one test fails but will look into it more. Ideas welcome, though.

It would be indeed good to get this done quickly because there is still some time to get into 8.1.0. I haven't unfortunately had time to figure out the tests.

I had a fresh look at the failing test but still no understanding came my way.

The failing test is Drupal\node\Tests\NodeAccessBaseTableTest, and it fails when checking if a user has access to an another user's private node. I have hard time understanding why because I haven't had time to figure out what actually this "private" node thing is.

The failure seems to be caused by the patch adding case 'view' in node_node_access(). I investigated implementing the unpublished check in NodeAccessControlHandler::checkAccess but I seem to be running into some other test failures with that approach.

The problem is that it didn't return neutral in case the node is published.

Thanks @swentel - Patch looks great to me, +1 for RTBC

OK while the patch works as advertised, I feel we should patch what views does when it comes to filtering out unpublished content, since views is in core, and all.

Something like what view_unpublished does for example:
http://cgit.drupalcode.org/view_unpublished/tree/?h=8.x-1.x

Else people are going to use these permissions, and will have to use view_unpublished module so that views actually respect these new permissions.

I'm not familiar with view_unpublished, so I have to confirm. Manuel, do you mean that there should be a filter in Views to the effect "select only published content, unless the user has permission to view unpublished content"? If I got this right, I agree that it is silly. I think the issue concerns cases where fields are used, and can be circumvented by using content display modes, because then access rights will be checked using the code in this patch, right? Still, using fields is very common so this note does not make the issue go away, although less of an impediment in my opinion.

However, this patch has been in the works for almost 8 years, and I think we now have a realistic possibility to get this in for 8.1.0. Having the feature at all is important, I don't see why we couldn't just fix Views support in a followup issue? Could it qualify for a patch release if this one makes 8.1.0? If yes, I think we should go for it, and implement view_unpublished in Core in a separate issue. (Or at least change the 8.x implementation of view_unpublished so that it utilises the permissions defined here, as it is a Views plugin after all, does it need to be in Core?)

This considered, I am going to tag this issue as needing followup and RTBC, feel free to push it back if I wasn't convincing enough. :)

This won't work; there's a reason 'view' doesn't exist in node_node_access: when trying to list nodes (via views for example) you need to know accurately (for paging purposes) which nodes are visible and which aren't. Think of it, you request nodes 30-39 but during access control turns out the user doesn't have access to any of them, is the 4th page then going to be empty? This was raised in #72 although @agentrickard said it could be ignored -- ignored why exactly?

@chx, sadly you're very right. (Previously I did not fully understand what @agentrickard meant in #72.)

But what does it mean? Should we actually, in contrast to what @agentrickard said in #72, implement this in NodeAccessControlHandler::checkAccess?

Edit: But checkAccess either is not called until node is being viewed, right? So where does this need to step in?

Ok, I managed to put some more time into this. What I did is as follows:

I modified the "Published status or admin user" (status_extra) filter plugin so that it checks for permission "view any unpublished content" (and renamed it more appropriately to "Published status or unpublished access"). Makes this almost work for Views, but per-node-type unpublished access checking is still needed.

I also tested for the problem @chx brought up. Either listing content or fields in Views with this feature enabled, with 20+ nodes of which 10 randomly unpublished I do not get empty pages in pager. Maybe I didn't think this far enough, but it seems to me that access checking works correctly in this use case as well.

As a slightly off topic point, I find it a bit odd user's permission to access individual nodes seems not to be checked if there is no status = 1 filtering in the view, meaning anyone can view unpublished nodes if Views doesn't filter them out. I guess this is by design?

View Unpublished Content for Specific User Types
May need to download Modules:

(note) Permission Settings: (Need Dashboard to view and Admin )

Dashboards:

View the Administrative dashboard (check)

Node:

View own unpublished content (check)

(view name) Create New Content (check)
(view name) Edit own content (check)
(view name) Edit any content (check)
(view name) Delete own content (check)
(view name) Delete any content (check)

Override node options:

Override (view name) published option (check)

System:

(note) will only be able to see publishing edit with the admin theme
View the administration theme (check)

(Module add) View Unpublished

View any unpublished content (check)
(view name) View any unpublished content

Rerolling...

Was just a simple conflict on node_views_query_substitutions().

1. +++ b/core/modules/node/node.module
   @@ -907,6 +907,14 @@ function node_node_access(NodeInterface $node, $op, $account) {
   +        return AccessResult::allowedIfHasPermissions($account, array('view any unpublished content', 'view unpublished ' . $type . ' content'), 'OR');
   

   Where adding or updating arrays should we use the short syntax []?

2. +++ b/core/modules/node/src/Plugin/views/filter/Status.php
   @@ -22,7 +22,8 @@ public function canExpose() { return FALSE; }
   +    // @todo: Add per-node-type unpublished checks.
   

   Is this out of scope for this issue?

Regarding #95:

1. I fail to understand what you mean, can you please elaborate? We're not adding or updating array there?
2. We add possibility to view unpublished nodes when the user has permission to view any unpublished, but even if user had permission to view unpublished particular content, it would not be shown, which is kind of broken. But now that I think of it, isn't the whole idea wrong? If the status filter is supposed to filter published nodes, why are we allowing it to show unpublished nodes? I suggest scrapping the whole part of this patch dealing with this here and leave it for another issue to a) decide what is wanted and b) implement the logic as wanted.

Addressing #95.1

This won't be ready in time for 8.2.0 because beta phase is starting any time now. But I updated issue summary to make it easier to review.

The status filter question still needs input, otherwise this looks good?

I've been in the situation of needing this permission and one of the solutions that came to my mind was enabling the content moderation module (now in core as experimental) and use the permission.

I think enabling the module just for that is an overkill so I suggest that the permission is moved from one module to another because we can't have the same permission in two modules.

If the module is rejected as part of core this issue will go back to the state it is now.

+1 @rodrigoaguilera makes sense - let's work on this in Dublin !

I think moving this out of Content Moderation makes sense. Although in the grand scheme I'm not sure node is the best place for it.

status (the field that stores the published state) is an entity key on node, so maybe we add the permissions to core, and only use them if the entity type has the status entity key.

Eventually I'd like to add the status field to more content entities, imagine a world where you could have unpublished blocks, or unpublished taxonomy terms. Putting this in core rather than node will mean it's "View any unpublished content entities" rather than "View any unpublished nodes".

Perhaps then we should add permissions per content entity as well.

yes, or even per bundle?

Should this be a core thing, just loop through all content entities with the status entity key and add a "View any unpublished $entity_type->label()" permission?

@timmillwood even better!

I'm wondering if core can have permissions like in a /core/core.permission.yml file
Maybe the permission should go in system.

So the permission exist only for entity types with status property?

It would be great if we can have a global permission and also granular per bundle.

So I think the permissions should be (in order of access granted):

1. View any unpublished content entity
2. View any unpublished [entity] - one per entity that has a status property
3. View any unpublished [entity bundle] - one per entity bundle for entities that have a status property

It looks like the good folks on Entity API are trying to do just this and more:
#2801031: Provide a generic entity access handler and permissions

And according to Bojan's tweet, the plan is to move that into core:
https://twitter.com/bojan_zivanovic/status/779288985197830144

I had a quick look on the Entity API issue, not claiming I've got a good understanding of how that feature would work but sounds great to me, at least that would be the right place for this.

And if I understand it correctly, it's actually quite far, so if there is work being done on this in Dublin on Friday, I'm in.

Maybe this module can be solution for some of the use cases
https://www.drupal.org/project/access_unpublished

I used content moderation with some module which implements hook_node_access_records() and hook_node_grants() for controlling access to node when they are published. Default behavior apply on nodes not yet published (in a draft mode, or any other state which is not the published state).

We can then view these nodes unpublished, using their url. But if we create a view to make a dashboard listing nodes in different status, then nodes not yet published are filtered because of hook_node_access_records implemented.

We can eventually use the module views_unpublished which take care of this specific use case for unpublished node (content type by content type), by implementing hook_node_access_records for unpublished nodes only.

I wonder if we could support this use case in this issue, by implementing hook_node_access_records and hook_node_grants for controlling access to node instead of using hook_node_access only ? Or is the right way is to use some contrib module as View unpublished if needed ?

Edit: I just saw that this was mentioned in #72. But what i don't uderstand is why using hook_node_access only to control access to unpublished nodes whereas hook_node_access_records() and hook_node_grants() take care of this (control access node by node) + views listing.

Sorry. I just realize. I was lost in my use case. I answer myself.
I didn't see that if we manage access to these unpublished nodes with hook_node_access_records() then we must do the same for published node, which is not by default and a bit overkilled if not needed really.

From #102 we kind of got derailed here. This issue is about letting editors view content that contributors created, so belongs in node. If we have plans to extend it to other content entities that should be a separate issue in my book.

What about getting #100 first then improve it by adding permissions for entity types/bundles in a follow-up? #BabySteps

How about just getting the permissions in, and deal with other issues (Views status filtering?) in either a follow-up or Content Moderation? I think majority of users will just need the generic permission, and the ones who need more granularity might want to use some more advanced tools anyways?

Could someone check reroll for current core? Did we just miss 8.3.0? :( The 9th birthday of this issue is approaching...

Checked with 8.4.x - does not require re-roll.

It seems that /core/modules/node/src/Tests/NodeAccessTest.php was moved to /core/modules/node/tests/src/Kernel/NodeAccessTest.php

So we would need to reroll #97

First try rerolling #97

Let's go with a second try!

I propose RTBC. The per node type permissions are not honored by the status filter but I think it is not a marjor issue and can be dealt with further on.

Added a related issue #2853512: View unpublished could be deprecated ? , as a possible follow up.

I propose RTBC. The per node type permissions are not honored by the status filter but I think it is not a marjor issue and can be dealt with further on.

Unfortunately, we cannot commit such feature regressions to minor releases. In order for this feature request to go forward, it needs to not regress any existing stable functionality.

@xjm, what is the regression here?

Rerolled #123 to the 8.4.x because it didn't apply anymore. Running the tests again.

@xjm I've just read through this issue summary and don't believe that #125 is noting a regression, but rather a place where future work would be welcome/needed. #72.2 confirms that Views' "status filter" does not check access for unpublished content, since access grants aren't tracked.

+++ b/core/modules/node/src/Plugin/views/filter/Status.php
@@ -22,7 +22,8 @@ public function canExpose() { return FALSE; }
-    $this->query->addWhereExpression($this->options['group'], "$table.status = 1 OR ($table.uid = ***CURRENT_USER*** AND ***CURRENT_USER*** <> 0 AND ***VIEW_OWN_UNPUBLISHED_NODES*** = 1) OR ***BYPASS_NODE_ACCESS*** = 1");
+    // @todo: Add per-node-type unpublished checks.
+    $this->query->addWhereExpression($this->options['group'], "$table.status = 1 OR ($table.uid = ***CURRENT_USER*** AND ***CURRENT_USER*** <> 0 AND ***VIEW_OWN_UNPUBLISHED_NODES*** = 1) OR ***VIEW_ANY_UNPUBLISHED_NODE*** = 1 OR ***BYPASS_NODE_ACCESS*** = 1");

As per above this todo is not yet complete. However it does not point to a followup issue either. We don't add @todo items without followup issues and linking the followup issue. I would personally consider it a bug that we allow viewing unpublished nodes in Views to people who can view any unpublished nodes, but we don't allow viewing unpublished Articles for people who can only view unpublished Articles. While this is "new" functionality, in terms of views, it is wrapped under this existing views handler where we introduce the bug with this patch (ie. the handler will not do what its title and description says it does).

+++ b/core/modules/node/src/NodeViewsData.php
@@ -40,12 +40,12 @@ public function getViewsData() {
     $data['node_field_data']['status_extra'] = [
-      'title' => $this->t('Published status or admin user'),
+      'title' => $this->t('Published status or unpublished access'),
       'help' => $this->t('Filters out unpublished content if the current user cannot view it.'),
       'filter' => [
         'field' => 'status',
         'id' => 'node_status',
-        'label' => $this->t('Published status or admin user'),
+        'label' => $this->t('Published status or unpublished access'),
       ],

Hello,

here's the patch for 8.4.x with the modifications about views stripped out as, if i got it well, the views changes should be addressed somewhere else? :)

thanks!

And here's the patch for 8.5.x.
Views modifications stripped out as the views bits need more work (and most likely a new thread).

cheers

This patch doesn't remove anything from Content Moderation.

I guess if we're looking to "move" the permission from Content Moderation to Node it should remove from Content Moderation, as well as adding to Node.

oh, didn't notice content_moderation has gone into core now.

Here's the adds to the patch.

thanks

I don't think this permission should be part of Node.

+++ b/core/modules/content_moderation/content_moderation.module
@@ -166,14 +166,7 @@ function content_moderation_entity_access(EntityInterface $entity, $operation, A
-    $access_result = (($entity instanceof EntityPublishedInterface) && !$entity->isPublished())
-      ? AccessResult::allowedIfHasPermission($account, 'view any unpublished content')
-      : AccessResult::neutral();

In content moderation this permission worked for all publishable entities. Do we need a permission for each module that provides a publishable entity (currently in core this is Node, Comment, and Media), or do we have a catch all permission?

Also as Content Moderation is now Beta we need to provide full backwards compatibility and an upgrade path. So we'll need to make sure that if a site had the permission enabled in Content Moderation that they have the permission in all other modules enabled after the patch.

Considering what @timmillwood said in #140, I talked with @joey-santiago, and we agreed with Tim that this feature is better provided by using Content Moderation. Content Moderation does provide better framework to manage content types' permissions than by implementing them individually.

I think this issue should retire in a respectable age of nine years, marked "Closed (outdated)".

I'm not really suggesting that it should be in Content Moderation, just that if the permission is being moved out of Content Moderation it will need to provide the same level of functionality as the one in Content Moderation does. The current patch is node specific so does not offer backwards compatibility with the Content Moderation permission.

I think permissions related to the publishing status are better in Content Moderation than in individual content entity types. Adding such permission to Node (and all other modules that need it) would fragment what is essentially one functionality. Besides, it is a working implementation in core that accomplishes the essence of what this issue originally requested, which I do prefer over this issue.

Now that I look into this, if the unpublished permission would be moved out of Content Moderation (it is a lot of module for one plain check, true), in my opinion the check for it should be defined in EntityPublishedInterface (and implemented in EntityPublishedTrait) which is the source of the published state for all editorial content entities, but that leaves the question of where the permission would be defined. If I haven't gotten something wrong, the only way is via a module's permissions.yml (or a callback defined in one). So that kind of brings us to the need for a module such as Content Moderation, right?

Hi,

Some (many?) sites may need a basic workflow (published/unpublished) and so need the "view unpublished" permissions without Content Moderation module. It would be a bit heavy to have to enable content moderation just to have these permissions.

The system module could be a good candidate to hold these permissions ?

We were using this patch before it was closed as outdated; we have no need for content moderation but we do need the ability to let certain roles view unpublished content.

Would it be that bad to get this into core?

This is still useful to get into core IMO. See the problems that people are getting into in #2934011: [PP-1] Add a selection handler to content_moderation that respects 'view any unpublished content' .

Heads up: The patch in #137 does not apply on 8.5.0. Good to keep in mind before #drupalgeddon2

I have rebased the last patch against 8.6.x, rearranged the tests, and updated \Drupal\node\Plugin\views\filter\Status::query to account for the "view all unpublished content" permission.

+++ b/core/modules/node/node.module
@@ -935,6 +935,15 @@ function node_node_access(NodeInterface $node, $op, $account) {
+      ¶

Too many spaces.

Remove extra spaces from #149

Thanks, my biggest concern here is that the current "View any unpublished content" permission works in Content Moderation for all entity types, such as Nodes and Blocks. Where as the patch changes this to only work with Nodes.

Yes, I agree that it should be moved somewhere else. Since the publish/unpublish api was added in Drupal 8.3 for all entities, it wouldn't be consistent to make the permission only available for nodes. It seems like it should be treated like the "access content" permission, which was moved into the system module in 8.5.x.

The real issue is where to put the access check logic. We need to pull this code from content moderation (lines 187-193 in content_moderation.module) and put it somewhere, but I'm not certain where the best place to put that is:

  $access_result = NULL;
  if ($operation === 'view') {
    $access_result = (($entity instanceof EntityPublishedInterface) && !$entity->isPublished())
      ? AccessResult::allowedIfHasPermission($account, 'view any unpublished content')
      : AccessResult::neutral();

    $access_result->addCacheableDependency($entity);
}

#143 mentions EntityPublishedInterface.php as a possibility. Does that still make sense? Also, how much will this impact performance?

It's worth pointing out that there is a similar permission for "View own unpublished content" that lives in the node module and only applies to nodes. Should there be an issue to move that out of the node module and make it apply to all entities as well?

I agree with #152 and doing #154 would make sense.

As far as where this should go, perhaps we should ping the Entity API maintainers for directions?

Had a chat with @timmillwood and we've figured out where we stand, there is a wider effort in this space which will make this patch easier to implement. And that is #2801031: Provide a generic entity access handler and permissions, which was implemented in entity api module, which is now being moved to core on #2809177: Introduce entity permission providers.

Let's postpone until #2809177: Introduce entity permission providers gets in, and focus our efforts there. After that is in, it will be simpler to figure out what we need to do here.

Re-roll

This will apply to the 8.7.0 branch.

When the status is 'postponed' even though on uploading it says "test with ..." the patch is not automatically queued. You have to click "add test / re-test" and do it manually (which is what I have just done with the #159 patch)

More accurate title

This will apply to the 8.8.0 branch.

This will apply to the 8.9.x branch.

Re-rolled to 9.2.x

Patch still applies to 10.1.x, however

+++ b/core/modules/node/node.module
@@ -806,6 +806,15 @@ function node_node_access(NodeInterface $node, $op, AccountInterface $account) {
+    case 'view':

Shouldn't we do this in NodeAccessControlHandler?

I don't understand why the existing code is in this hook but perhaps it just hasn't been moved out yet.

Re-roll for latest 10.1