Make WordPress Core

Opened 15 years ago

Closed 19 months ago

Last modified 17 months ago

#12821 closed enhancement (fixed)

Merge get_posts() and get_pages()

Reported by: mikeschinkel's profile mikeschinkel Owned by: spacedmonkey's profile spacedmonkey
Milestone: 6.3 Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: has-patch has-unit-tests needs-testing dev-feedback commit add-to-field-guide has-dev-note
Focuses: performance 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 (7)

garyc40-12821-rev1.patch (9.9 KB) - added by garyc40 14 years ago.
first attempt at merging get_posts() and get_pages()
garyc40-12821-rev2.patch (10.8 KB) - added by garyc40 14 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 14 years ago.
depth is not an argument of get_pages() or get_posts()
post.diff (8.5 KB) - added by dbernar1 13 years ago.
12821-wp-query.diff (9.3 KB) - added by mdawaffe 10 years ago.
WP_Query
Screenshot from 2023-02-07 19-27-53.png (9.7 KB) - added by spacedmonkey 22 months ago.
Before
Screenshot from 2023-02-07 19-28-03.png (10.1 KB) - added by spacedmonkey 22 months ago.
After

Download all attachments as: .zip

Change History (108)

#1 @nacin
15 years ago

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

#2 @scribu
15 years ago

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

#3 @scribu
15 years ago

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

#4 @filosofo
15 years ago

+1 to scribu's suggestion

#6 follow-up: @scribu
15 years ago

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

#7 in reply to: ↑ 6 @mikeschinkel
15 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.

#8 @scribu
15 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.

#9 @nacin
15 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.

#10 @mikeschinkel
15 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?

#11 @scribu
15 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.

#12 @scribu
15 years ago

Related: #12873

#13 @kevinB
14 years ago

  • Cc kevinB added

#14 @scribu
14 years ago

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

#15 follow-up: @nacin
14 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()

#16 in reply to: ↑ 15 ; follow-up: @mikeschinkel
14 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

#17 in reply to: ↑ 16 @nacin
14 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.

#18 @mikeschinkel
14 years ago

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

#19 @aaroncampbell
14 years ago

  • Cc aaroncampbell added

#20 @sillybean
14 years ago

  • Cc steph@… added

#21 @voyagerfan5761
14 years ago

  • Cc WordPress@… added

#22 @jane
14 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.

#23 @jane
14 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.

#24 @garyc40
14 years ago

  • Owner changed from mikeschinkel to garyc40

@garyc40
14 years ago

first attempt at merging get_posts() and get_pages()

#25 @garyc40
14 years ago

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

#26 @kawauso
14 years ago

  • Cc otterish@… added

#27 @markoheijnen
14 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.

@garyc40
14 years ago

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

#28 @grandslambert
14 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

#29 @scribu
14 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?

#30 follow-up: @scribu
14 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 ('postin') inside get_pages(), before calling get_posts() or WP_Query.

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

#31 in reply to: ↑ 30 @garyc40
14 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.

#32 follow-up: @scribu
14 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().

#33 in reply to: ↑ 32 @garyc40
14 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.

@garyc40
14 years ago

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

#34 @kevinB
14 years ago

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

#35 @wlindley
14 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?

#36 @dbernar1
13 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.

@dbernar1
13 years ago

#37 @kawauso
13 years ago

  • Cc otterish@… removed

#38 follow-up: @scribu
13 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()

#39 in reply to: ↑ 38 @dbernar1
13 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.

#40 @scribu
13 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.

#41 @dbernar1
13 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?

#42 @scribu
13 years ago

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

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

#43 @Chouby
13 years ago

  • Cc frederic.demarle@… added

#44 @andrewsfreeman
12 years ago

  • Cc andrewsfreeman added

#45 @brokentone
11 years ago

  • Cc kenton.jacobsen@… added

#46 @atimmer
11 years 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.

@mdawaffe
10 years ago

