57893 row action media table

[kebbet]

https://core.trac.wordpress.org/ticket/57893

[costdev]

I know the aim is to keep each action in its own separate condition block, but I'm thinking we can reduce the calls to wp_get_attachment_url() here.

$attachment_url = wp_get_attachment_url( $post->ID );
if ( $attachment_url ) {
	if ( ! $this->is_trash ) {
		$actions['copy'] = sprintf(
			'<span class="copy-to-clipboard-container"><button type="button" class="button-link copy-attachment-url media-library" data-clipboard-text="%s" aria-label="%s">%s</button><span class="success hidden" aria-hidden="true">%s</span></span>',
			esc_url( $attachment_url ),
			/* translators: %s: Attachment title. */
			esc_attr( sprintf( __( 'Copy &#8220;%s&#8221; URL to clipboard' ), $att_title ) ),
			__( 'Copy URL' ),
			__( 'Copied!' )
		);
	}

	$actions['download'] = sprintf(
		'<a href="%s" aria-label="%s" download>%s</a>',
		esc_url( $attachment_url ),
		/* translators: %s: Attachment title. */
		esc_attr( sprintf( __( 'Download &#8220;%s&#8221;' ), $att_title ) ),
		__( 'Download file' )
	);
}

What do you think @kebbet?

[kebbet]

While it's reducing calls to wp_get_attachment_url() it nests two action in one conditional check. I don't like the look of it, but my opinion on that is just opinonated. Reduced number of calls is better I guess.

[costdev]

We can reduce the number of calls to get_permalink() here too:

Suggested change

	if ( ! $this->is_trash && get_permalink( $post->ID ) ) {
	$actions['view'] = sprintf(
	'<a href="%s" aria-label="%s" rel="bookmark">%s</a>',
	get_permalink( $post->ID ),
	/* translators: %s: Attachment title. */
	esc_attr( sprintf( __( 'View &#8220;%s&#8221;' ), $att_title ) ),
	__( 'View' )
	);
	}
	if ( ! $this->is_trash ) {
	$permalink = get_permalink( $post->ID );
	if ( $permalink ) {
	$actions['view'] = sprintf(
	'<a href="%s" aria-label="%s" rel="bookmark">%s</a>',
	$permalink,
	/* translators: %s: Attachment title. */
	esc_attr( sprintf( __( 'View &#8220;%s&#8221;' ), $att_title ) ),
	__( 'View' )
	);
	}
	}

[costdev]

Oh also, shouldn't the permalink be passed through esc_url()? It isn't current escaped in trunk though 🤔.

@SergeyBiryukov What do you think?

[kebbet]

I think the permalink could be escaped through esc_url(). Do you think get_edit_post_link() and wp_nonce_url() should be escaped with it too, the pattern exists in core/trunk. Any directions what to do here Sergey?