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.
Comment | File | Size | Author |
---|---|---|---|
#56 | interdiff-2877383-55-56.txt | 2.49 KB | starshaped |
#56 | 2877383-56.patch | 15.42 KB | starshaped |
#55 | interdiff-2877383-53-55.txt | 2.64 KB | starshaped |
#55 | 2877383-55.patch | 15.41 KB | starshaped |
#53 | interdiff-2877383-47-53.txt | 10.72 KB | chr.fritsch |
Comments
Comment #2
phenaproximaThis patch was extracted from #389 in #2831274: Bring Media entity module to core as Media module. It restores action integration to Media.
Comment #3
phenaproximaUnblocked!
Comment #4
Gábor HojtsyFound this that is highly related: #2886178: Media view includes media_bulk_form but no implementation.
Comment #5
chr.fritschI added the removed code from #2886178: Media view includes media_bulk_form but no implementation to this patch to make the tests working again.
Comment #7
chr.fritschCould this land in 8.4.x? Because currently we have a broken schema, which is a bug.
Comment #8
phenaproximaTests 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.
Comment #9
phenaproximaComment #10
Gábor HojtsyCan 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.
Comment #11
xjmAlso, 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?
Comment #12
phenaproximaI 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:
In light of all this, I think we can go back to RTBC :)
Comment #13
Wim LeersThe access checking logic looks correct!
Nit: s/$result/$access/ would make the code more legible.
->setChangedTime(0)->save()
Comment #14
chr.fritschFixed the nits from #13
Comment #15
phenaproximaBack to RTBC on the assumption that Drupal CI will pass it.
Comment #16
xjm@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.
Comment #17
Gábor HojtsyActions 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.
Comment #18
chr.fritschRegarding 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.
Comment #19
phenaproximaThey 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.
Comment #20
phenaproximaI also want to address this:
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.
Comment #21
Wim Leers#14 only fixed #13.1 in
PublishMedia
, there were more occurrences.Comment #22
phenaproximaOkay, let's get that fixed.
Comment #23
seanBFixed #13.1 in
UnpublishMedia
as well. That seemed to be the last one.Comment #24
phenaproximaSo, we have established:
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.
Comment #25
phenaproximaFixing the IS, since this issue was un-postponed a long time ago.
Comment #26
marcoscanoPostponed #2915738: Increase reliability of upgrade path to Media in core on this issue.
Comment #27
seanBFollowup created..
Comment #28
phenaproximaThen I think we're ready.
Comment #29
xjmAlright, 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!
Comment #30
phenaproximaPutting this back in review while the Media Initiative maintainers discuss internally how to proceed.
Comment #31
phenaproximaAfter 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!
Comment #32
Wim LeersWouldn'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?
Comment #33
phenaproximaIt 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
Comment #34
chr.fritsch#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
Comment #35
xjmYeah, let's reroll this on top of that once it lands, since we can't really backport it either way. Thanks!
Comment #36
chr.fritschThis should also be postponed on #2670730: Provide a delete action for each content entity type
Comment #37
chr.fritschI 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
Comment #38
chr.fritschSince #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.
Comment #39
chr.fritschI added the missing MediaBulkForm class
Comment #40
larowlan#2934424: Media has no collection route is in
Comment #41
chr.fritschThis would be the implementation if we could land #2670730: Provide a delete action for each content entity type
Comment #43
chr.fritschSo, finally. Let's land this
Comment #44
chr.fritschDamn, forgot to add the form class
Comment #45
BerdirI 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:)
Comment #47
chr.fritschOk, deleted that class
Comment #48
phenaproximaPatch is straightforward. Tests are suitably thorough. Let's land this.
Comment #49
marcoscanoCreated #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.
Comment #51
chr.fritschTestbot hickup
Comment #52
alexpottDiscussed 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.
Comment #53
chr.fritschSo I deprecated MediaDeleteMultipleConfirmForm and let media use the DeleteMultipleForm
Comment #54
phenaproximaLooks good, just a couple of grammatical nitpicks.
Can we say "This route is not used in Drupal core"?
Should be "As an internal API..."
Let's do the same corrections here.
No need for "The", or __NAMESPACE__. I think we can just do
@trigger_errors(__CLASS__ . ' is deprecated...')
Comment #55
starshapedUpdated the grammatical nitpicks that phenaproxima noted in #54.
Comment #56
starshapedMade a few more grammatical changes.
Comment #57
phenaproximaThanks, @starshaped! I think this addresses all concerns. Back to RTBC.
Comment #58
alexpottAdding review credit.
Comment #59
alexpottCommitted f4c5cdf and pushed to 8.6.x. Thanks!
Comment #61
chr.fritschWould it be possible to port this together with #2670730: Provide a delete action for each content entity type back to 8.5.x?
Comment #63
John Pitcairn CreditAttribution: John Pitcairn commented@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.