WP_Query

#47 @mdawaffe
10 years ago

Not knowing about this ticket or the previous work, I created a patch that makes get_pages() a wrapper for WP_Query (as get_posts() is already).

Oddities in attachment:12821-wp-query.diff​:

  • Having to use array_intersect_key() on the arguments since wp_dropdown_pages() has a name argument which is not compatible with WP_Query's. There are other ways to resolve this.
  • Lack of post_modified sort in WP_Query.
  • As written in this patch, the cacheing is handled a bit differently. I think the post terms/meta are cached with this patch, but not in the current version. We could change that with the update_post_meta_cache and update_post_term_cache WP_Query arguments.

#48 @helen
10 years ago

#32077 was marked as a duplicate.

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


9 years ago

#50 @benjibee
5 years ago

Attempting to maintain the order of IDs as given and ran into this strange decade-old behavior. Using get_posts() with post__in works as expected, I think it's time for this fix, no?

<?php
$args = array(
    // 'post__in' => array(1, 2 ,3),   // doesn't work at all, ok…
    'include' => array(1, 2 ,3),       // let's try it with this aliased parameter
    'orderby' => 'post__in'            // ignored
);
$pages = get_pages($args);             // posts sorted by default `orderby` value of 'DESC'

#51 @johnbillion
3 years ago

  • Keywords 3.8-early removed

#52 @peterwilsoncc
3 years ago

#55806 was marked as a duplicate.

#53 @peterwilsoncc
3 years ago

  • Focuses performance added
  • Milestone set to Awaiting Review

Putting this back on the review list following @spacedmonkey's renewed interest.

This ticket was mentioned in PR #3178 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#54

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

This ticket was mentioned in PR #3178 on WordPress/wordpress-develop by spacedmonkey.


2 years ago
#55

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

#56 @spacedmonkey
2 years ago

  • Keywords needs-patch needs-unit-tests added; has-patch has-unit-tests removed
  • Owner changed from garyc40 to spacedmonkey

@peterwilsoncc Started a patch at #3178.

#57 @spacedmonkey
2 years ago

  • Milestone changed from Awaiting Review to 6.2

#58 @mukesh27
2 years ago

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

@peterwilsoncc commented on PR #3178:


2 years ago
#59

I pushed up a PR with just the tests to compare the results with trunk.

I'm seeing a couple of minor differences in the new tests for the SQL queries but I think they may be false positives. As the legacy queries don't have ordering statements, there may end up been some unpredictable behavior across different MySQL versions/engines. Good times.

#60 @spacedmonkey
2 years ago

While working on this ticket, I found another important issues #57373. The issue is means that post objects are loaded twice when running unlimited queires. As in passing -1 and expecting all posts / pages in a query. This is specially a problem here, as a get_pages call to get all pages is called on every page requests. A solution for #57373 will need to found before committing this change.

#61 @spacedmonkey
2 years ago

  • Keywords needs-dev-note has-unit-tests added; needs-testing removed

#62 @spacedmonkey
2 years ago

  • Keywords needs-testing added

#63 @costdev
23 months ago

  • Keywords dev-feedback added

While #57373 has been committed, there is an ongoing discussion that needs to be resolved before this ticket is unblocked.

In the meantime, @ironprogrammer do you think we need some testing information for this ticket to help the Test Team, or is a before/after comparison of get_pages() calls with various arguments sufficient?

#64 @spacedmonkey
22 months ago

I don't think that #57373 is a blocker here.

#65 @flixos90
22 months ago

@spacedmonkey Is this something we can finalize before the 6.2 Beta 1 tomorrow? Otherwise this seems like a punt candidate.

#66 @spacedmonkey
22 months ago

  • Keywords commit added

@flixos90 Been approved. This is ready to commit.

#67 @flixos90
22 months ago

Thanks @spacedmonkey. I won't have time to review this, so I don't feel comfortable committing, particularly since this is such an old ticket.

