Make WordPress Core

Opened 16 years ago

Closed 3 months ago

Last modified 2 days ago

#10275 closed defect (bug) (wontfix)

Filter logic has been put into the template loader while it not belongs there.

Reported by: hakre's profile hakre Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.8
Component: Themes Keywords: has-patch needs-testing has-test-info close
Focuses: template Cc:

Description

Some time ago, filter logic has been introduced in the template-loader. Looks like a fix for the inability to handle attachments propperly (WP misses more and more a strict request parsing so that newer features tend to introduce more and more bugs).

The code has not been removed yet. It should be removed there or put into a more appropriate location.

The code in question is:

remove_filter('the_content', 'prepend_attachment');

in /wp-inclueds/template-loader.php around line 30.

Change History (20)

#1 @Denis-de-Bernardy
16 years ago

  • Milestone changed from Unassigned to 2.9

#2 @hakre
16 years ago

related: #6357 , code in question was introduced by ryan

#3 @ryan
15 years ago

  • Milestone changed from 2.9 to Future Release

#4 @hakre
13 years ago

  • Keywords needs-patch removed
  • Resolution set to maybelater
  • Status changed from new to closed

#5 @ocean90
13 years ago

  • Milestone Future Release deleted

#6 @Ninos Ego
11 years ago

  • Focuses template added
  • Resolution maybelater deleted
  • Status changed from closed to reopened

Is it planned to remove this filter in one of the next releases? I think this hack should be removed from code, otherwise you'll see the attachments image on the attachment page if you're using the the_content filter for compiling shortcodes in widgets or somewhere else.
For theme developers it should be very easy to add a new line of code to their single-attachment.php (or use map the attachment image to the post_thumbnail function). If you want I can publish a patch...

#7 @SergeyBiryukov
10 years ago

  • Component changed from General to Themes
  • Milestone set to Awaiting Review

#8 @chriscct7
9 years ago

  • Keywords dev-feedback added

This ticket was mentioned in PR #7321 on WordPress/wordpress-develop by @desrosj.


8 months ago
#9

  • Keywords has-patch added

#10 @desrosj
8 months ago

  • Keywords needs-testing added; dev-feedback removed
  • Milestone set to 6.7

It's not exactly clear what problem this addresses. But I can't find a scenario where prepend_attachment() is actually utilized in Core as a filter anymore.

In the latest version of Core (currently 6.6), it's added as a default filter for the_content, and only removed when is_attachment is the template file. But when the filter is called when displaying non-attachment posts, the function returns early.

A brief check of the plugin directory shows that plugins are either calling prepend_attachment() directly or adding it as a filter for their own custom hooks. So I don't expect anything to break related to 3rd-party usage.

I've opened a PR and removed the related code and default filter, and no tests broke. I'm moving this to the 6.7 milestone for a second set of eyes before committing.

This ticket was mentioned in Slack in #core-test by ankit-k-gupta. View the logs.


8 months ago

#12 @sppramodh
7 months ago

Test Report

This report validates that the changes in the patch do not introduce any issues and work as intended.

Patch tested: https://github.com/WordPress/wordpress-develop/pull/7321

Environments Tested

Environment 1

  • WordPress Playground
  • WordPress 6.7 (Patch applied)
  • PHP: 7.4.29
  • Browser: Google chrome 129.0.6668
  • Theme: Twenty Twenty-Four (Version 1.2)
  • Active Plugins:

Environment 2

  • WordPress Playground
  • PHP: 8.0
  • WordPress: 6.6.2
  • Browser: Google chrome 129.0.6668
  • Theme: Twenty Twenty-Four (Version 1.2)
  • Active Plugins:

Steps followed

1 Enable Attachment Pages:
Since attachment pages are fully disabled in WordPress 6.4 and later, the "Toggle wp_attachment_pages_enabled" plugin was used to enable attachment pages for testing.

2 Media Upload:
Uploaded an image to the Media Library.

3 View Attachment Page:

  • On WordPress 6.6.2 (without the patch), the image was visible on the attachment page.
  • On WordPress 6.7 (with the patch applied), the image did not appear on the attachment page.

screenshot from WordPress 6.7 patch applied

https://imgur.com/0EIYOXL

screenshot from WordPress 6.6.2

https://imgur.com/1RwiaxW

4 Behavior When Attachment Pages are Disabled (Default):
There was no change in how the images were displayed when viewing media items.

5 Custom Shortcode Testing:

  • Created a custom shortcode that utilizes the the_content filter for compiling shortcodes.
  • Developed a separate shortcode to display an attachment by ID.

6 Testing Shortcodes on the Header:
Added the custom shortcode and the attachment shortcode to the header, linking to a PDF (Download rates).

screenshot from WordPress 6.7 patch applied

https://imgur.com/DtjpMTa

screenshot from WordPress 6.6.2

https://imgur.com/esCp8X7

https://imgur.com/RqpVckj

7 Observations:

  • On WordPress 6.6.2, the attachment was automatically added to the header, appearing multiple times due to the the_content filter's effect when using shortcodes.
  • On WordPress 6.7 (with the patch), no such issue occurred on the attachment page. The attachment did not appear multiple times.

8 Compatibility Testing with WooCommerce:
Installed WooCommerce to verify if the shortcodes in the header caused any issues on WooCommerce pages.
No problems were found with WooCommerce active.

Actual Results

  • ✅ Issue resolved with patch.
Last edited 7 months ago by sppramodh (previous) (diff)

#13 @sppramodh
7 months ago

  • Keywords has-testing-info added

#14 @desrosj
7 months ago

  • Milestone changed from 6.7 to 6.8
  • Owner set to desrosj
  • Status changed from reopened to reviewing

6.7 RC1 is due out any moment. Unfortunately, this one missed the deadline so I'm going to punt this.

#15 @desrosj
4 months ago

  • Keywords close added
  • Owner desrosj deleted

@sppramodh Thanks for the testing you did! Based on the results, the PR is not the right approach as images will disappear from attachment pages in some scenarios. Can you share the shortcodes you were using to test?

I'm leaning towards closing this as a wontfix. This is such a long-standing "temporary" fix and the risks associated with changing this are probably not worth the trade off.

@desrosj commented on PR #7321:


4 months ago
#16

This results in images disappearing from single attachment pages in some themes, so not the right approach.

#17 @sppramodh
4 months ago

Sure, Please find the shortcode, i used for testing

function display_attachment_by_id( $atts ) {
    $atts = shortcode_atts( array(
        'id' => 0,
    ), $atts, 'attachment_id' );

    $attachment_id = intval( $atts['id'] );

    // Check if a valid attachment ID is provided
    if ( $attachment_id <= 0 ) {
        return 'Invalid attachment ID.';
    }

    // Get the attachment HTML markup (image, file link, etc.)
    $attachment_html = wp_get_attachment_link( $attachment_id );

    // If attachment is found, return the HTML, otherwise return a message
    return $attachment_html ? $attachment_html : 'Attachment not found.';
}

add_shortcode( 'attachment_id', 'display_attachment_by_id' );

Usage:

[attachment_id id="12"]

Let me know if further details or additional tests are needed!

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


3 months ago

#19 @audrasjb
3 months ago

  • Milestone 6.8 deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

As per today's bug scrub: Given the above discussion, decision was made to close this ticket as wontfix for now. Please feel free to reopen it if a new approach is discussed.

Last edited 3 months ago by audrasjb (previous) (diff)

#20 @wordpressdotorg
2 days ago

  • Keywords has-test-info added; has-testing-info removed
Note: See TracTickets for help on using tickets.