This issue is spun off from #2831274: Bring Media entity module to core as Media module, #390. Specifically, this point:

Actions integration, including all test coverage for it, configuration, SaveMedia action, etc. This should be a "Should" followup since we are unsure how much contrib needs this. There should also be a related followup for generic entity actions (as per tstoeckler's suggestion early on the issue).

To make it easier to review and commit the monstrosity that is #2831274: Bring Media entity module to core as Media module, action integration for Media should be discussed, refined, reviewed and committed in this issue.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

This patch was extracted from #389 in #2831274: Bring Media entity module to core as Media module. It restores action integration to Media.

phenaproxima’s picture

Status: Postponed » Needs review

Unblocked!

Gábor Hojtsy’s picture

chr.fritsch’s picture

I added the removed code from #2886178: Media view includes media_bulk_form but no implementation to this patch to make the tests working again.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chr.fritsch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Category: Feature request » Bug report

Could this land in 8.4.x? Because currently we have a broken schema, which is a bug.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Media Initiative

Tests pass. I see no reason to delay this anymore, and as @chr.fritsch points out, this breaks some config schema and is therefore a blocker. We need this.

phenaproxima’s picture

Issue tags: +blocker
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs review

Can we identify where is the schema broken that this is fixing? It looks purely schema additions. I believe we removed the part of the config where this schema would be needed in #2886178: Media view includes media_bulk_form but no implementation. Why is config import test not failing if the schema is missing? This would be good to identify. Otherwise there may be other config problems if there is a test missing.

xjm’s picture

Also, missing schemata is a bug, whereas action support sounds like a feature addition, so would be good to clarify the scope there. Is this two separate issues bundled together?

phenaproxima’s picture

Category: Bug report » Feature request
Status: Needs review » Reviewed & tested by the community
Issue tags: -blocker

I looked into it. It turns out this is a feature request, not a bug, and it is not really a blocker.

However, we really do need it because without it, the update path from Media Entity to core Media is incomplete. Out of the box, Media Entity included four Action config entities. And without this patch, Media Entity's update path to core needs to delete those entities entirely, since the plugins and schemata to back up those config entities won't exist. Obviously, this could potentially break sites that are using those entities in some way.

So, to summarize:

  • Not really a bug
  • One issue, not two
  • Necessary to complete Media Entity's update path to core

In light of all this, I think we can go back to RTBC :)

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

The access checking logic looks correct!

  1. +++ b/core/modules/media/src/Plugin/Action/PublishMedia.php
    @@ -0,0 +1,40 @@
    +    $result = $object->access('update', $account, TRUE)
    
    +++ b/core/modules/media/src/Plugin/Action/UnpublishMedia.php
    @@ -0,0 +1,40 @@
    +    $result = $object->access('update', $account, TRUE)
    

    Nit: s/$result/$access/ would make the code more legible.

  2. +++ b/core/modules/media/src/Plugin/Action/SaveMedia.php
    @@ -0,0 +1,40 @@
    +      $entity->changed = 0;
    +      $entity->save();
    

    ->setChangedTime(0)->save()

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
20.38 KB
1.2 KB

Fixed the nits from #13

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC on the assumption that Drupal CI will pass it.

xjm’s picture

Status: Reviewed & tested by the community » Needs review

@phenaproxima, counterpoint, couldn't the actions just be provided by contrib?

The questions in the summary haven't been answered yet. We pulled these out of the main patch for a reason, so let's address it.

  1. How widely used are the actions?
  2. Do we have an issue for exploring generic entity actions instead, like @tstoeckler suggested?
Gábor Hojtsy’s picture

Actions is what drives bulk operations. It would be sad IMHO if core could not provide bulk publish/unpublish, etc. actions for media. Based on my discussion with @catch he said that he suggested to pull this out of the main patch because it was not required to make it stable in itself, not that this is not needed in core.

chr.fritsch’s picture

Regarding to #16.2: In #2843395: Add a trait or base class for BuildForm() and SubmitForm() in Drupal\node\Form\DeleteMultiple i proposed to move some generic actions from the contrib entity module to core.

phenaproxima’s picture

We pulled these out of the main patch for a reason

They were pulled out because the patch from which they emerged was a monstrous 250+ KB, and removing this stuff made that easier to review, since the actions are not really part of the API. But they are very necessary. As @Gabor Hojtsy points out, the Media view's bulk actions do not work without them. And sites that are already using these actions will break unless we put them somewhere.

We could move them into contrib, sure, but what is the advantage of that? All it means is that people will need to keep an additional module (probably Media Entity 2.x) enabled until a suitable replacement (whatever it is) lands in core, which could take a very long time -- at which point it will impose upon us the burden of adding an update path. This, to me, represents unnecessary confusion and inconvenience for users.

