WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 6 months ago

#12821 assigned enhancement

Merge get_posts() and get_pages()

Reported by: mikeschinkel Owned by: garyc40
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: needs-testing needs-patch needs-unit-tests 3.8-early
Focuses: Cc:

Description (last modified by nacin)

get_pages() should wrap get_posts() the same way get_page() wraps get_post(). Arguments of different names need to be retained for back compat.

Reasoning for this includes #14823. Querying a nonhierarchical post type should still be allowed with child_of for example, to allow for cross-type relationships.

Attachments (4)

garyc40-12821-rev1.patch (9.9 KB) - added by garyc40 3 years ago.
first attempt at merging get_posts() and get_pages()
garyc40-12821-rev2.patch (10.8 KB) - added by garyc40 3 years ago.
restored "get_pages" filter hook, fixed bug introduced in rev1 where arguments are parsed incorrectly in wp_dropdown_pages()
12821.3.diff (10.7 KB) - added by garyc40 3 years ago.
depth is not an argument of get_pages() or get_posts()
post.diff (8.5 KB) - added by dbernar1 2 years ago.

Download all attachments as: .zip

Change History (50)

comment:1 nacin4 years ago

get_pages() should be hierarchical. get_posts() should be non-hierarchical.

comment:2 scribu4 years ago

Me thinks get_posts() should handle both. get_pages() should be a wrapper for get_posts().

comment:3 scribu4 years ago

...similar to how get_categories() is a wrapper for get_terms().

comment:4 filosofo4 years ago

+1 to scribu's suggestion

comment:6 follow-up: scribu4 years ago

  • Keywords needs-patch added
  • Milestone changed from Unassigned to 3.1

comment:7 in reply to: ↑ 6 mikeschinkel4 years ago

Replying to scribu:
I'd like to try to create this patch. I think the scope it defined small enough and straightforward enough that I could tackle it. This would give me the first patch beyond trivial for me to contribute.

If you think it's okay for me to take it on please assign to me.

comment:8 scribu4 years ago

Go for it. You can set it's status to "accepted".

Just remember that it probably won't make it into WP 3.0.

comment:9 nacin4 years ago

Go for it. In general, just find a ticket and write a patch if you'd like. I never found a need to mess with the owner components for the most part, though I find it helpful as a committer now.

We need to be fully back compat. The args order and sort_order (and orderby and sort_column) must both stay, for example. We'd want to make orderby and order the 'proper' ones since that is our naming convention. The structural differences between the two -- get_pages() relies on its own query, while get_posts() uses WP_Query, for example -- should be interesting to merge.

Just remember that it probably won't make it into WP 3.0.

Yeah, this isn't going to happen for 3.0. Way too much to change and test late in the dev cycle. Consider it blessed for 3.1 as of ryan's comment in #4711.

comment:10 mikeschinkel4 years ago

@scribu / @nacin

Thanks. Agree with full backward compatibility.

So for clarity we want to start using the parameters that get_posts() uses in addition to the ones get_pages() has always used so that get_pages() can behave like get_posts() but also so it can still behave the same as it used to.

As far as the two queries, do you know if there's any current incompatibilities between the two?

Understood about 3.1 vs. 3.0 and agree. 3.0 is ready to close out, not add new things to it.

Last question: Shouldn't I create a new ticket for the merge and close this one out?

comment:11 scribu4 years ago

So for clarity we want to start using the parameters that get_posts() uses in addition to the ones get_pages() has always used so that get_pages() can behave like get_posts() but also so it can still behave the same as it used to.

The other way around: add the get_pages() specific parameters ('child_of', 'exclude_tree' etc.) to get_posts(). Then, retrofit get_pages() to use get_posts().

Last question: Shouldn't I create a new ticket for the merge and close this one out?

Sure, just mention this one in the description.

comment:12 scribu4 years ago

Related: #12873

comment:13 kevinB4 years ago

  • Cc kevinB added

comment:14 scribu4 years ago

  • Milestone Awaiting Triage deleted
  • Resolution set to wontfix
  • Status changed from new to closed

comment:15 follow-up: nacin4 years ago

  • Description modified (diff)
  • Milestone set to 3.1
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from Allow get_pages() to query non-heirarchical post types? to Merge get_posts() and get_pages()

comment:16 in reply to: ↑ 15 ; follow-up: mikeschinkel4 years ago

  • Cc mikeschinkel@… added

Replying to nacin:

Would you like me to attempt to tackle this one? I think it's small enough scope, sufficiently along my expertise and devoid of debatable design decisions it might make a good project for me?

-Mike

comment:17 in reply to: ↑ 16 nacin4 years ago

Replying to mikeschinkel:

Would you like me to attempt to tackle this one? I think it's small enough scope, sufficiently along my expertise and devoid of debatable design decisions it might make a good project for me?

Go for it.

comment:18 mikeschinkel4 years ago

  • Owner set to mikeschinkel
  • Status changed from reopened to assigned

comment:19 aaroncampbell4 years ago

  • Cc aaroncampbell added

comment:20 sillybean4 years ago

  • Cc steph@… added

comment:21 voyagerfan57614 years ago

  • Cc WordPress@… added

comment:22 jane3 years ago

@mikeschinkel: any status on this? Freeze is a few days away, so posting a patch, even if it's not done yet, to get community feedback would be good, so you'd have time to revise based on feedback.

comment:23 jane3 years ago

  • Milestone changed from 3.1 to Future Release

Punting due to no patch and entering beta. @mikeschinkel: In general, don't bother asking if you should do a patch, just do the patch. People will put more attention on it if there's a patch to respond to.

comment:24 garyc403 years ago

  • Owner changed from mikeschinkel to garyc40

garyc403 years ago

first attempt at merging get_posts() and get_pages()

comment:25 garyc403 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

comment:26 kawauso3 years ago

  • Cc otterish@… added

comment:27 markoheijnen3 years ago

  • Cc info@… added

I looked at the patch and I noticed that the filter for get_pages is removed.
Removing this filter isn't handy since there are plugins that are using that filter.

If it is possible (have no clue) I would recommend to insert it with a deprecated notice.

garyc403 years ago

restored "get_pages" filter hook, fixed bug introduced in rev1 where arguments are parsed incorrectly in wp_dropdown_pages()

comment:28 grandslambert3 years ago

  • Cc shane@… added

I need to know if this issue will fix the following error when trying to use wp_dropdown_pages on a non-hierarchical custom post type:

Notice: Only variable references should be returned by 
reference in /var/www/wpblabber.lan/wp-includes/post.php on line 3313

That seems to be something that needs to be looked at as well. This only shows when debug mode is on, but when using wp_dropdown_pages on non-hierarchical post types, no dropdown appears.

Before anyone complains about this not being related, please see ticket ##14823

comment:29 scribu3 years ago

In garyc40-12821-rev2.patch, in get_posts(), is it really necessary to add the default args from get_pages()? Won't those be set in WP_Query anyway?

comment:30 follow-up: scribu3 years ago

What I mean is that get_posts() should remain unchanged.

Any get_pages() specific args, like 'include' should be converted to the standard ones ('post__in') inside get_pages(), before calling get_posts() or WP_Query.

Last edited 3 years ago by scribu (previous) (diff)

comment:31 in reply to: ↑ 30 garyc403 years ago

Replying to scribu:

What I mean is that get_posts() should remain unchanged.

Any get_pages() specific args, like 'include' should be converted to the standard ones ('post__in') inside get_pages(), before calling get_posts() or WP_Query.

'include' is not a get_pages() specific argument. The inline documentation of get_posts() specifies that 'include' is an argument, as does our codex.

In my patch, get_pages() call get_posts(), so converting args like include inside get_pages() before passing into get_posts() would result into duplicate code.

Also, I think get_pages() should become a wrapper of get_posts(), in the sense that get_posts() should process arguments that are associated with hierarchical post types. In other words, whatever arguments that get_pages() can handle, get_posts() itself must also be able to handle.

There are indeed some non-standard arguments in get_pages() such as post_parent, sort_order, number etc. but as you can see in the patch, they all get mapped into standard arguments (parent, order, numberposts etc.) for get_posts().

Other arguments like child_of, depth should be supported by get_posts() in case the developer wants to use it with a hierarchical post type.

comment:32 follow-up: scribu3 years ago

Ok, but 'child_of' and 'depth' will actually be handled in WP_Query, no? So it's redundant to set them as defaults in get_posts().

comment:33 in reply to: ↑ 32 garyc403 years ago

Replying to scribu:

Ok, but 'child_of' and 'depth' will actually be handled in WP_Query, no? So it's redundant to set them as defaults in get_posts().

Actually depth is an argument for wp_dropdown_pages(), so I'm removing it from get_posts() in an upcoming patch.

