Make WordPress Core

Opened 17 years ago

Closed 19 months ago

Last modified 16 months ago

#10275 closed defect (bug) (wontfix)

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

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

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
17 years ago

  • Milestone Unassigned2.9

#2 @hakre
17 years ago

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

#3 @ryan
17 years ago

  • Milestone 2.9Future Release

#4 @hakre
15 years ago

  • Keywords needs-patch removed
  • Resolutionmaybelater
  • Status newclosed

#5 @ocean90
14 years ago

  • Milestone Future Release

#6 @Ninos Ego
12 years ago

  • Focuses template added
  • Resolution maybelater
  • Status closedreopened

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
12 years ago

  • Component GeneralThemes
  • MilestoneAwaiting Review

#8 @chriscct7
11 years ago

  • Keywords dev-feedback added

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


2 years ago
#9

  • Keywords has-patch added

#10 @desrosj
2 years ago

  • Keywords needs-testing added; dev-feedback removed
  • Milestone6.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.


2 years ago

#12 @sppramodh
23 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 23 months ago by sppramodh (previous) (diff)

#13 @sppramodh
23 months ago

  • Keywords has-testing-info added

#14 @desrosj
23 months ago

  • Milestone 6.76.8
  • Owner set to desrosj
  • Status reopenedreviewing

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

#15 @desrosj
20 months ago

  • Keywords close added
  • Owner desrosj removed

@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:


20 months ago
#16

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

#17 @sppramodh
20 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.


19 months ago

#19 @audrasjb
19 months ago

  • Milestone 6.8
  • Resolutionwontfix
  • Status reviewingclosed

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.

Version 0, edited 19 months ago by audrasjb (next)

#20 @wordpressdotorg
16 months ago

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