I really like the idea of generic entity actions. If an explicit issue does not exist, I'm totally on board to open one. But I don't think we should block this on that.

phenaproxima’s picture

I also want to address this:

How widely used are the actions?

We can assume that they are used by just about everyone. Out of the box, Media Entity provides a view, similar to the /admin/content view, which shows all media items in your site. This is the main entry point for managing your site's entire media library, just as /admin/content is the main entry point to managing your content.

That view has a set of bulk operations (powered by the actions) enabled out of the box. So in other words, every single installation of Media Entity comes with this functionality. Since the bulk actions are quite useful for managing large numbers of media items, I see very little reason why 90% of users would disable or remove them, unless there was some sort of incompatibility with another module. So the safe assumption, to me, is that the actions are very widely used.

Wim Leers’s picture

#14 only fixed #13.1 in PublishMedia, there were more occurrences.

phenaproxima’s picture

Status: Needs review » Needs work

Okay, let's get that fixed.

seanB’s picture

Status: Needs work » Needs review
FileSize
20.38 KB
844 bytes

Fixed #13.1 in UnpublishMedia as well. That seemed to be the last one.

phenaproxima’s picture

Issue tags: +Needs followup

So, we have established:

  • This issue is important for the cross-grade from Media Entity to be complete
  • It is safe to assume that most sites with Media Entity installed are using at least some of these actions
  • Because of that, we cannot retire Media Entity until this lands, since the actions must live somewhere
  • We'd really like entity actions to be generic, at least at some point

Given this, I see no reason this cannot go back to RTBC...once there is an issue, either opened anew or linked to this one, exploring the possibility of generic entity actions in core. I'm tagging this as needing a follow-up, and once we have that, this is ready to go as far as I'm concerned. I simply do not see any reason to delay it further.

phenaproxima’s picture

Issue summary: View changes

Fixing the IS, since this issue was un-postponed a long time ago.

marcoscano’s picture

seanB’s picture

Followup created..

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

Then I think we're ready.

xjm’s picture

Version: 8.4.x-dev » 8.5.x-dev

Alright, all that makes sense to me. Thanks! And I see the value of adding the actions and not blocking them on some generic solution for all of core. (I think I misunderstood the scope of what @tstoeckler was suggesting previously.)

As a feature addition, this should only go into a minor release, since Media is stable. @phenaproxima pointed out that this indirectly creates issues for the upgrade path from contrib for 8.4.x, since this is a key feature and contrib may expect it to exist, but that they will comment with a plan to work around that. I'd propose detailing said plan elsewhere than this issue (the issue already linked above?) and just summarizing since we've determined it is actually just a feature addition. :) I haven't reviewed the patch itself in detail yet but I'm comfortable with this for 8.5.x given the information above. Thanks!

phenaproxima’s picture

Status: Reviewed & tested by the community » Needs review

Putting this back in review while the Media Initiative maintainers discuss internally how to proceed.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

After internal discussion and agreement among the members of the Media Initiative, @marcoscano began work on a separate Media Entity Actions contrib module, which will fill the gap left by this patch during the 8.4.x cycle: #2917124: Create Media Entity Actions to hold actions not included in 8.4.x.

Once 8.5.x is released, that module will be responsible for migrating to the actions included in core. So as not to break existing sites that are currently using these actions, Media Entity 2.x will, as part of its cross-grade path to core Media, require Media Entity Actions to be present in the system.

And with that, all questions and concerns surrounding this patch seem to have been addressed. Let's land this!

Wim Leers’s picture

Wouldn't it be simpler for everybody if this were also committed to 8.4.x instead of creating a separate contrib module? What is the potential harm?

phenaproxima’s picture

It would break semver in core, since it’s technically a feature addition. I too would prefer it were backported, but this is how the chips have fallen. /shrug

chr.fritsch’s picture

#2916740: Add generic entity actions is now RTBC and adds Publish/Unpublish and Save actions. If this only goes in 8.5.x, then we should now wait for #2916740: Add generic entity actions

xjm’s picture

Status: Reviewed & tested by the community » Postponed

Yeah, let's reroll this on top of that once it lands, since we can't really backport it either way. Thanks!

chr.fritsch’s picture

Title: Add action support to Media module » [PP-2] Add action support to Media module
Related issues: +#2670730: Provide a delete action for each content entity type
chr.fritsch’s picture

I just realized, that the media_bulk_form implementation is still missing ins this patch. And we should not implement it, and instead wait for #2909423: Get rid of BulkForm subclasses when they just override emptySelectedMessage

chr.fritsch’s picture

Status: Postponed » Needs work

