Enhanced twg settings validation #5573
Conversation
|
Size Change: +881 kB (+65%) 🆘 Total Size: 2.25 MB
ℹ️ View Unchanged
|
| if ( ! is_array( $button_post_types ) ) { | ||
| return array( 'post' ); | ||
| } |
There was a problem hiding this comment.
Why is this here? I guess it's for the default value/settings? I'd think a safer default is to show this on no post types by default 🤔
| if ( ! is_array( $button_post_types ) ) { | |
| return array( 'post' ); | |
| } | |
| if ( ! is_array( $button_post_types ) ) { | |
| return array(); | |
| } |
| if ( ! is_array( $button_post_types ) ) { | ||
| return array( 'post' ); | ||
| } | ||
| return array_intersect( $button_post_types, get_post_types( array( 'public' => true ) ) ); |
There was a problem hiding this comment.
Additionally, I wonder if this matches the UI we show… in the UI we only show posts that support viewable (see: #5456). I wonder if this would allow a user to submit a post type successfully that wouldn't appear in the UI.
Not a big deal as they'd need to intentionally do so, but it would be nice if the only posts we allowed to save would be ones that would appear with the 'viewable' value in supports. 🤔
I know this is what was in the ACs/IB, but I think it's maybe a bit inaccurate? Would there be an easy way to get all post types that match what wp.data.select('core').getPostTypes().filter((postType) => postType.viewable) returns? If so let's do that 😄
There was a problem hiding this comment.
Hi, the API for getPostTypes() uses is_post_type_viewable for each post to determine the viewable status. For a post to be viewable:
- if it's a builtin post type, the
publicmust betrue. - If it's not a builtin post type, the
publicly_queryablemust betrue.
Which definitely makes your point valid. I'll see what's the best way to achieve that here.
Off Topic: speaking of getPostTypes be aware that it only returns 10 post types if the site has more. See here. The workaround is to supply { per_page: -1 } to the selector which will return 100. If there's 101 post type this workaround will not work 😁
There was a problem hiding this comment.
Yeah, sadly I think that's our best option for now until someone runs into an issue 😅
We could add pagination if needed, but for now 101 is probably enough.
| @@ -0,0 +1,114 @@ | |||
| <?php | |||
| /** | |||
| * Class Google\Site_Kit\Tests\Modules\Thank_With_Google\Settings | |||
There was a problem hiding this comment.
| * Class Google\Site_Kit\Tests\Modules\Thank_With_Google\Settings | |
| * Class Google\Site_Kit\Tests\Modules\Thank_With_Google\SettingsTest |
| /** | ||
| * Gets the setting value. | ||
| * | ||
| * @since m.e.x.t |
There was a problem hiding this comment.
Please be sure to use the correct placeholder as search/replace will miss it otherwise.
| * @since m.e.x.t | |
| * @since n.e.x.t |
There was a problem hiding this comment.
Oh my goodness, they looks same to me on monospaced font. Sorry!
There was a problem hiding this comment.
I missed it too, waiting for someone to make a m.e.x.t release in ZenHub just to really mess with us 😉
|
Update: looks like a bug, because I ran it locally and got this:
|
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist