WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#9726 closed defect (bug) (fixed)

wp_unique_post_slug() not used when inserting attachment from media / add new

Reported by: Denis-de-Bernardy Owned by: Denis-de-Bernardy
Milestone: 2.8 Priority: normal
Severity: normal Version: 2.8
Component: Upload Keywords: has-patch
Focuses: Cc:

Description

patch attached

Attachments (5)

9726.diff (1.4 KB) - added by Denis-de-Bernardy 6 years ago.
9726.2.diff (3.3 KB) - added by ryan 6 years ago.
9726.3.diff (4.2 KB) - added by Denis-de-Bernardy 6 years ago.
9726.4.diff (4.8 KB) - added by ryan 6 years ago.
9726.5.diff (4.5 KB) - added by ryan 6 years ago.

Download all attachments as: .zip

Change History (27)

@Denis-de-Bernardy6 years ago

comment:1 @Denis-de-Bernardy6 years ago

  • Keywords has-patch tested commit added

this is required to sort out #9539

comment:2 @hakre6 years ago

nice find.

comment:3 @ryan6 years ago

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

(In [11202]) Use wp_unique_slug() in wp_insert_attachment(). Props Denis-de-Bernardy. fixes #9726

comment:4 @azaozz6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This breaks the gallery as it allows identical slugs for attachments with different parents. Since posts are not hierarchical like pages, some of the rewrite rules depend on all attachments having unique slugs.

Is there any particular need for this patch?

comment:5 @azaozz6 years ago

(In [11461]) Revert [11202], see #9726

comment:6 @Denis-de-Bernardy6 years ago

before the patch, it was causing weirdness on static pages that shared their slug with attachments. #9539 has yet to be committed btw, and it's required concluding patch to make it all work as needed.

can you expand a bit on the issue you're getting so I can look into it?

comment:7 follow-up: @azaozz6 years ago

Perhaps the proper solution is to check attachment slugs against other attachments and also pages.

To reproduce with the patch: upload a file to a post; upload another file with the same name to different post; visit the attachment post for one of the files. Both attachment posts are displayed (may be theme dependent but happens in the default theme).

comment:8 in reply to: ↑ 7 ; follow-up: @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch tested commit removed
  • Owner set to Denis-de-Bernardy
  • Status changed from reopened to accepted

Replying to azaozz:

Perhaps the proper solution is to check attachment slugs against other attachments and also pages.

That's precisely what wp_unique_post_slug() is supposed to do. :-)

To reproduce with the patch: upload a file to a post; upload another file with the same name to different post; visit the attachment post for one of the files. Both attachment posts are displayed (may be theme dependent but happens in the default theme).

OK. Will give it a shot and nail it down.

comment:9 in reply to: ↑ 8 @azaozz6 years ago

Replying to Denis-de-Bernardy:

That's precisely what wp_unique_post_slug() is supposed to do. :-)

No, it only checks for posts with the same parent. Attachment slugs need to be unique globally.

comment:10 @Denis-de-Bernardy6 years ago

ah, ok. please restore 11202 then, I'll change wp_unique_post_slug accordingly during the afternoon and upload the patch.

comment:11 @Denis-de-Bernardy6 years ago

quick ref so I don't forget:

[11:58] <azaozz> as deleting the parent page will mess things up a lot..
[11:59] <ddebernardy> I believe the slug gets changed in case this results in a conflict
[12:00] <ddebernardy> will test that while I'm at it

comment:12 @ryan6 years ago

How about making unique against all posts/pages/attachments that are not draft or revision?

@ryan6 years ago

comment:13 @Denis-de-Bernardy6 years ago

dunno. I think the actual issue is two-fold:

  • the check we replaced in the initial patch for wp_unique_post_slug() is actually erroneous. the sql params differ from a check to the next
  • the check we replaced in this ticket's patch was yet another sql statement

uploading a new patch in a sec, it's still untested.

@Denis-de-Bernardy6 years ago

comment:14 @Denis-de-Bernardy6 years ago

  • Keywords has-patch added; needs-patch removed

@ryan6 years ago

comment:15 @ryan6 years ago

Patch makes unique across all post types and states and insulates slugs for published posts and pages from changes to uniqueness rules.

comment:16 @Denis-de-Bernardy6 years ago

yeah, but the trouble with this is, if you're creating a site that is static page oriented, and for some reason you've a slug that is shared between a static page and a post, 9726.4.diff would prevent the page from getting its preferred slug. That's not necessarily desirable. :-|

comment:17 @ryan6 years ago

I like that, but it is a deparature from past behavior.

Shouldn't attachments be unique across all types, though?

@ryan6 years ago

comment:18 @ryan6 years ago

.5 is based on .4 but modifies the attachment query to look across all types and statuses and adds some comments.

comment:19 @ryan6 years ago

Correction, .5 is based on .3, not .4.

comment:20 @Denis-de-Bernardy6 years ago

I'll give it a shot tomorrow. Still got 15 plugins in need of a rewrite over here. And finding bugs like #9944 along the way. ;-)

comment:21 @azaozz6 years ago

9726.5.diff is working well: attachments have globally unique slugs, child pages with different parents can have the same slug, and posts are unique among themselves.

comment:22 @ryan6 years ago

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

(In [11467]) wp_unique_post_slug() fixes. Props Denis-de-Bernardy. fixes #9726

Note: See TracTickets for help on using tickets.