WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 2 months ago

#24257 closed enhancement (fixed)

is_attachment() should accept $attachment parameter

Reported by: alex-ye Owned by: wonderboymusic
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.5.1
Component: Query Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

Like is_single(),is_page(),is_author() ... etc I can't see any reason why we is_attachment() function accept $attachment parameter to make easy checks.

Attachments (2)

query.php.patch (1.5 KB) - added by alex-ye 12 months ago.
24257.diff (3.0 KB) - added by wonderboymusic 3 months ago.

Download all attachments as: .zip

Change History (13)

alex-ye12 months ago

comment:1 toscho12 months ago

  • Cc info@… added

comment:2 alex-ye12 months ago

  • Version set to 3.5.1

comment:3 follow-ups: wonderboymusic8 months ago

  • Keywords close added; has-patch removed

You aren't checking the post_type - nothing about that function is attachment-specific, it could evaluate true against an arbitrarily-passed post

comment:4 in reply to: ↑ 3 alex-ye8 months ago

  • Keywords has-patch added; close removed

Replying to wonderboymusic:

You aren't checking the post_type - nothing about that function is attachment-specific, it could evaluate true against an arbitrarily-passed post

I just make the patch like the is_page() function, We doesn't need to check the post-type since we pass an attachment ID, title, slug, or array of such.. so we don't need to check the Post object too.

You didn't give a reasons why the core doesn't add $attachment parameter.

comment:5 in reply to: ↑ 3 toscho8 months ago

Replying to wonderboymusic:

You aren't checking the post_type - nothing about that function is attachment-specific, it could evaluate true against an arbitrarily-passed post

The check for $this->is_attachment does exactly this. How could that return TRUE for another post type?

comment:7 alex-ye7 months ago

can we put this in WordPress 3.7 ?

wonderboymusic3 months ago

comment:8 wonderboymusic3 months ago

  • Milestone changed from Awaiting Review to 3.9

24257.diff​ refreshes and adds Unit Tests

comment:9 wonderboymusic3 months ago

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

In 27016:

Let is_attachment() accept an $attachment parameter, similar to is_page() and is_single(). Adds Unit Tests for all 3.

Props alex-ye for the initial patch.
Fixes #24257.

comment:10 nacin3 months ago

  • Keywords needs-unit-tests added
  • Resolution fixed deleted
  • Status changed from closed to reopened

I think this is solid. Could we add some assertions that also assert false? Just to make sure we don't have problems in the future. It might be nice to further assert things like $query->is_single/is_page/is_attachment (the property, not the methods), that the queried object matches the post that was created, etc.

Additionally, note that attachments are also is_single except in a single situation where it is instead is_page. It would be good to test is_singular(), is_singular and the respective is_(post|page) (methods and properties) in both truthiness and falseness.

comment:11 wonderboymusic2 months ago

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

In 27187:

Add some more assertions to Tests_Query_Conditionals, specifically for is_single(), is_page(), and is_attachment().

See [27016].
Fixes #24257.

Note: See TracTickets for help on using tickets.