WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 7 weeks ago

Last modified 6 weeks ago

#45708 closed defect (bug) (fixed)

Allow retrieving adjacent image link markup without echoing it

Reported by: flixos90 Owned by: whyisjake
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests has-testing-info commit
Focuses: template Cc:

Description

Currently, themes can only echo image links (via next_image_link(), previous_image_link() and indirectly adjacent_image_link()), but not retrieve the markup before printing it. This is a problem, since you often want to manually print extra markup, however only if there are actual image links to display. You can currently of course accomplish that by using an output buffer, but that is far from preferred.

Most other templating functions have two versions, one for retrieving the markup, the other for echoing it (for example previous_post_link() and get_previous_post_link()). We should add the necessary counterparts for these ones too.

As another indicator that this has been needed for a long time, you might want to look at this article from 2009: ​https://katz.co/911/

Attachments (3)

45708.diff​ (4.7 KB) - added by flixos90 3 years ago.
45708.1.diff​ (19.9 KB) - added by hellofromTonya 2 months ago.
Refreshed, set to 5.8.0, and adds unit tests.
45708-output-buffering-test.gif​ (4.3 MB) - added by hellofromTonya 2 months ago.
Test original functions work with output buffering for backompat. Results: Works as expected.

Change History (21)

@flixos90
3 years ago

#1 @flixos90
3 years ago

  • Keywords has-patch added; needs-patch removed

45708.diff​ introduces the following functions:

  • get_adjacent_image_link()
  • get_next_image_link()
  • get_previous_image_link()

Their whole code is what was previously part of the respective function without the get_ prefix, with the only exception that the result is returned instead of echoed. The existing functions are now simply wrappers echoing the output of the new get_*() functions.

This is in line with how other templating functions act.

#2 @DrewAPicture
3 years ago

  • Keywords needs-unit-tests added

Seems sensible to have a getter, though I'm a little torn on whether we need to introduce three new functions just so there's getter parity for both of them.

#3 @mor10
3 years ago

@DrewAPicture I think getter parity is the entire point here. I remember working with the existing functions here and having to do a lot of extra work just because the get_ functions were missing. There should be parity across different similar functions which is what @flixos90 is proposing.

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

#5 @antpb
3 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 5.8

This issue was discussed in a recent Media bug scrub. It's agreed that parity is the goal here and valid. Moving this to 5.8 to keep track of it as it seems to have been buried over the last two years. The only thing currently blocking this is unit tests for the three new functions.

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

This ticket was mentioned in ​Slack in #core-test by hellofromtonya. ​View the logs.
2 months ago

@hellofromTonya
2 months ago

Refreshed, set to 5.8.0, and adds unit tests.

#8 @hellofromTonya
2 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Owner set to hellofromTonya
  • Status changed from new to assigned

Reviewed the patch and looks good to me. I agree with having separation functions for rendering and getting. Why?

  • Saves having to pass a render/echo argument
  • Each function serves a different purpose
  • Follows established patterns such as get_the_ID() + the_ID(), the_permalink() + get_permalink(), get_the_title() + the_title(), etc.

45708.1.diff​ refreshes the patch against the latest, updates to 5.8.0, and adds unit tests for each of the functions.

@antpb this one is ready for review and commit consideration.

This ticket was mentioned in ​Slack in #core-media by hellofromtonya. ​View the logs.
2 months ago

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

#11 @hellofromTonya
2 months ago

  • Keywords has-testing-info added

Steps to manually test

To manually test the original functions still work with output buffering, do these steps:

  • Add at least 3 images to the Media Library
  • Create a wp-content/mu-plugins/test.php file
  • Add this test code: 
    <?php

add_filter( 'the_content', 'test_45708' );
function test_45708( $content ) {
    ob_start();
    previous_image_link( false, '&larr; Previous Image' );
    $previous_image_link = ob_get_clean();

    ob_start();
    next_image_link( false, 'Next Image &rarr;' );
    $next_image_link = ob_get_clean();

    $nav = <<<HTML
    <nav id="image-navigation" class="navigation" role="navigation">
        <span class="previous-image">$previous_image_link</span>
                <span class="next-image">$next_image_link</span>
    </nav>
HTML;    

    return $content . $nav;
}
  • View one of the images by going to Media > Select an image > View

Notice the navigation links under the image:
← Previous Image Next Image →

@hellofromTonya
2 months ago

Test original functions work with output buffering for backompat. Results: Works as expected.

This ticket was mentioned in ​Slack in #core-media by hellofromtonya. ​View the logs.
2 months ago

#13 @hellofromTonya
7 weeks ago

  • Keywords commit added

This one is ready for commit @antpb

This ticket was mentioned in ​Slack in #core by hellofromtonya. ​View the logs.
7 weeks ago

#15 @whyisjake
7 weeks ago

  • Owner changed from hellofromTonya to whyisjake
  • Status changed from assigned to reviewing

This ticket was mentioned in ​Slack in #core by whyisjake. ​View the logs.
7 weeks ago

#17 @whyisjake
7 weeks ago

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

In 51122:

Media: Add new functions to return the previous/next attachment links.

New functions:

  • get_adjacent_image_link()
  • get_next_image_link()
  • get_previous_image_link()

Fixes #45708.

Props flixos90, DrewAPicture, mor10, antpb, hellofromTonya, whyisjake.

#18 @SergeyBiryukov
6 weeks ago

In 51172:

Tests: Make some optional parameters required in unit tests for previous/next attachment links.

This resolves a "Deprecated: Required parameter follows optional parameter" notice on PHP 8.

Follow-up to [48794], [51122].

See #45708, #52625.

Note: See TracTickets for help on using tickets.