get_pages: post_modified_gmt value for sort_column/orderby silently dropped

[david.binda]

With the refactoring of the get_pages function to internally use the WP_Query ( in r55569 ) the post_modified_gmt value for sort_column param (being translated to WP_Query's orderby) is no longer supported and as unsupported it gets silently dropped leading to different order of returned pages if used.

While the backward incompatibility has been brought up in the original ticket it has not been addressed, not even mentioned in the related ​dev note.

Shall the order by post_modified_gmt be backported to the new implementation?

[joemcgill]

Thanks for raising, @davidbinda! I'm adding the performance focus, to aid in a speedy review.

[azaozz]

Thinking this regression would warrant a revert of [55569]? Will also fix #59224.

Also see https://core.trac.wordpress.org/ticket/59224#comment:2. There is usually a good reason why very old tickets that received attention were not fixed at the time. Unfortunately the reasons often are not spelled out in the comments, assuming that may be to "spear the contributor's feelings" at the time, perhaps?

Revisiting such old tickets and deciding to "just go for it" usually carries a lot more risks as now all code has "adjusted" to work with/around any quirks there, and backwards compatibility may be pretty hard to achieve.

[spacedmonkey]

I think that reverting would not be a good idea. As this was part of WP 6.3 and has a dev note and people would also expect filters from WP_Query to now run now. Reverting this would be a breaking change itself.

I think there if a workable fix for this. I will take a look when I am back to work. This was missed as their was no unit test for it, any fix must contain a unit test.

[azaozz]

Replying to spacedmonkey:

I think that reverting would not be a good idea. As this was part of WP 6.3 and has a dev note and people would also expect filters from WP_Query to now run now.

Umm, the filters from WP_Query running for pages is unexpected and might be considered a regression in itself :)

The fact that there was a dev. note doesn't magically "fix" all plugins to comply with the changes. It simply informs extenders about new functionality that is available. Many plugins will most likely never change to use that functionality. Also dev. notes cannot be considered a "replacement" for backwards compatibility.

Reverting this would be a breaking change itself.

WP 6.3 was released few days ago. It would be much easier for the (few) plugins that might have adopted the new functionality to adapt.

I think there if a workable fix for this.

Could be. Looking at [55569] there is a big chunk of red (deleted) code starting at line 6072. Guessing this regression comes from deleting that code and not providing backwards compatibility for it. There may also be other regressions from that which will eventually show up.

[flixos90]

I think both this ticket and #59224 can quite reasonably be fixed, so I don't think a revert is appropriate here. This was indeed a notable oversight on the original commit, but that doesn't mean we can't fix it.

I think we'll need to add "translation layer" logic into get_pages(), to ensure that all the orderby values which were supported by get_pages() but aren't supported by WP_Query are properly translated (post_modified_gmt is not the only one as far as I can tell).

Looking at the two lists, I don't think any of the values is truly unsupported by WP_Query, it's more about adjusting the names, for example post_modified_gmt and modified_gmt should map to modified so that it works in WP_Query (the GMT value will be sorted just the same as the non-GMT value since they are just in a different timezone). It's trivial to add such logic, so let's do it for all previously supported orderby fields.

[azaozz]

Replying to flixos90:

I think both this ticket and #59224 can quite reasonably be fixed, so I don't think a revert is appropriate here.
...
I think we'll need to add "translation layer" logic into get_pages(), to ensure that all the orderby values which were supported by get_pages() but aren't supported by WP_Query are properly translated (post_modified_gmt is not the only one as far as I can tell).

Yep, a "translation layer" would probably work. But wouldn't adding that layer basically mean the old code is recreated so the refactoring is totally useless? That will likely negate any minuscule speed improvements that were gained from that refactoring, and at the same time might still miss other possible regressions. Also see above about unexpectedly running more filters for pages now. That can be considered a regression too.

Looking at the two lists, I don't think any of the values is truly unsupported by WP_Query, it's more about adjusting the names, for example post_modified_gmt and modified_gmt should map to modified so that it works in WP_Query (the GMT value will be sorted just the same as the non-GMT value since they are just in a different timezone). It's trivial to add such logic, so let's do it for all previously supported orderby fields.

