Make WordPress Core

Opened 20 months ago

Closed 17 months ago

Last modified 17 months ago

#57893 closed enhancement (fixed)

Remove the redundancy of adding row actions in `class-wp-media-list-table.php`

Reported by: kebbet's profile kebbet Owned by: sergeybiryukov's profile 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)

57893-test.png (59.2 KB) - added by oglekler 17 months ago.

Download all attachments as: .zip

Change History (33)

#1 @kebbet
20 months ago

  • Type changed from defect (bug) to enhancement

#2 @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 6.3

#3 @ignatggeorgiev
19 months ago

@kebbet , I'd like to work on this ticket.

Version 0, edited 19 months ago by ignatggeorgiev (next)

This ticket was mentioned in PR #4364 on WordPress/wordpress-develop by @kebbet.


19 months ago
#4

  • Keywords has-patch added; needs-patch removed

#5 @kebbet
19 months ago

  • Keywords needs-patch added; has-patch removed

My attached PR (#PR 4364) also adresses #57890. So there is a bit of a overlap.

#6 @kebbet
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 and MEDIA_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 @kebbet
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

@oglekler
17 months ago

#10 @oglekler
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 @kebbet
17 months ago

  • Keywords needs-refresh added

Thanks so much for testing @oglekler. I can confirm your findings.

#12 @kebbet
17 months ago

  • Keywords needs-refresh removed

Linked PR (PR#4364) is updated to fix the issue. Please test again.

#13 @kebbet
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 @kebbet
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?

#15 @SergeyBiryukov
17 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#16 @kebbet
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.

#17 @SergeyBiryukov
17 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55949:

Media: Display the “Copy URL” and “Download file” row actions when the “Unattached” filter is applied.

Due to partially duplicated logic for displaying row actions in the Media Library with and without the “Unattached” filter, the “Copy URL” and “Download file” row actions were unintentionally missing with the filter applied.

This commit aims to simplify the logic and bring more consistency to the code.

Includes displaying the “Download file” row action even when the “Trash” filter is applied, giving the user one more chance to download the media file before they delete it.

Follow-up to [8901], [13100], [16227], [16229], [52842], [55198], [55221].

Props kebbet, costdev, pbiron, oglekler, SergeyBiryukov.
Fixes #57890, #57893.

#18 @costdev
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 @kebbet
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

#21 @costdev
17 months ago

PR 4651 is ready for review

@kebbet commented on PR #4651:


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 @costdev
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 🙂

#25 @oglekler
17 months ago

  • Keywords needs-testing added

#26 @kebbet
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

#29 @kebbet
17 months ago

In what way does this ticket needs testing @oglekler ?

#30 @oglekler
17 months ago

@kebbet from my point of view PR looks good, I suggest pinging this ticket owner @SergeyBiryukov 😅

#31 @SergeyBiryukov
17 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 56072:

Media: Only show “Copy” and “Download” actions when an attachment URL is available.

Includes unit tests to verify the logic for displaying row actions in the Media Library in certain scenarios, e.g. with and without the “Trash” or “Unattached” filter.

Follow-up to [55949].

Props costdev, kebbet, mukesh27, oglekler.
Fixes #57893.

@SergeyBiryukov commented on PR #4651:


17 months ago
#32

Thanks for the PR! Merged in r56072.

Note: See TracTickets for help on using tickets.