WordPress.org

Make WordPress Core

Opened 17 months ago

Closed 16 months ago

Last modified 12 months ago

#22829 closed defect (bug) (fixed)

[gallery ids=] have no influence on adjacent_image_link() for attachment pages

Reported by: Bichareh Owned by: ryan
Milestone: 3.5 Priority: normal
Severity: normal Version: 3.5
Component: Gallery Keywords:
Focuses: Cc:

Description

Hi,

the "new" gallery from 3.5 RC5..

  1. Upload images in your article
  2. Create new gallery with them
  3. Then add some "old" existent images to this Gallery
  4. The output in the frontend is not working as it should be...you can't click straight through all images in this gallery.

e.g. http://dev.promicabana.de/?p=631
You can only click through the first two pictures or the last two...weird. But that's the code: [gallery ids="632,633,629,628"]

In other words: You can't add old pictures to any new gallery, otherwise it works not as it should be.

I hope you understand the problem, because my english is not the best.

Attachments (2)

22829.diff (710 bytes) - added by nacin 17 months ago.
22829.2.diff (737 bytes) - added by nacin 17 months ago.

Download all attachments as: .zip

Change History (31)

comment:1 scribu17 months ago

  • Milestone changed from Awaiting Review to 3.5

comment:2 follow-up: helenyhou17 months ago

  • Keywords close added

The gallery shortcode with IDs actually has no influence on the previous/next links on attachment pages. Not sure there's really a bug here, though the confusion is understandable.

comment:3 in reply to: ↑ 2 ; follow-up: DrewAPicture17 months ago

  • Keywords dev-feedback added

Replying to helenyhou:

The gallery shortcode with IDs actually has no influence on the previous/next links on attachment pages.

I think that's unfortunately kind of the point.

I kept seeing these forum posts on basically the same issue and started poking around in adjacent_image_link(). I came to the conclusion that there actually is a demonstrable difference from 3.4.2 behavior.

In 3.4.2, we had a form of "context" in the post parent. A post could only have one gallery so we could provide context to a media attachment page. Links to adjacent images would honor that context.

But in 3.5, the context isn't being transferred from the gallery shortcode which now has images' explicit IDs. This works fine and awesome in the post view. But once you click one of the gallery images and are sent to its attachment page, you're effectively outside the context of the post. (I'm sure it's more complicated than that, but that's the gist). At this point, you're given adjacent image links but there doesn't seem to be a "post" context anymore so much as "anything" context.

Looking at the permalinks, it's more obvious.

In 3.4.2, this is what my permalink would look like for a gallery image attachment page:

gallery-test-3-4-2/drew-picture

All gallery-attached images have gallery-test-3-4-2/ prepended, e.g. post context.

In 3.5, it looks like this:

attachments/drew-picture

It makes sense that there's no prepended slug for the parent here but if we want to restore previous behavior, we need to find a way to tie images together on attachment pages, outside the post context, without being able to rely on post_parent.

Last edited 17 months ago by DrewAPicture (previous) (diff)

comment:4 DrewAPicture17 months ago

So assuming what I said in comment:3 is sort of correct (and I could be way off the mark), seems like we need some kind of unique identifier to tie images together outside the context of the post.

comment:5 follow-up: helenyhou17 months ago

The shortcode is different. You could specify IDs before, there just wasn't a UI for it. If you use the [gallery] shortcode as before, without IDs, then it will only show what's attached to the post, and thus navigation between attachments of that post via attachment pages will be between the images displayed in the gallery. I don't see a difference in actual behavior, only an exposure of something that wasn't as easy to accomplish as before (specifying IDs).

Attachments are a post type. The post they are attached to is stored as the post_parent. adjacent_image_link() is basically like adjacent_post_link(), except with a post_parent context to relate within the set.

comment:6 in reply to: ↑ 5 DrewAPicture17 months ago

Replying to helenyhou:

