Make WordPress Core

Opened 7 years ago

Last modified 19 months ago

#42005 new feature request

filter get_terms_args and orderby meta_value_num not working as expected

Reported by: fred_bdx's profile Fred_Bdx Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8.2
Component: Taxonomy Keywords: has-patch
Focuses: ui, administration Cc:

Description

This is a follow-up to #34996.

Hi,

this is working :

<?php
add_filter( 'get_terms_defaults', 'my_get_terms_defaults', 10, 2 );
function my_get_terms_defaults( $defaults, $taxonomies ) {
        if ( in_array( 'my_tax_types', $taxonomies ) ) {
                $defaults['orderby']            = 'meta_value_num';
                $defaults['meta_key']   = 'tax_position';
        }
        return $defaults;
}

this is working too (for example) :

<?php
add_filter( 'get_terms_args', 'my_get_terms_args', 10, 2 );
function my_get_terms_args( $args, $taxonomies ) {
        if ( in_array( 'my_tax_types', $taxonomies ) ) {
                $args['orderby']        = 'name';
                $args['order']          = 'DESC';
        }
        return $args;
}

BUT this is NOT working :

<?php
add_filter( 'get_terms_args', 'my_get_terms_args', 10, 2 );
function my_get_terms_args( $args, $taxonomies ) {
        if ( in_array( 'my_tax_types', $taxonomies ) ) {
                $args['orderby']        = 'meta_value_num';
                $args['meta_key']       = 'tax_position';
        }
        return $args;
}

As Samuel said here : https://wordpress.org/support/topic/filter-get_terms_args-and-orderby-meta_value_num-not-working/#post-9533065 , is it possible to move earlier in the process the get_terms_args filter ?

Because for now, the get_terms_defaults filter doesn't apply on all returned get_terms(), the get_terms_args filter does, but not accept orderby meta_value_num.

Thanks.

Change History (24)

#1 @JordanPak
3 years ago

I just debugged and traced through WP_Term_Query for a while to find this same issue: get_terms_args is too late for the orderby arg to be meta_value_num. I'm using get_terms_defaults for now as well, but if this can't be patched there should at least be a disclaimer in the get_terms_args that it won't work?

This ticket was mentioned in PR #3319 on WordPress/wordpress-develop by scruffian.


20 months ago
#2

  • Keywords has-patch added

It looks like this PR was missed in the backport:
https://github.com/WordPress/gutenberg/pull/42005

To test, you can follow the testing instructions on https://github.com/WordPress/gutenberg/pull/44363.

ockham commented on PR #3319:


20 months ago
#3

To test, you can follow the testing instructions on https://github.com/WordPress/gutenberg/pull/44363.

Thank you @scruffian!

I can confirm that the following:

Using a theme without theme.json (for example, Canape):

  • Verify that the styles for the pullquote and navigation blocks are still present in the global-styles-inline-css. These blocks are the only ones that use the __experimentalStyle API in its block.json. It should look like this:

{{{css
.wp-block-pullquote{font-size: 1.5em;line-height: 1.6;}
.wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;}
}}}

isn't present on trunk, but is on this branch ✅

ockham commented on PR #3319:


20 months ago
#4

I'll also rebase this quickly since a lot of stuff has been merged to trunk recently.

scruffian commented on PR #3319:


20 months ago
#5

Thanks for fixing @ockham

hellofromtonya commented on PR #3319:


20 months ago
#6

Self-assigning for commit review and prep.

hellofromtonya commented on PR #3319:


20 months ago
#7

@ockham @scruffian The original PR included adding 'blocks' to WP_Theme_JSON_6_1::VALID_ORIGINS constant. Should this constant also be changed in Core's WP_Theme_JSON ? I assume yes.

hellofromtonya commented on PR #3319:


20 months ago
#8

Also @scruffian @ockham What aboutthe changes made here in the original PR? Should these also be included?

hellofromtonya commented on PR #3319:


20 months ago
#9

@ockham @scruffian The original PR included adding 'blocks' to WP_Theme_JSON_6_1::VALID_ORIGINS constant. Should this constant also be changed in Core's WP_Theme_JSON ? I assume yes.

If yes, I can add it with some updates I'm doing in the branch.

hellofromtonya commented on PR #3319:


20 months ago
#10

Oh wait, I see. This PR is a partial and should be paired with PR #3313. Aha.

So with https://github.com/WordPress/wordpress-develop/pull/3319/commits/48c8feacb4b67daed3ddf7a026adfac2a1bfe1d3, this PR is now a complete backport of the original Gutenberg PR https://github.com/WordPress/gutenberg/pull/44363. This means PR #3313 is no longer needed. I'll close that one in favor of this one (since I already updated this one 🤣 ).

ockham commented on PR #3319:


20 months ago
#11

Thank you, @hellofromtonya! And sorry for the extra work -- we should've probably made clearer that this one was originally just to unblock https://github.com/WordPress/wordpress-develop/pull/3313 😅

hellofromtonya commented on PR #3319:


20 months ago
#12

No problem @ockham Took me a few minutes to realize - doh - what was going on. But now the two are consolidated for a complete backport 🎉 I'm currently looking at the tests and doing manual testing.

hellofromtonya commented on PR #3319:


20 months ago
#13

# Test Report

Env:

  • Localhost: wp-env
  • OS: macOS
  • Browser: Firefox
  • Plugins: none
  • Theme: TT2

## Test 1: Test that the styles are enqueued when the blocks are in use

### Instructions

  • Step 1: Create a page that uses the pullquote and navigation blocks and publish.
  • Step 2: View the page in the front-end
  • Step 3: Verify that the styles for the pullquote and navigation blocks are not part of the global-styles-inline-css.
  • Step 4: Verify that (a) there's a wp-block-pullquote-inline-css stylesheet and (b) contains .wp-block-pullquote{border-width: 1px 0;font-size: 1.5em;line-height: 1.6;}.
  • Step 5: Verify that (a) there's a wp-block-navigation-inline-css stylesheet and (b) it contains .wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;}.

### Results:

Expectations > Results:

  • global-styles-inline-css does not include styles for pullquote and navigation blocks > Verified ✅
  • <style id="wp-block-pullquote-inline-css">..</style> exists > Verified ✅
  • wp-block-pullquote-inline-css contains .wp-block-pullquote{border-width: 1px 0;font-size: 1.5em;line-height: 1.6;} > No, it does not ❌

<details>

<summary>Results: HTML</summary>