Sure, sounds like that'd work for fixing the orderby. If you think it is better to keep fixing back-compat problems, the new code will also need all possible tests to confirm old functionality and quirks are still supported. Frankly I see few more places that look "suspicious" in that changeset. Keep in mind this code has been "fixed" many times and incrementally improved over 15 years :)

This PR reinstates the missing support for post_modified_gmt and modified_gmt as possible values for the sort_column key in get_pages().

These two are the only values that aren't anyway supported by WP_Query.

Trac ticket: https://core.trac.wordpress.org/ticket/59226

[david.binda]

Replying to flixos90:

I think we'll need to add "translation layer" logic into get_pages(), to ensure that all the orderby values which were supported by get_pages() but aren't supported by WP_Query are properly translated (post_modified_gmt is not the only one as far as I can tell).

When opening the ticket, I have specifically checked whether there are any other orderby params which have been dropped, and the post_modified_gmt is the only one missing. But might be worth double checking, of course :)

the GMT value will be sorted just the same as the non-GMT value since they are just in a different timezone

This is, IMHO, not correct. While the post_modified_gmt is always in the same no matter what timezone is currently set for the installation, the post_modified for new post changes whenever installation timezone is changed, but older posts are not affected. Thus the post_modified can differ upon timezone change. Can imagine it leading to issues e.g.: with day light saving time or a post published close to each other before and after timezone change (the new timezone would need to be further back in the offset that the difference in between the time of post published). But I must admit that I haven't really tested this.

That said, I believe that the post_modified_gmt orderby should be added to WP_Query for proper backward compatibility. But I'm not sure why it's not there already.

Thanks @joemcgill! Updated in ​https://github.com/WordPress/wordpress-develop/pull/5111/commits/1a00c192112fdf64307a793943114eda88eacf11

[flixos90]

Replying to david.binda:

When opening the ticket, I have specifically checked whether there are any other orderby params which have been dropped, and the post_modified_gmt is the only one missing. But might be worth double checking, of course :)

You're right, it's just that one (plus the unprefixed modified_gmt) :)

the GMT value will be sorted just the same as the non-GMT value since they are just in a different timezone

While the post_modified_gmt is always in the same no matter what timezone is currently set for the installation, the post_modified for new post changes whenever installation timezone is changed, but older posts are not affected. Thus the post_modified can differ upon timezone change. Can imagine it leading to issues e.g.: with day light saving time or a post published close to each other before and after timezone change (the new timezone would need to be further back in the offset that the difference in between the time of post published). But I must admit that I haven't really tested this.

That said, I believe that the post_modified_gmt orderby should be added to WP_Query for proper backward compatibility. But I'm not sure why it's not there already.

I am not opposed to this, though I think it would be more appropriate to implement that as a separate ticket as it would require a bit more effort and potentially additional discussion. Mapping modified_gmt to modified will work in the vast majority of cases, and it's a simple fix for the root problem (right now the order is completely different, while when using modified it would be the same in most scenarios).

We could first commit this fix, then open another ticket to implement post_modified_gmt support in WP_Query, and as part of that ticket remove the "translation" logic again, thus fixing it in an even better way. WDYT?

[SergeyBiryukov]

Replying to flixos90:

We could first commit this fix, then open another ticket to implement post_modified_gmt support in WP_Query, and as part of that ticket remove the "translation" logic again, thus fixing it in an even better way. WDYT?

That makes sense to me, reverting [55569] seems like an overkill at this point.

[joemcgill]

In 56490:

Posts, Post Types: Reinstate missing sort_column options in get_pages().

This fixes an issue introduced in [55569] that broke sort ordering by post_modified_gmt or modified_gmt.

Props david.binda, azaozz, spacedmonkey, flixos90, joemcgill.
Fixes #59226.

Committed in https://core.trac.wordpress.org/changeset/56490.

[joemcgill]

Reopening for inclusion in the 6.3 branch.

[flixos90]

In 56517:

Posts, Post Types: Reinstate missing sort_column options in get_pages().

This fixes an issue introduced in [55569] that broke sort ordering by post_modified_gmt or modified_gmt.

Props david.binda, azaozz, spacedmonkey, flixos90, joemcgill.
Merges [56490] to the 6.3 branch.
Fixes #59226.