But there are other arguments such as offset, meta_key, meta_value etc. which are being set as defaults in get_posts() but are also checked and handled in WP_Query. Putting child_of as a default there does no harm and simply makes it more obvious which arguments and default values a WP developer can use with get_posts(). That being said, I don't really mind if it's considered redundant and gets removed.

I'm more concerned with whether there's any wrong assumption I made when merging the two functions. I tested the patch and it seems like it returns the correct results. But there might be edge cases of get_pages() that I don't know about, let alone performance issues.

garyc403 years ago

depth is not an argument of get_pages() or get_posts()

comment:34 kevinB3 years ago

Please remember to include 'suppress_filters' => false in the get_posts() args.

comment:35 wlindley3 years ago

On line 343 you set the 'order_by' attribute (with underline) but note that the routine actually uses 'orderby' (no underline) (see line 1334).

Also, the 'sort_order' attributes to get_pages() differ from the 'orderby' attributes of get_posts() -- the original get_pages() needs, for example, ('sort_order' => 'post_date') while the equivalent in get_posts() would be ('orderby' => 'date')... but only for certain columns (date, modified, author, name, title; but not menu_order, ID, etc.) The exact logic is in this code, file wp-includes/query.php in get_posts():

         switch ( $orderby ) {
                 case 'menu_order':
                         break;
                 case 'ID':
                         $orderby = "$wpdb->posts.ID";
                         break;
[...other special cases...]
                 default:
                         $orderby = "$wpdb->posts.post_" . $orderby;
         }

That case-specific test for "ID" should probably change, too, surely 'id' should work as well?

comment:36 dbernar12 years ago

Hello,

I've removed duplication between WP_Query::get_posts() and get_pages(), while preserving the unique interface get_pages() provides. I've not pulled in any of the unique functionality of get_pages() into WP_Query::get_posts() at this time. Please help me understand which functionality I should pull into WP_Query, and what interface I should provide.

I will then implement that, and perform testing on the code to ensure it works as expected.

I am providing the following patch as a reference for anyone who would like to read the code and provide the feedback I need, do not rely on it to work at this time - it has not been tested.

dbernar12 years ago

comment:37 kawauso2 years ago

  • Cc otterish@… removed

comment:38 follow-up: scribu2 years ago

Eliminating the duplication, but keeping the hierarchical parts inside get_pages() seems like the right way to go.

TODO extract this to something like is_a_hierarchical_post_type( $post_type )

We already have is_post_type_hierarchical()

comment:39 in reply to: ↑ 38 dbernar12 years ago

Replying to scribu:

Eliminating the duplication, but keeping the hierarchical parts inside get_pages() seems like the right way to go.

TODO extract this to something like is_a_hierarchical_post_type( $post_type )

We already have is_post_type_hierarchical()

Thank you very much. I could even leave the further evolvement of get_pages() for later, just provide what I have after testing as a first step.

Also thanks for the tip about is_post_type_hierarchical. I saw another place in the code where it wasn't being used, and I might provide another patch for that after.

Great to be collaborating with you, scribu.

comment:40 scribu2 years ago

Thank you very much. I could even leave the further evolvement of get_pages() for later, just provide what I have after testing as a first step.

You're welcome. You should upload an improved patch as soon as you have it; old versions will still be available, if needed.

comment:41 dbernar12 years ago

I made a note while refactoring the code:

"get_pages() had this additional sorting field. Figure out what to do about this one: $allowed_keys = array( 'modified_gmt', 'post_modified_gmt' );"

Basically, WP_Query::get_posts has:

"$allowed_keys = array('name', 'author', 'date', 'title', 'modified', 'menu_order', 'parent', 'ID', 'rand', 'comment_count');" on line 2332

which means it does not support post_modified_gmt, from what I can tell. What shall we do about that?

comment:42 scribu2 years ago

We should make WP_Query support ordering by 'modified_gmt' and 'date_gmt', while we're at it.

Version 0, edited 2 years ago by scribu (next)

comment:43 Chouby2 years ago

  • Cc frederic.demarle@… added

comment:44 andrewsfreeman22 months ago

  • Cc andrewsfreeman added

comment:45 brokentone8 months ago

  • Cc kenton.jacobsen@… added

comment:46 atimmer6 months ago

  • Keywords needs-patch needs-unit-tests 3.8-early added; has-patch removed

We really need unit tests before we change this around. Patch needs refresh + a lot of expanding.

I will be working on this, goal will be 3.8.

Note: See TracTickets for help on using tickets.