{{{html
<style id="wp-block-pullquote-inline-css">

/

  • Colors */

/

  • Breakpoints & Media Queries */

/

  • SCSS Variables. *
  • Please use variables from this sheet to ensure consistency across the UI.
  • Don't add to this sheet unless you're pretty sure the value will be reused in many places.
  • For example, don't add rules to this sheet that affect block visuals. It's purely for UI. */

/

  • Colors */

/

  • Fonts & basic variables. */

/

/

  • Dimensions. */

/

  • Shadows. */

/

  • Editor widths. */

/

  • Block & Editor UI. */

/

  • Block paddings. */

/

  • React Native specific.
  • These variables do not appear to be used anywhere else. */

/

  • Converts a hex value into the rgb equivalent.

*

  • @param {string} hex - the hexadecimal value to convert
  • @return {string} comma separated rgb values

*/
/

  • Breakpoint mixins */

/

  • Long content fade mixin *
  • Creates a fading overlay to signify that the content is longer
  • than the space allows. */

/

  • Focus styles. */

/

  • Applies editor left position to the selector passed as argument */

/

  • Styles that are reused verbatim in a few places */

/

  • Allows users to opt-out of animations via OS-level preferences. */

/

  • Reset default styles for JavaScript UI based pages.
  • This is a WP-admin agnostic reset */

/

  • Reset the WP Admin page styles for Gutenberg-like pages. */

.wp-block-pullquote {

margin: 0 0 1em 0;
padding: 3em 0;
text-align: center;
overflow-wrap: break-word;
box-sizing: border-box;

}
.wp-block-pullquote p,
.wp-block-pullquote blockquote,
.wp-block-pullquote cite {

color: inherit;

}
.wp-block-pullquote.alignleft, .wp-block-pullquote.alignright {

max-width: 420px;

}
.wp-block-pullquote cite,
.wp-block-pullquote footer {

position: relative;

}
.wp-block-pullquote .has-text-color a {

color: inherit;

}

.wp-block-pullquote.has-text-align-left blockquote {

text-align: left;

}

.wp-block-pullquote.has-text-align-right blockquote {

text-align: right;

}

.wp-block-pullquote.is-style-solid-color {

border: none;

}
.wp-block-pullquote.is-style-solid-color blockquote {

margin-left: auto;
margin-right: auto;
max-width: 60%;

}
.wp-block-pullquote.is-style-solid-color blockquote p {

margin-top: 0;
margin-bottom: 0;
font-size: 2em;

}
.wp-block-pullquote.is-style-solid-color blockquote cite {

text-transform: none;
font-style: normal;

}

.wp-block-pullquote cite {

color: inherit;

}
/

  • Colors */

/

  • Breakpoints & Media Queries */

/

  • SCSS Variables. *
  • Please use variables from this sheet to ensure consistency across the UI.
  • Don't add to this sheet unless you're pretty sure the value will be reused in many places.
  • For example, don't add rules to this sheet that affect block visuals. It's purely for UI. */

/

  • Colors */

/

  • Fonts & basic variables. */

/

/

  • Dimensions. */

/

  • Shadows. */

/

  • Editor widths. */

/

  • Block & Editor UI. */

/

  • Block paddings. */

/

  • React Native specific.
  • These variables do not appear to be used anywhere else. */

/

  • Converts a hex value into the rgb equivalent.

*

  • @param {string} hex - the hexadecimal value to convert
  • @return {string} comma separated rgb values

*/
/

  • Breakpoint mixins */

/

  • Long content fade mixin *
  • Creates a fading overlay to signify that the content is longer
  • than the space allows. */

/

  • Focus styles. */

/

  • Applies editor left position to the selector passed as argument */

/

  • Styles that are reused verbatim in a few places */

/

  • Allows users to opt-out of animations via OS-level preferences. */

/

  • Reset default styles for JavaScript UI based pages.
  • This is a WP-admin agnostic reset */

/

  • Reset the WP Admin page styles for Gutenberg-like pages. */

.wp-block-pullquote {

border-top: 4px solid currentColor;
border-bottom: 4px solid currentColor;
margin-bottom: 1.75em;
color: currentColor;

}
.wp-block-pullquote cite,
.wp-block-pullquote footer, .wp-block-pullquotecitation {

color: currentColor;
text-transform: uppercase;
font-size: 0.8125em;
font-style: normal;

}
</style>
}}}

</details>

  • <style id="wp-block-navigation-inline-css">..</style> exists > Verified ✅
  • wp-block-navigation-inline-css contains .wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;} > Verified ✅

<details>

<summary>Results: HTML</summary>

{{{html
<style id="wp-block-pullquote-inline-css">
.wp-block-navigation a:where(:not(.wp-element-button)){color: inherit;}
</style>
}}}
</details>

## Test 2: Test that the styles are not enqueued when the blocks are not in use:

### Instructions

  • Step 1: Create a page that does not use the pullquote and navigation blocks and publish.
  • Step 2: View the page in the front-end
  • Step 3: Verify that the styles for the pullquote and navigation blocks are not in the global-styles-inline-css styles.
  • Step 4: Verify there's no <style id="wp-block-pullquote-inline-css"> in the <head>.
  • Step 5: Verify there's no <style id="wp-block-navigation-inline-css"> in the <head>.

### Results

Expectations > Results:

  • global-styles-inline-css does not include styles for pullquote and navigation blocks > Verified ✅
  • <style id="wp-block-pullquote-inline-css">..</style> does not exist > Verified ✅
  • <style id="wp-block-navigation-inline-css">..</style> does not exist > Does exist but likely for the page navigation (not in-page content nav) ❌