Is this something that maybe you or @joemcgill feel comfortable doing?

#68 follow-up: @peterwilsoncc
22 months ago

#56689 also changed a function from using an SQL query to a wrapper for WP_Query. It was since reverted as it could cause an infinite loop if the function was called on a hook fired in WP_Query.

A similar risk exists for converting this function to a WP_Query wrapper, so my inclination is to wait until early in the 6.3 cycle before making this change. It seems these changes are riskier than first thought, so the early tag is appropriate.

#69 in reply to: ↑ 68 @spacedmonkey
22 months ago

Replying to peterwilsoncc:

#56689 also changed a function from using an SQL query to a wrapper for WP_Query. It was since reverted as it could cause an infinite loop if the function was called on a hook fired in WP_Query.

A similar risk exists for converting this function to a WP_Query wrapper, so my inclination is to wait until early in the 6.3 cycle before making this change. It seems these changes are riskier than first thought, so the early tag is appropriate.

I feel like pushing this doesn't really do anything than delay this discussion. Bug found in #56689 BECAUSE it was in core and tested. At any point, we going to have commit and handle the issues as they come up.

My votes is that we commit and if we do find issues, then can revert. Wonder the thoughts on @joemcgill @flixos90 @peterwilsoncc on this.

#70 @joemcgill
22 months ago

I agree with @peterwilsoncc that there is a back-compatibility risk with this change. However, I think it's ok to test it during the beta period since that still covers quite a bit of soak time and will likely give us better information if a revert during beta is needed.

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


22 months ago

#72 @costdev
22 months ago

  • Milestone changed from 6.2 to Future Release

This ticket was discussed in the pre-6.2 Beta 1 bug scrub. Moving to Future Release.

#73 @flixos90
22 months ago

@spacedmonkey Thanks for providing some benchmarks. Which type of content / URL is this data based on?

#74 @spacedmonkey
22 months ago

@flixos90 Home page, no object caching, 10 pages.

#75 @spacedmonkey
21 months ago

  • Milestone changed from Future Release to 6.3

#76 @spacedmonkey
21 months ago

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

In 55569:

Posts, Post Types: Use WP_Query internally in get_pages.

Convert get_pages to use WP_Query internally. Using WP_Query means that a lot of code has been removed however existing parameters supported by get_pages are transformed in to query arguments. The custom caching solution found in the old version of this function is replaced with the caching found in WP_Query (added in [53941]). This change adds consistency to the codebase, as improvements and changes to WP_Query will filter down to the get_pages function.

Props mikeschinkel, spacedmonkey, nacin, scribu, filosofo, jane, garyc40, markoheijnen, grandslambert, kevinB, wlindley, dbernar1, atimmer, mdawaffe, helen, benjibee, johnbillion, peterwilsoncc, costdev, flixos90, joemcgill.
Fixes #12821.

#78 follow-up: @marianne38
20 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hi,

The way get_pages() has been modified leads to a regression on our side.

With the previous method, we used a workaround by hooking on this filter :

apply_filters( 'get_pages', $pages, $parsed_args )

to filter the pages per language, see https://github.com/polylang/polylang/blob/3.3.2/include/filters.php#L193

We used to pass the language to get_pages() in the arguments like this :

get_pages( array( 'lang' => 'en' ) ).

We pass a taxonomy on the pages for the languages.
This new method with the WP_Query breaks the sorting of the pages by language when a language is passed as argument.

Because we filter the WP_Query requests by the current language, however if we add a 'lang' argument to get_pages(), we have to filter the pages by the language passed in argument and not the current one.

So it would be useful to carry the arguments received by get_pages() to the WP_Query, or to add a filter to filter the $query_args (with the $parsed_args).

#80 @spacedmonkey
20 months ago

@marianne38 The filter get_pages as not changed. The arguments are still the same.

Would adding a filter for the args passed to WP_Query be helpfully here?

