#57893 closed enhancement (fixed)
Remove the redundancy of adding row actions in `class-wp-media-list-table.php`
Reported by: | kebbet | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | 3.1 |
Component: | Media | Keywords: | has-patch has-unit-tests needs-testing |
Focuses: | ui | Cc: |
Description
This is a ticket related to #57890 which dealt with issues of row actions not displayed while a user has filtered the media library list.
The reason is _get_row_actions
which duplicates code for multiple row actions in the if ( $this->detached )
and else
logic.
The goal of this ticket is to remove the redundancy in that function to avoid furter issues like the ones in #57890.
Props to @pbiron for suggesting creation of this ticket.
Version set to 3.1
, introduced in [16229].
Attachments (1)
Change History (33)
#3
@
19 months ago
This ticket was mentioned in PR #4364 on WordPress/wordpress-develop by @kebbet.
19 months ago
#4
- Keywords has-patch added; needs-patch removed
#6
@
19 months ago
- Keywords has-patch needs-testing has-testing-info added; needs-patch removed
PR ready for review, if you have the time @SergeyBiryukov @pbiron
To test:
- test all filters in the media list view
- test setting and unsetting media constants
EMPTY_TRASH_DAYS
andMEDIA_TRASH
- make sure all row actions actually perform their action
- extra: make sure
media_row_actions
works as expected.
What has changed from current implementation in trunk
Download
is shown in all filter views (even trash)Copy
is shown in all views except trash
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
18 months ago
#8
@
18 months ago
If this ticket gets merged into core before #57890, then that ticket can be closed.
This ticket was mentioned in Slack in #core by kebbet. View the logs.
17 months ago
#10
@
17 months ago
I tested the patch, Copy URL
and Download file
are in place and working, but Delete permanently
link went missing (screenshot is above).
#11
@
17 months ago
- Keywords needs-refresh added
Thanks so much for testing @oglekler. I can confirm your findings.
#12
@
17 months ago
- Keywords needs-refresh removed
Linked PR (PR#4364) is updated to fix the issue. Please test again.
#13
@
17 months ago
I've now tested with both setting and unsetting EMPTY_TRASH_DAYS
and MEDIA_TRASH
in wp-config.php
, with kept behavior (except the changes intended with this ticket, see comment 6).
#14
@
17 months ago
Hi @desrosj (as you are a maintainer of media component). Can you please review this ticket and the linked PR? Is it possible to get this into 6.3?
#16
@
17 months ago
Attached PR has been updated after a nice review by @costdev, thanks!
It now skips my suggested introduction of a foreach loop, and adds the action directly to the $actions
array after the conditional check for each action.
It also now removes the $delete_ays
variable which was only used once.
#18
@
17 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
@SergeyBiryukov PR 4364 included a check that $attachment_url
was truthy before adding the copy
and download
actions. This wasn't included in [55949], so the copy
and download
actions may added with an empty string URL if wp_get_attachment_url()
returns false
.
Applying add_filter( 'wp_get_attachment_url', '__return_false' );
shows that when clicking Copy URL
or Download
. Copy
won't do anything, and Download
will download the wp-admin/upload.php
as an HTML file.
I'm preparing a PR with the conditions added as well as PHPUnit tests to protect the behaviour touched in [55949]. I'll have this ready later this evening.
#19
@
17 months ago
- Keywords needs-patch needs-unit-tests added; has-patch needs-testing has-testing-info removed
This ticket was mentioned in PR #4651 on WordPress/wordpress-develop by @costdev.
17 months ago
#20
- Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed
17 months ago
#22
The PR looks good, looking at the media table part. I'm a novice at tests, so can not really say anything about them.
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
17 months ago
#24
@
17 months ago
@SergeyBiryukov Just checking if you saw comment 18 and PR 4651 yet? No rush, just in case the noise of pre-Beta meant this one slipped by 🙂
#26
@
17 months ago
I have tested the patch, but can not say anything on tests. What I have tested looks and works correctly, as in removal of Download link and Copy-action when adding add_filter( 'wp_get_attachment_url', '__return_false' );
. When patch is not applied, those links are still present.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
17 months ago
This ticket was mentioned in Slack in #core-test by oglekler. View the logs.
17 months ago
#30
@
17 months ago
@kebbet from my point of view PR looks good, I suggest pinging this ticket owner @SergeyBiryukov 😅
@SergeyBiryukov commented on PR #4651:
17 months ago
#32
Thanks for the PR! Merged in r56072.
@kebbet , I'd like to work on this ticket.