If you use the [gallery] shortcode as before, without IDs, then it will only show what's attached to the post, and thus navigation between attachments of that post via attachment pages will be between the images displayed in the gallery.

Using and reproducing behavior from the vanilla [gallery] shortcode isn't at issue. This concerns new-style galleries that can have both attached and unattached images in them.

In 3.5 we can't rely on post_parent because we can put any image in any gallery anytime, regardless of parentage.

Using [gallery] will produce the same results as in 3.4.2, but if you're using any unattached images in a gallery, the behavior will not be the same.

comment:7 DrewAPicture17 months ago

  • Keywords dev-feedback removed

From IRC:

 <DrewAPicture> And did the adjacent links work with unattached images? Because adjacent_image_link() has always relied on testing for post_parent
 <helenyhou> nope
 <helenyhou> the behavior isn't changed, is what i'm getting at. weird or not is not my current focus :)
 <DrewAPicture> Ahh :) So it's not a change in behavior, people just are now realizing it.
 <helenyhou> right, it's just an exposed behavior
 * DrewAPicture puts foot in mouth

Based on the several forum posts (and tickets) we've seen covering this, sounds like it would be worth looking at a solution for this in the future.

On that note, +1 for close :)

comment:8 follow-up: markjaquith17 months ago

In order to fix this in the way you're expecting, we'd need something in the URL to indicate context. This is tricky, because galleries are essentially transient. We'd have to pass in info about the gallery when clicking to attachment URLs from within a gallery context. Flickr handles this with URLs like /in/photostream/ and /in/set-{set_id}/. We'd have to do something similar, probably including some sort of way to hash gallery ID lists and store them for reverse lookup.

comment:9 in reply to: ↑ 8 ; follow-up: knutsp17 months ago

  • Cc knut@… added

Replying to markjaquith:

We'd have to do something similar, probably including some sort of way to hash gallery ID lists and store them for reverse lookup.

Just a thought:

What about simply include query ids=1,2,3,4,5 as part of the attachment link? In this way the gallery array will be saved inside the post content. Like:

<a href="http://example.com/attachments/my-picture/?ids=1,2,3,4,5"><img .../></a>

Then let get_adjacent_post() detect this "ids" parameter and override the default behaviour.

New ticket, I guess. This as maybelater?

comment:10 in reply to: ↑ 9 mdgl17 months ago

Replying to knutsp:

What about simply include query ids=1,2,3,4,5 as part of the attachment link?

This is a bit messy but looks workable. In the same way, we should probably handle the existing gallery "include" and "exclude" parameters as well as the new (in 3.5) "ids". The functions for next/previous image link could default to using post_parent if these parameters are not present.

Alternatively we need to start viewing a "gallery" as an object in its own right and implement as a new post type (see comment:ticket:22085:1). I was a bit disappointed there wasn't more discussion or explanation about the planned changes to galleries when 3.5 started.

comment:11 in reply to: ↑ 3 nacin17 months ago

Replying to DrewAPicture:

Looking at the permalinks, it's more obvious.

In 3.4.2, this is what my permalink would look like for a gallery image attachment page:

gallery-test-3-4-2/drew-picture

All gallery-attached images have gallery-test-3-4-2/ prepended, e.g. post context.

In 3.5, it looks like this:

attachments/drew-picture

That's only accurate for images that were not uploaded ("attached") to a post. In 3.4.2, they *couldn't* be added to a gallery. In 3.5, they can.

This entire bug report is based on the new custom galleries not going along with adjacent_image_link(), previous_image_link(), next_image_link(). These functions are used in Twenty Ten, Twenty Eleven, *and* Twenty Twelve. It's simply something we didn't prepare for.

Not to brush this use case off entirely, but I personally find attachment pages a waste that shouldn't exist for 95% of users, and clicking through them one-by-one as a terrible experience. Carousels — which can support these custom galleries — are much better off.