#81 @marianne38
20 months ago

@spacedmonkey Here is a small test to see our issue :

<?php
        public function test_get_pages_with_taxonomy_on_page() {
                wp_set_current_user( 1 ); // For "assign_terms" access with "tax_input" in wp_insert_post().

                register_taxonomy( 'language', 'page' );

                $term_en = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'english' ) );
                $term_fr = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'french' ) );

                $page_en = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' =>  array( $term_en ) ) ) );
                $page_fr = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' =>  array( $term_fr ) ) ) );

                add_filter(
                        'get_pages',
                        function ( $pages, $args ) {
                                static $once = false;
                                if ( $once ) {
                                        return $pages;
                                }
                                $once = true; // Avoid infinite loop.

                                $r = array(
                                        'post_type'   => $args['post_type'],
                                        'post_status' => $args['post_status'],
                                        'fields'      => 'ids',
                                        'tax_query'   => array(
                                                array(
                                                        'taxonomy' => 'language',
                                                        'field'    => 'slug',
                                                        'terms'    => $args['language'],
                                                        'operator' => 'NOT IN',
                                                ),
                                        ),
                                );

                                $args['exclude'] = array_merge( wp_parse_id_list( $args['exclude'] ), get_posts( $r ) );
                                $numbered_pages  = get_pages( $args );
                                $pages           = ! $numbered_pages ? $pages : $numbered_pages;
                                return $pages;
                        },
                        10,
                        2
                );

                add_action(
                        'parse_query',
                        function ( $query ) {
                                if ( ! isset( $query->query_vars['language'] ) ) {
                                        $query->set( 'language', 'english' );
                                }
                        }
                );

                $pages = get_pages( array( 'language' => 'french' ) );
                $this->assertCount( 1, $pages );
                $this->assertSame( $page_fr, $pages[0]->ID );
        }

The get_pages() filter is a simplified version of what we did to sort the pages by language.

The action on parse_query represents our filter on the current language.

This test works with WP 6.2 and not with WP 6.3.

We had to do this because get_pages() was not working as get_posts().

Now that get_pages() uses a WP_Query, our pages are always filtered by the current language, even if we pass a language argument to get_pages() like this :

get_pages( array( 'language' => 'english' ) )

A possible solution would be if get_pages() could propagate the arguments it gets directly to the WP_Query, we wouldn't have to do anything on our side (we could do without the get_pages filter).

Or, as you suggested above, add a filter that would allow to filter the $query_args of the WP_Query in order to be able to propagate ourselves the language argument passed to get_pages().

#82 follow-up: @spacedmonkey
20 months ago

@marianne38 I think adding a filter is the only way forward here.

I will work on a PR, that allows for this query args to be filtered.

This would mean that existing sites would break after 6.3 without update on your end, correct?

#83 in reply to: ↑ 82 @marianne38
20 months ago

Replying to spacedmonkey:

@marianne38 I think adding a filter is the only way forward here.

I will work on a PR, that allows for this query args to be filtered.

This would mean that existing sites would break after 6.3 without update on your end, correct?

Thank you.

Yes, that's correct.

#84 @spacedmonkey
20 months ago

@marianne38 Is a filter enough to solve this?

#85 follow-up: @marianne38
20 months ago

@spacedmonkey Yes, the filter on query args is enough (with the $args received by get_pages() as parameter).

#87 in reply to: ↑ 85 ; follow-up: @spacedmonkey
20 months ago

Replying to marianne38:

@spacedmonkey Yes, the filter on query args is enough (with the $args received by get_pages() as parameter).

I put together a PR that adds a filter. See #4386.

#88 in reply to: ↑ 87 @marianne38
20 months ago

Replying to spacedmonkey:

Replying to marianne38:

@spacedmonkey Yes, the filter on query args is enough (with the $args received by get_pages() as parameter).

I put together a PR that adds a filter. See #4386.

Thank you.

It looks good to me.

