Remove published/unpublished operations when WBM is enabled, as they no longer apply

Replacing that view outright would depend on #2674520: Add current revision filter to views first. That's why we need that issue. :-)

Simply removing the publish/unpublish commands seems like the best way to handle that. Those commands are no longer useful in any context when an entity is moderated.

Patch welcome. :-)

Retitling. If you can get a patch posted today it may get into beta2. If not, it can go into the next release (which I hope is an RC, but that depends on the Views integration).

Something like this. (HT to EclipseGc for the Plugin help.) I'm not quite sure of the best way to test this, though.

Shouldn't these plugins be overriding the access() method to deny the action instead of overridding the execute() method?

Talked this through with Dave in IRC. Probably we should be checking in both methods, to be sure; however, using the access method also means that the error message you get becomes much more generic and thus less useful on the admin/content page, or anywhere that's using the Views BulkForm plugin. However, it means it will behave properly if someone calls access() on the plugin from anywhere else. So... I'm torn on the approach to take. Feedback welcome.

@Bcwald: Unfortunately the messages you see are set by core, now that access() is used. I would argue that the error messages could be improved (displaying actual number of updated instead of blindly saying all were updated, even though access() returned FALSE).

Yeah, the messaging is pretty bad. Unfortunately, there's extremely little I can do from the plugin to fix it. Let me try and lay it out:

The plugin type has two methods: access() and execute(). The way you are *supposed* to use the actions is something to the effect of:

if ($action->access($entity)) {
  $action->execute($entity);
}

Which is, on the surface, a pretty common pattern in core now. However, actions are frequently going to be applied to multiple entities at once (as in the Views BulkForm case). What BulkForm does is essentially this (simplified from the actual code to emphasize the problem):

foreach ($entities as $entity) {
  if (!$action->access($entity)) {
    drupal_set_message("No access to {$entity->label}"));
    continue;
  }
  $real_entities[] = $entity;
  $count++;
}

foreach ($real_entities as $entity) {
  $action->execute($entity);
}

$count = count($entities);
drupal_set_message("Did stuff to {$count} entities.");

First off, there's a bug here in BulkForm because it keeps track of the number of entities for which access() passed, but then does nothing with it and uses the original entity count for the message. That's just flat out wrong and should be fixed in core.

Secondly, it means that if we use access(), there is *no way at all* to suppress the "No access to $entity" message. So implementing access() means that message appears. If we don't want that message to appear, we have to not use access().

But! Not using access() means that someone other than BulkForm that tries to use the action will end up trying to execute the action even when they shouldn't, thus execute() needs to have the opt-out logic. That's what I did originally. If we don't, then the entity will get saved, its moderation state field (which was unmodified) will result in the published property being overwritten with whatever WBM says it should be (which is then presumably the same as the previous state), and a new revision with no change gets resaved, seemingly "nothing happened".

Additionally, execute() is a single-item method. That means I have no way of counting the total number of times it was called, and thus no way to report "skipped X items". The only options are a static string (as the patch has now), for which drupal_set_message() helpfully removes duplicates (which is kind of cool), or a "skipped $entity->label()" message, which leads to one message per entity skipped.

Removing the publish/unpublish commands from the /admin/content form requires a hook_views_alter(), which is highly fragile magic. I really don't want to go there.

It would be quite easy to just kill the Publish/Unpublish actions entirely via an alter hook, but I don't know how well that view would handle its configured actions suddenly missing (would views handle that gracefully or explode in a fireball?), and it would also make those actions unavailable to any module anywhere, ever, which seems excessively draconian. Causing fatal errors for other modules is not a route I am comfortable taking.

So in the end, the options we have are:

1) Use the API "properly", get an unhelpful generic message from BulkForm, one per item skipped.

2) Filter in execute() instead, get a single static message.

3) Filter in execute() instead, get one message per item skipped.

4) Both 1 and 2.

5) Both 1 and 3.

None of those are good options, but those are the only options I see available without monkeypatching the BulkForm plugin, too, and I am not going there. :-)

Correct, the view could be modified to disable those actions, and honestly I'd recommend everyone using WBM do so. However, doing so programmatically from WBM is considerably trickier and opens up a whole host of questions (what if someone customized the view already, what if they replaced it, what if they don't want those options to go away, etc.) that I really would rather avoid, especially when I'm hoping for a 1.0 this week. :-)

Anyone else want to argue for option 5, instead of option 4?

One note: Options 4 and 5 mean that the message from execute will NOT get shown on the bulk page! That message will show ONLY in the case that someone other than BulkForm tries to call execute() without properly checking access() first.

Posted a patch to fix BulkForm's counting issues: #2705823: BulkForm reports incorrect count

Oh yeah, I had a README file lying around in another branch that I'd started on but never finished. :-) Patch attached adds a README file, including the recommendation from #18. (No interdiff because nothing changes except that readme.)

I'm going to make an executive decision: You really ought to just disable those actions for that view entirely. The README now documents that you should do that. Therefore, follow the API "properly" (option 1) but also include the fail-safe in case someone doesn't call access() before calling execute() (option 2).

If the bot approves of this patch I will go ahead and merge it.

Merged. I also gave Bcwald some credit love for the readme text. :-) Thanks all!

Nice. It makes sense to do this by default in the Lightning distro: #2705931: Remove publish/unpublish actions from /admin/content view

That would be a separate README entry. That would again be a "don't want to go there" views alter to do automatically.