I think we could solve this by implementing a /gallery/ endpoint on a post. Then it would take either an attachment ID (which could be duplicated) or an index (which could change), and we would parse post_content to figure out what is previous/next. We can have the endpoint essentially serve attachment pages under the hood, but avoid the implications of ugly URLs trying in vain to pass state around.

See also #22806, which I punted. This does not seem feasible to fix 3.5.0. We should however:

  • Add a filter to adjacent_image_link(), now.
  • Come up with a clever cookie-based solution (perhaps?) as a potential hotfix.

Also note that before 3.5, the gallery shortcode allowed any arbitrary ID in the "include" shortcode. Not saying that was used often — or ever — but we're technically just exposing more UI this release, that's about it. But, "exclude" was fairly commonly used, and that *wouldn't* work to exclude images from the prev/next images loop.

Also note that in 3.5, we're not forcing anyone to use this approach. I think we could handle most of this by A) defaulting to "Uploaded to this post" rather than all "Images" when creating a gallery, and B) attempt to save menu_order inside gallery sorting. We currently do menu_order saving inside "Uploaded to this post", so (B) might be tough to pull off without causing two different screens to compete with each other.

comment:12 nacin17 months ago

#22833 was marked as a duplicate.

comment:13 nacin17 months ago

  • Summary changed from Corrupt Gallery-Output to [gallery ids=] have no influence on adjacent_image_link() for attachment pages

comment:14 nacin17 months ago

#22806 was marked as a duplicate.

comment:16 follow-up: ryan17 months ago

We have similar problems with gallery previews in themes like Twenty Eleven. As Nacin mentions, these bugs have existed forever, but you had to edit the shortcode to notice them. I think these small teething pains are sufferable for now in exchange for the gallery flexibility we've all been wanting. Let's do a filter for 3.5 and work up a plugin that can become the basis for a fix in 3.6 (or 3.5.1 if the changes are small enough).

comment:17 in reply to: ↑ 16 westi17 months ago

Replying to ryan:

We have similar problems with gallery previews in themes like Twenty Eleven. As Nacin mentions, these bugs have existed forever, but you had to edit the shortcode to notice them. I think these small teething pains are sufferable for now in exchange for the gallery flexibility we've all been wanting. Let's do a filter for 3.5 and work up a plugin that can become the basis for a fix in 3.6 (or 3.5.1 if the changes are small enough).

+1

Completely agree with this, filter now and other stuff later.

comment:18 sabreuse17 months ago

+1 filter

nacin17 months ago

comment:19 markjaquith17 months ago

Another vote for a filter. This is not a new problem. It's just more obvious now that galleries are more flexible.

nacin17 months ago

comment:20 helenyhou17 months ago

  • Keywords close removed

Filter looks sane to me.

comment:21 nacin17 months ago

Another point: Galleries could have any order you pleased. Only "menu_order" galleries would allow for a sane previous/next. Title, date, random — fuggedaboutit.

comment:22 ryan17 months ago

I made a quick plugin and tested with a number of themes. Looks good.

comment:23 ryan17 months ago

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

In 23139:

Introduce previous_image_link and next_image_link filters. Allows customizing the gallery display order.

Props nacin
fixes #22829 for trunk

comment:24 ryan17 months ago

In 23140:

Introduce previous_image_link and next_image_link filters. Allows customizing the gallery display order.

Props nacin
fixes #22829 for 3.5

comment:25 jond3r17 months ago

  • Cc jond3r@… added

comment:26 Bichareh16 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

There is still no working solution for this. You still can't click through all images at a time... http://dev.promicabana.de/?p=652

comment:27 nacin16 months ago

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

This ticket has been closed for 3.5. You are happy to create a new ticket for potential future changes.

comment:28 SergeyBiryukov14 months ago

#23598 was marked as a duplicate.

comment:29 lancewillett12 months ago

Noting for future reference a user bug report with custom gallery order and Twenty Thirteen: http://wordpress.org/support/topic/next-and-previous-link-wrong-order-in-gallery

Note: See TracTickets for help on using tickets.