#89 @spacedmonkey
19 months ago

@peterwilsoncc @flixos90 Do mind looking at https://github.com/WordPress/wordpress-develop/pull/4386. This adds a filter, so the plugins like polyglot can change WP_Query. It is a simple enough filter and should land before 6.3.

#90 @kenwins
19 months ago

The provided code includes a test function that demonstrates an issue with the get_pages() function in WordPress. The test involves registering a custom taxonomy called "language" and creating two pages assigned with different language terms. The objective is to retrieve pages based on the language argument passed to get_pages(). However, in WordPress 6.3, a change in the behavior of get_pages() results in pages being always filtered by the current language, disregarding the language argument provided to get_pages().

Proposed Solution:
To address this issue, the following solution is proposed:

Solution: Adding a Filter to Modify WP_Query Arguments
To allow for modifying the $query_args of the WP_Query instance used within get_pages(), we can introduce a filter. This filter enables us to intercept and modify the query arguments, including the language argument, before the query is executed.

Here's an example implementation of this solution:

public function test_get_pages_with_taxonomy_on_page() {
    wp_set_current_user( 1 );

    register_taxonomy( 'language', 'page' );

    $term_en = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'english' ) );
    $term_fr = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'french' ) );

    $page_en = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' =>  array( $term_en ) ) ) );
    $page_fr = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' =>  array( $term_fr ) ) ) );

    add_filter(
        'get_pages_query_args',
        function ( $query_args, $r ) {
            $query_args['tax_query'][] = array(
                'taxonomy' => 'language',
                'field'    => 'slug',
                'terms'    => $r['language'],
                'operator' => 'IN',
            );

            return $query_args;
        },
        10,
        2
    );

    add_action(
        'parse_query',
        function ( $query ) {
            if ( ! isset( $query->query_vars['language'] ) ) {
                $query->set( 'language', 'english' );
            }
        }
    );

    $pages = get_pages( array( 'language' => 'french' ) );

    $this->assertCount( 1, $pages );
    $this->assertSame( $page_fr, $pages[0]->ID );
}

In this solution, we introduce the get_pages_query_args filter, which allows us to modify the query arguments before executing the WP_Query inside get_pages(). We add a tax query to include pages with the specified language term. By implementing this solution, we can ensure that the language argument is correctly handled and respected in the get_pages() query. Note: This solution assumes that you have already updated to WordPress version 6.3, where the behavior of get_pages() has changed.
Solution 2: Add Filter to Modify WP_Query Arguments
Another solution involves introducing a filter that allows for modifying the $query_args array of the WP_Query instance used within get_pages(). By implementing this filter, we can intercept and modify the arguments, including the language argument, before the query is executed.

Here's an example implementation of this solution:

public function test_get_pages_with_taxonomy_on_page() {
    wp_set_current_user( 1 );

    register_taxonomy( 'language', 'page' );

    $term_en = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'english' ) );
    $term_fr = self::factory()->term->create( array( 'taxonomy' => 'language', 'name' => 'french' ) );

    $page_en = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' => array( $term_en ) ) ) );
    $page_fr = self::factory()->post->create( array( 'post_type' => 'page', 'tax_input' => array( 'language' => array( $term_fr ) ) ) );
add_filter(
    'get_pages_query_args',
    function ( $query_args, $r ) {
        $query_args['tax_query'][] = array(
            'taxonomy' => 'language',
            'field'    => 'slug',
            'terms'    => $r['language'],
            'operator' => 'IN',
        );

        return $query_args;
    },
    10,
    2
);

add_action(
    'parse_query',
    function ( $query ) {
        if ( ! isset( $query->query_vars['language'] ) ) {
            $query->set( 'language', 'english' );
        }
    }
);

$pages = get_pages( array( 'language' => 'french' ) );

$this->assertCount( 1, $pages );
$this->assertSame( $page_fr, $pages[0]->ID );
}

