WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#25268 closed defect (bug) (fixed)

Re-enable processing of scheduled and private posts in _fix_attachment_links()

Reported by: jond3r Owned by: nacin
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.3.2
Component: Media Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

The purpose of _fix_attachment_links() is to change attachment links in post content to its final "pretty" format, if non-default permalinks are enabled. The function is called late in the post update cycle, and for posts transitioned from draft to published, the attachment links are fixed by calling get_attachment_link() with the post in a 'publish' status. While a post is in draft status, get_attachment_link() will return permalinks of the form http://mysite.com/?attachment_id=xxx, and when the post is published, get_attachment_link() will typically return something like http://mysite.com/parent-post-slug/attachment-slug/, i.e. in a pretty format.

At present however, _fix_attachment_links() will return prematurely, without accomplishing its task, for scheduled post and private posts. This is caused by the "sanity check" early up in the function which returns if pretty permalinks are not enabled or the post_status is not 'publish'. This sanity check was introduced in [20308] (v. 3.3.2), ticket #13429 together with a major revamp of the function.

The proposed attached patch modifies the sanity check to ignore posts of type 'attachment', and with a status of 'draft', 'pending' or 'auto-draft'. This latter check is the same as done in get_permalink() to determine the type of link to return (pretty or not).

There are some other post statuses that one might suspect being affected by this patch. Attachments have a status of 'inherit', and are explicitly excluded in the sanity check. Revisions also have a status of 'inherit', but will never end up in _fix_attachment_links(). The same applies for trashed posts, with a status of 'trash'. Thus only posts with a status of 'publish', 'private' or 'future' are let through the sanity check.

Example procedure how to reproduce for scheduled posts:

  1. Ensure that non-default permalinks are chosen in the Permalink Settings
  2. Create a new post, and add a title.
  3. Add an unattached image to the content, and ensure that the Link To drop-down shows "Attachment Page". (The image is unattached if the URL in the text field below the drop-down contains "/?attachment_id=").
  4. Schedule the post some minute in the future by clicking "Edit" next to "Publish immediately". Press OK.
  5. Press "Schedule".
  6. Observe that the attachment link URL is not changed in the content area (it still contains a "?"). The same applies when viewing the post after (and before) the post is transitioned to published.

Contrast this to the behavior when the post is published immediately. Then the attachment link is changed (prettified) when "Publish" is pressed.

Though not explicitly tested, I'm quite sure this is a regression introduced in [20308].

The function _fix_attachment_links() dates back to [3145] (v. 2.0.0), ticket #1870, under the name fix_attachment_links() in the now deprecated file admin-functions.php. In [5542] (v. 2.2.1) it was moved to /includes/post.php, and the function name was prepended by an _.

The patch also changes the @since tag to 2.0.0 to reflect this, and fills in the missing type info for @param and @return. Possibly the @since tag should be 2.2.1?

The material for this this ticket emerged in the process of refreshing #22679.

Attachments (2)

25268.diff (1.4 KB) - added by jond3r 19 months ago.
25268-2.diff (1.3 KB) - added by jond3r 19 months ago.
Whitelist publish-like statuses

Download all attachments as: .zip

Change History (8)

@jond3r19 months ago

comment:1 @azaozz19 months ago

25268.diff looks good. It will be really really helpful to have few testcases for this.

In short: _fix_attachment_links() has to run after the post's permalink is finalized. This happens when the post is either published or scheduled or set as private. Thinking it would be better to "whitelist" the publish or equivalent post_status rather than "blacklist" draft, auto-draft and pending.

comment:2 @azaozz19 months ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 3.7

comment:3 @azaozz19 months ago

Sry, wrong ticket, disregard :)

Last edited 19 months ago by azaozz (previous) (diff)

@jond3r19 months ago

Whitelist publish-like statuses

comment:4 @jond3r19 months ago

The reason for blacklisting draft-like statuses in the original patch was to mimic the check done in get_permalink(), which ultimately determines whether the attachment permalink is prettified. However, the whitelisting approach works equally well as long as there are no new publish-like statuses introduced.

I'll try to come up with a few unit tests, but don't expect anything very soon.

comment:5 @nacin18 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 25697:

Run _fix_attachment_links() when post_status is future or private in addition to publish.

Fixes regression caused by [20308].

props jond3r.
fixes #25268.

comment:6 @jond3r18 months ago

Great to have this fixed!
Re: Unit tests. For my part, they will not happen for at least a couple of more months, I'm afraid.

Note: See TracTickets for help on using tickets.