WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 4 days ago

#48325 closed enhancement (fixed)

Additional filter in media-template.php. Let developers to hide attachment page link.

Reported by: limera1n Owned by: garrett-eclipse
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch commit
Focuses: ui, administration Cc:

Description

Hi,

currently there is no easy way to hide "View attachment page" link from "media-modal" window. I think it would be great to add simple filter to disable this link and also add some HTML markups for "|" separators which can help users to use CSS selectors and style them. For example:

media-template.php, lines from 428

<div class="actions">
<?php
if ( ! apply_filters( 'disable_attachment_page_link', '' ) ) : ?>
<a class="view-attachment" href="{{ data.link }}"><?php _e( 'View attachment page' ); ?></a>
<span class="links-separator">|</span>
<?php endif; ?>

I think this feature is especially important when we don't want allow our users to access this type of pages.

https://i.imgur.com/C0AcLAG.png

Attachments (8)

48325.1.patch (1.4 KB) - added by Mista-Flo 8 weeks ago.
48325.1.2.patch (1.2 KB) - added by Mista-Flo 8 weeks ago.
48325.3.patch (1.4 KB) - added by Mista-Flo 8 weeks ago.
Fix display of separator
48325.4.patch (1.2 KB) - added by Mista-Flo 8 weeks ago.
Remove gitignore change
48325.5.patch (1.1 KB) - added by Mista-Flo 8 weeks ago.
48325.6.patch (1.1 KB) - added by Mista-Flo 8 weeks ago.
48325.7.patch (1.2 KB) - added by Mista-Flo 8 weeks ago.
48325.8.diff (1.7 KB) - added by garrett-eclipse 2 weeks ago.
Minor refresh to add a @since 5.6.0 comment to the attachment_link filter.

Download all attachments as: .zip

Change History (28)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


10 months ago

#2 @joemcgill
10 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hi @limera1n,

Thanks for this report. I agree that this is not an easy thing to override at the moment. You would either need to override the whole template, or hide that link with CSS—neither a good option.

Allowing that template code to be filtered is one option for how to solve this, but I wonder if a better approach would be to update that template to check for the existence of a data.link value before printing the link so that this won't show up if the link value isn't available for any reason?

#3 @Mista-Flo
8 weeks ago

  • Keywords has-patch added; needs-patch removed

All right, I have combined both requests.

I have created a hook and checked if data.link is set.

I used a boolean to check the filter value, so if you want to disable it from a theme/plugin, you would do it like this:

<?php
add_filter( 'disable_attachment_page_link', '__return_true' );

@Mista-Flo
8 weeks ago

@Mista-Flo
8 weeks ago

#4 @Mista-Flo
8 weeks ago

Removed the .gitignore update on the second patch, sorry

@Mista-Flo
8 weeks ago

Fix display of separator

@Mista-Flo
8 weeks ago

Remove gitignore change

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


8 weeks ago

#6 @garrett-eclipse
8 weeks ago

  • Keywords dev-feedback added

@Mista-Flo great work. Wondering if we need a new filter here though. If we just conditionally check for data.link before use then users can disable using the pre-existing attachment_link filter which lives in the get_attachment_link used to populate data.link. Could update the filter docblock to indicate it's now used in this case.

Most users will want to just disable all or they could conditionally run on a screen or post bases.

P.S. If a new filter is decided to be added then take a look at documenting them here;
https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-hooks-actions-and-filters

@Mista-Flo
8 weeks ago

@Mista-Flo
8 weeks ago

#7 @Mista-Flo
8 weeks ago

Arg forgot to check if an existing filter was in the PHP side. Good catch thank you Garret. So IMO, it doesn’t make sense to add a new filter in the template. But what we can do is to keep a check if data.link so that empty strings won't be displayed if the user want to use the filter you mentioned. This is my proposed patch (6th).

#8 @garrett-eclipse
8 weeks ago

Thanks @Mista-Flo that's great, I think it's looking really well.

Only issue I see now is with the separator functionality. Prior to our changes there was always a first item so all following conditional items could just place the separator first and it would always work nicely.

Now if there's not a following item due to the conditionals we can end up with a orphaned separator on the end. There's two approaches (maybe more) not sure what's best; we could wrap conditionals around the separators to check there's going to be a following item, or we could drop them in the html and move to CSS to place as an after element only when element + element selector works. Or there might be css we can use to hide that orphaned separator.

Thoughts?

#9 @Mista-Flo
8 weeks ago

I have uploaded a new patch. I'm not a huge fan of the resulted logic (less readable), but it handles each case so we won't end up with orphaned.

The CSS way seems a bit overkill here, but if anyone feels it's better, feel free to write an updated patch.

@Mista-Flo
8 weeks ago

#10 @garrett-eclipse
8 weeks ago

  • Focuses administration added
  • Keywords needs-testing added; dev-feedback removed
  • Milestone changed from Future Release to 5.6

Thanks @Mista-Flo appreciate the refresh, it's looking good to me. Can't wrap my head around a simpler CSS solution so let's stick with that.

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


7 weeks ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


2 weeks ago

#15 follow-up: @hellofromTonya
2 weeks ago

Recapping discussion from today's Media scrub, ie so we don't forget:

Per @antpb

I can do the test and even run the commit. I think from the comment Garrett is feeling good about it so we really just need one more person to test and commit

@garrett-eclipse
2 weeks ago

Minor refresh to add a @since 5.6.0 comment to the attachment_link filter.

#16 in reply to: ↑ 15 @garrett-eclipse
2 weeks ago

  • Keywords commit added; needs-testing removed
  • Owner set to garrett-eclipse
  • Status changed from assigned to accepted

Replying to hellofromTonya / @antpb:

I can do the test and even run the commit. I think from the comment Garrett is feeling good about it so we really just need one more person to test and commit

Marking for commit, I'm happy with the logic and setup. After re-reading the thread I found my comment on adding a @since to the attachment_link filter to expose fact you can now disable the link via that filter. To handle that I added 48325.8.diff which is the same logic just adds a @since comment. For reference my mention of that:
comment#6:

Could update the filter docblock to indicate it's now used in this case.

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


5 days ago

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


5 days ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


4 days ago

#20 @antpb
4 days ago

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

In 49222:

Media: Allow hiding of ‘View attachment page’ link in media modal.
If an empty string is supplied to the attachment_link filter the ‘View attachment page’ link will be hidden in the media modal.
Props limera1n, garrett-eclipse, joemcgill, Mista-Flo, hellofromTonya.
Fixes #48325.

Note: See TracTickets for help on using tickets.