In this solution, we introduce the get_pages_query_args filter, which allows us to modify the query arguments before executing the WP_Query inside get_pages(). We add a tax query to include pages with the specified language term.

Both solutions aim to resolve the issue by ensuring that the language argument is correctly handled and respected in the get_pages() query. You can choose the solution that best suits your requirements and integrate it into your codebase accordingly.

@spacedmonkey commented on PR #4386:


19 months ago
#91

Oh, but +1 on adding a test to ensure that the expected args are passed to the filter and that it affects the resulting query.

@westonruter I am not personally a fan of adding unit tests for filters. Do you have a thoughts on the kind of unit tests I should add.

@spacedmonkey commented on PR #4386:


19 months ago
#92

Oh, but +1 on adding a test to ensure that the expected args are passed to the filter and that it affects the resulting query.

@westonruter I am not personally a fan of adding unit tests for filters. Do you have a thoughts on the kind of unit tests I should add.

@westonruter commented on PR #4386:


19 months ago
#93

If a unit test isn't added, how would we guard against us breaking the filter in the future?

I'd suggest something like adding filters to capture the original arguments and to make some mutation to the filtered value, and then add assertions to make sure that the expected arguments were passed and that the filtering made the expected change:

{{{php
$get_pages_query_args_filter_args = array();
add_filter(

'get_pages_query_args',
static function ( $query_args, $parsed_args ) use ( &$get_pages_query_args_filter_args ) {

$get_pages_query_args_filter_args = [ $query_args, $parsed_args ];
$query_argspost_type? = 'post'; Do something to affect the query.
return $query_args;

},
10,
2

);

$get_pages_filter_args = array();
add_filter(

'get_pages',
static function ( $pages, $parsed_args, $query_args ) use ( &$get_pages_filter_args ) {

$get_pages_filter_args[] = array( $pages, $parsed_args, $query_args );
$pages[] = self::factory()->post->create( array( 'post_type' => 'foo' ) );
return $pages;

},
10,
3

);

$pages = get_pages( array( 'number' => 1, 'post_type' => 'page' ) );

$this->assertCount( 2, $get_pages_query_args_filter_args );
$this->assertCount( 3, $get_pages_filter_args );
TODO: Add assertions for the array items of the above variables to ensure the expected shape is supplied.
$this->assertCount( 2, $pages );
$this->assertSame( 'post', $pages[0]->post_type, 'Expected get_pages_query_args filter to override post_type' );
$this->assertSame( 'foo', $pages[1]->post_type, 'Expected get_pages filter to add another post of type foo.' );
}}}

@spacedmonkey commented on PR #4386:


19 months ago
#94

@westonruter @felixarntz I have added a simple unit test to prove the filter works. I looked at adding more tests, but they all felt overkill.

#95 @spacedmonkey
19 months ago

In 55845:

Posts, Post Types: Add a new filter for query arguments in get_pages.

In [55569] get_pages was converted to use WP_Query internally. But for plugins that were extending the get_pages filters and filter WP_Query query arguments, this could result in a conflict. Add a filter get_pages_query_args to allow developers to change arguments passed to WP_Query but also have the context of the original arguments passed to the get_pages function.

This change also expands test coverage of get_pages to ensure no breakages in the future.

Props spacedmonkey, westonruter, costdev, flixos90, kenwins, marianne38.
See #12821.

#96 @spacedmonkey
19 months ago

@kenwins @marianne38 A new filter has been added ([55845]) to core called get_pages_query_args.

This should help. Can you please test and get back if you find any issue.

#98 @spacedmonkey
19 months ago

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

#99 in reply to: ↑ 78 @stevenlinx
17 months ago

  • Keywords add-to-field-guide added

#100 @spacedmonkey
17 months ago

#56660 was marked as a duplicate.

#102 @flixos90
17 months ago

  • Keywords has-dev-note added; needs-dev-note removed
Note: See TracTickets for help on using tickets.