## Test 3: Test that themes can opt-out from loading blocks separately even if the post uses the blocks:

### Instructions

  • Step 1: Add add_filter( 'should_load_separate_core_block_assets', '__return_false' ); to the functions.php of the theme.
  • Step 2: View the Test 1 page in the front-end.
  • Verify that styles for all blocks are part of the global-styles-inline-css stylesheet.

### Results

Expectations > Results:

  • global-styles-inline-css contains pullquote or wp-block-pullquote styles > Does not contain ❌
  • global-styles-inline-css contains wp-block-navigation styles > Verified ✅

hellofromtonya commented on PR #3319:


20 months ago
#14

@oandregal Can you double check this backport. Using the test instructions, notice the ❌ items (in my Test Report) that did not meet the expectations.

hellofromtonya commented on PR #3319:


20 months ago
#15

Committer Note: This PR is not yet ready for commit as it is not meeting the defined testing expectations.

oandregal commented on PR #3319:


20 months ago
#16

@hellofromtonya @ockham I can confirm I got the same results as Tonya when I tried this PR without the commit https://github.com/WordPress/wordpress-develop/pull/3319/commits/48c8feacb4b67daed3ddf7a026adfac2a1bfe1d3 that ported https://github.com/WordPress/gutenberg/pull/44363 So probably the root cause is not that.

I've noticed that the other PR we're porting https://github.com/WordPress/gutenberg/pull/42005 had a few follow-ups so I wonder if we're missing something else.

ockham commented on PR #3319:


20 months ago
#17

Thank you @hellofromtonya and @oandregal! Apologies, this has been on my list for a while, but I haven't gotten around to look into it yet.

I'll be AFK on Friday and Monday; maybe @c4rl0sbr4v0 and @michalczaplinski can help figure this out? 😊

ockham commented on PR #3319:


20 months ago
#18

Rebased.

ockham commented on PR #3319:


20 months ago
#19

@michalczaplinski Could you re-test this and see if the re-base fixed the items that didn't work ❌ for @hellofromtonya?

If things still don't work, also try with the following patch:

{{{diff
diff --git a/src/wp-includes/class-wp-theme-json-resolver.php b/src/wp-includes/class-wp-theme-json-resolver.php
index 344ad3ed99..0520fbd1d6 100644
--- a/src/wp-includes/class-wp-theme-json-resolver.php
+++ b/src/wp-includes/class-wp-theme-json-resolver.php
@@ -178,7 +178,7 @@ class WP_Theme_JSON_Resolver {

$options = wp_parse_args( $options, array( 'with_supports' => true ) );


  • if ( null === static::$theme ) {

+ if ( null === static::$theme ) {

$theme_json_data = static::read_json_file( static::get_file_path_from_theme( 'theme.json' ) );
$theme_json_data = static::translate( $theme_json_data, wp_get_theme()->get( 'TextDomain' ) );
/

@@ -203,7 +203,7 @@ class WP_Theme_JSON_Resolver {

$parent_theme->merge( static::$theme );
static::$theme = $parent_theme;

}

  • }

+ }

if ( ! $optionswith_supports? ) {

return static::$theme;

}}}

And if that still doesn't do the trick -- any help to investigate this and track it down is appreciated 😅

michalczaplinski commented on PR #3319:


20 months ago
#20

Investigating this now 👀

michalczaplinski commented on PR #3319:


20 months ago
#21

I've tested locally by reproducing the testing instructions from above and I can still confirm the same results.

The result is the same with and without the patch.

oandregal commented on PR #3319:


20 months ago
#22

I've rebased and tested this and it works as expected. Coordinating with Ben to see who can rebase it.

audrasjb commented on PR #3319:


20 months ago
#23

As asked by @oandregal, this was committed in https://core.trac.wordpress.org/changeset/54408

@oandregal commented on PR #3319:


19 months ago
#24

For future reference, this PR bundles two different things that are unrelated to each other. The SVN commit and issue description does not reflect that well.

Note: See TracTickets for help on using tickets.