Since #2916740: Add generic entity actions is currently postponed on #2922451: [policy no patch] Make it possible to mark plugins as deprecated it doesn't seems to land soon, i would vote for unpostponing this and deprecate the Media actions in #2916740: Add generic entity actions later.

chr.fritsch’s picture

Title: [PP-2] Add action support to Media module » Add action support to Media module
Status: Needs work » Needs review
FileSize
21.02 KB
504 bytes

I added the missing MediaBulkForm class

larowlan’s picture

chr.fritsch’s picture

This would be the implementation if we could land #2670730: Provide a delete action for each content entity type

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chr.fritsch’s picture

So, finally. Let's land this

chr.fritsch’s picture

Damn, forgot to add the form class

Berdir’s picture

+++ b/core/modules/media/src/Plugin/views/field/MediaBulkForm.php
@@ -0,0 +1,21 @@
+class MediaBulkForm extends BulkForm {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function emptySelectedMessage() {
+    return $this->t('No media selected.');
+  }
+

I think it's overkill to have a custom class for this just so that the the empty message says "No media selected" instead of "No items selected" from the parent:)

Status: Needs review » Needs work

The last submitted patch, 44: 2877383-43.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
2.35 KB

Ok, deleted that class

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Patch is straightforward. Tests are suitably thorough. Let's land this.

marcoscano’s picture

Created #2940035: Update upgrade path requirements to account for generic entity actions in core (8.5.x+) as a follow-up, to make the upgrade path from Media Entity contrib take into account this as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: 2877383-47.patch, failed testing. View results

chr.fritsch’s picture

Status: Needs work » Reviewed & tested by the community

Testbot hickup

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Discussed with @Berdir and @chr.fritsch on slack. We resolved to not use the existing MediaDeleteMultipleConfirmForm just slightly tweaked messages. The default message from DeleteMultipleForm is fine. We should just deprecate that class and say it is not used in core and change the route to use the correct class.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
15.49 KB
10.72 KB

So I deprecated MediaDeleteMultipleConfirmForm and let media use the DeleteMultipleForm

phenaproxima’s picture

Status: Needs review » Needs work

Looks good, just a couple of grammatical nitpicks.

  1. +++ b/core/modules/media/media.routing.yml
    @@ -1,3 +1,7 @@
    +#   It is not used in Drupal core. If you are using it, copy the class and the
    

    Can we say "This route is not used in Drupal core"?

  2. +++ b/core/modules/media/media.routing.yml
    @@ -1,3 +1,7 @@
    +#   related "entity.media.multiple_delete_confirm" route to your module. As
    +#   internal API, it may also be removed in a minor release.
    

    Should be "As an internal API..."

  3. +++ b/core/modules/media/src/Form/MediaDeleteMultipleConfirmForm.php
    @@ -13,6 +13,11 @@
    + * @deprecated in Drupal 8.6.x, to be removed before Drupal 9.0.0.
    + *   It is not used in Drupal core. If you are using it, copy the class and the
    + *   related "entity.media.multiple_delete_confirm" route to your module. As
    + *   internal API, it may also be removed in a minor release.
    

    Let's do the same corrections here.

  4. +++ b/core/modules/media/src/Form/MediaDeleteMultipleConfirmForm.php
    @@ -47,6 +52,7 @@ class MediaDeleteMultipleConfirmForm extends ConfirmFormBase {
    +    @trigger_error('The ' . __NAMESPACE__ . '\MediaDeleteMultipleConfirmForm is deprecated in Drupal 8.6.0 and will be removed before Drupal 9.0.0. It is not used in Drupal core. If you are using it, copy the class and the related "entity.media.multiple_delete_confirm" route to your module. As internal API, it may also be removed in a minor release.', E_USER_DEPRECATED);
    

    No need for "The", or __NAMESPACE__. I think we can just do @trigger_errors(__CLASS__ . ' is deprecated...')

starshaped’s picture

Status: Needs work » Needs review
FileSize
15.41 KB
2.64 KB

Updated the grammatical nitpicks that phenaproxima noted in #54.

starshaped’s picture

Made a few more grammatical changes.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, @starshaped! I think this addresses all concerns. Back to RTBC.

alexpott’s picture

Adding review credit.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f4c5cdf and pushed to 8.6.x. Thanks!

  • alexpott committed f4c5cdf on 8.6.x
    Issue #2877383 by chr.fritsch, starshaped, seanB, phenaproxima, xjm, Wim...
chr.fritsch’s picture

Would it be possible to port this together with #2670730: Provide a delete action for each content entity type back to 8.5.x?

Status: Fixed » Closed (fixed)

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

John Pitcairn’s picture

@chrfritsch: You'll need to open a separate issue for a backport I think.

The bulk actions plugin for core media is indeed present but broken in 8.5.x. Views_bulk_operations module provides a "delete entities" action that seems to work.