Permit unbounded per_page=-1 requests for Pages and Shared Blocks #6657

danielbachhuber · 2018-05-09T12:32:59Z

Description

Permits requests of per_page=-1 to /wp/v2/pages and /wp/v2/blocks in order to fetch all available items; updates corresponding Gutenberg components to fetch all items.

Fixes #6487
Fixes #4632
Related #6627
See #6180 (comment)

How has this been tested?

I generated 200 pages with:

wp post generate --post_type=page --post_status=publish --count=200

When I access the Page editor, I can see all items in the dropdown:

Similarly, I created more than 10 shared blocks and can see all of them in the inserter UI:

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

danielbachhuber · 2018-05-09T13:43:16Z

Test failure was unrelated; I cleared cache and restarted.

rianrietveld

@danielbachhuber I don't think I'm the best person to review this PR.

tofumatt

This doesn't seem to answer UX questions around large lists or how to approach this, and returning an unbounded list of authors/block types (or really, anything) is dangerous.

The trac issue linked to in this code also mentions that unbounded requests can cause performance issues. I don't see how the proposed patch in Core (https://core.trac.wordpress.org/ticket/43998) or this patch prevent DOS issues from unbounded queries. Aside from straight-up DOS (exhausting the DB, whatever else), this could result in slow load times which would negatively impact Gutenberg UX.

This solution seems to hope that:

100000 is enough for $prepared_args['posts_per_page']–https://github.com/WordPress/gutenberg/pull/6657/files#diff-d0c65e7ac505d44d8571ddaffc510d98R494
We don't need to worry about the performance impact this will have

I understand the existing discussion around this issue has been ongoing for awhile and it's quite a frustrating bug, but I'm not sure this addresses many of the issues raised.

tofumatt · 2018-05-09T15:50:17Z

lib/rest-api.php

+	$post_types = array( 'page', 'wp_block' );
+	if ( in_array( $post_type->name, $post_types, true )
+		&& isset( $query_params['per_page'] ) ) {
+		// Change from '1' to '-1', which means unlimited.


I don't quite understand this comment; where is '1' being checked for?

where is '1' being checked for?

It doesn't need to be checked for. '1' is the default value: https://github.com/WordPress/WordPress/blob/2f792d442bf771a3aade170cc9cae459f024c57b/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L321

I don't see any checks at all for minimum though.

Maybe I just don't get the query params variable here but it looks like it's a dictionary and here you're just checking for $query_params['per_page'], not $query_params['per_page']['minimum'].

It would be at least handy to say // Change from default value '1' to '-1', which means unlimited.

That said this code strikes me as assuming minimum is set to the default if set at all which I can't tell from here.

I'm not sure I follow. Can you create a PR against this PR with your suggested changes?

Oh wait, I understand the code now but it was not obvious at first. I'd suggest changing this function to add a few more comments for context:

function gutenberg_filter_post_collection_parameters( $query_params, $post_type ) { $post_types = array( 'page', 'wp_block' ); // HACK: If 'per_page' is set, ignore it and change the items returned // per-page to be '-1'. // See: https://github.com/WordPress/gutenberg/issues/6180#issuecomment-384511059 if ( in_array( $post_type->name, $post_types, true ) && isset( $query_params['per_page'] ) ) { // Change from '1' (default) to '-1', which means unlimited. $query_params['per_page']['minimum'] = -1; // Default sanitize callback is 'absint', which won't work in our case. $query_params['per_page']['sanitize_callback'] = 'rest_sanitize_request_arg'; } return $query_params; }

That seems to be the intent of the code, right?

I don't think that comment is right. This is adjusting the collection params schema, not the actual request arguments. The schema ( per_page) is validated by WordPress itself in WP_REST_Request using rest_validate_value_from_schema.

Ah, gotcha; the structure of these filters is unclear to me as I'm new to the project. Thanks for the explanation 👍

tofumatt · 2018-05-09T15:53:16Z

lib/rest-api.php

+		// Avoid triggering 'rest_post_invalid_page_number' error
+		// which will need to be addressed in https://core.trac.wordpress.org/ticket/43998.
+		if ( -1 === $prepared_args['posts_per_page'] ) {
+			$prepared_args['posts_per_page'] = 100000;


This looks like a hack that will be hard to debug as there isn't any logging around it.

Also: it seems unbounded requests are only allowed for authorised users... seems like they need edit_posts to be able to use this feature but just wanted to make sure that's also a requirement to use Gutenberg (sorry, I'm new here!) or this will have unpredictable results.

This looks like a hack that will be hard to debug as there isn't any logging around it.

Yes. It's a hack that will only exist until the patch I've yet to prepare for https://core.trac.wordpress.org/ticket/43998 lands in WP 4.9.7

Also: it seems unbounded requests are only allowed for authorised users... seems like they need edit_posts to be able to use this feature but just wanted to make sure that's also a requirement to use Gutenberg (sorry, I'm new here!) or this will have unpredictable results.

Correct. edit_posts is a reasonable minimum capability assumption for Gutenberg.

danielbachhuber · 2018-05-09T17:11:29Z

This doesn't seem to answer UX questions around large lists or how to approach this, and returning an unbounded list of authors/block types (or really, anything) is dangerous.

Correct, this is intentional. See conversation in #6180 (comment)

tofumatt · 2018-05-09T17:24:29Z

I read through the issues linked and I think I accidentally skimmed that comment, my bad.

I guess right now this is broken for anyone with lists over 100, whereas the new version will only break for those with lists over $performance-breaking-number 😆 👍

So it is a definite improvement, good point. There are a few code things I don't quite grok that I think could at least use some comments but I'll withdraw my "no unbounded results" request 😄

adamsilverstein · 2018-05-09T19:01:58Z

@danielbachhuber I'm still concerned here around scalability. Does this still work if the site has 2,000 pages? what is memory footprint client/server wise to fulfill these large request?

danielbachhuber · 2018-05-09T20:38:56Z

I'm still concerned here around scalability.

Do your scalability concerns with this pull request differ from those with the wp_dropdown_pages() implementation in the classic editor?

Does this still work if the site has 2,000 pages?

Yes, it works in my testing. The REST API request takes ~7s to fulfill.

what is memory footprint client/server wise to fulfill these large request?

Is memory utilization a limiting factor here?

adamsilverstein · 2018-05-09T21:00:28Z

Do your scalability concerns with this pull request differ from those with the wp_dropdown_pages() implementation in the classic editor?

yes, I'm concerned about the additional client side memory requirements added by keeping all of the records in the app memory/state.

Yes, it works in my testing. The REST API request takes ~7s to fulfill.

good to hear. i hesitate to ask about 20,000 or 200,000.

danielbachhuber · 2018-05-09T21:05:31Z

yes, I'm concerned about the additional client side memory requirements added by keeping all of the records in the app memory/state.

How much additional memory is this, over the data that's already present in <select>? What percentage of WordPress sites are impacted by this concern? And, given the API request isn't even made until the UI is opened, is this a concern we need to optimize for?

adamsilverstein · 2018-05-10T15:20:52Z

> How much additional memory is this, over the data that's already present in <select>?

twice as much vs a select? i'm comparing this to a more search/request based solution that never loads the full set

What percentage of WordPress sites are impacted by this concern?

no idea what the percentage is, certainly the total number is not insignificant. For these users, a dropdown may be unusable.

And, given the API request isn't even made until the UI is opened, is this a concern we need to optimize for?

I'm not sure if its a concern we should ignore.

If we do take this approach, I'd still love to work on a more scalable search based selector. Since this pattern of selecting from a (potentially very large) list is something we are likely to repeat in different contexts across Gutenberg, I'd love to see it solved in a way that worked very well for all cases. My hope is that something similar to the searchable select i suggested in #5921 does.

danielbachhuber · 2018-05-10T16:11:48Z

twice as much vs a select? i'm comparing this to a more search/request based solution that never loads the full set

Just to clarify, is this a hypothetical concern, or something you've tested and have data / methodology for?

I'm not sure if its a concern we should ignore.

This is an incorrect characterization of my perspective. Rather, I'm suggesting that:

It's May 10th, 2018.
We have at least two critical bugs to resolve (Page select UI is limited to 101 select items #6487 and Saved blocks list doesn't show all saved blocks (only a maximum of 10) #4632).
In the interest of time, we go with the solution we have.

If we do take this approach, I'd still love to work on a more scalable search based selector. Since this pattern of selecting from a (potentially very large) list is something we are likely to repeat in different contexts across Gutenberg, I'd love to see it solved in a way that worked very well for all cases.

None of this precludes producing a "better" solution in the future, particularly if new information presents itself. As I mentioned in #5921, changing the UI/UX of these components will need design first, and then be validated for accessibility. This work should be done before development.

felixarntz

Just a minor strict code issue. I think the approach is fine. For core, this should obviously become part of the respective REST API controllers.

felixarntz · 2018-05-10T17:10:17Z

lib/rest-api.php

+		'/wp/v2/pages',
+		'/wp/v2/users',
+	);
+	if ( in_array( $request->get_route(), $routes ) ) {


This should perform a strict check.

Feedback was addressed

danielbachhuber · 2018-05-10T19:38:42Z

Going ahead and landing this. We can iterate on the UX as solutions become available.

nylen · 2018-05-10T19:57:58Z

@danielbachhuber rather than this "unlimited but not really unlimited" approach that uses -1 as a special value, I think it would be better to set a higher limit for users who have edit_posts. This should lead to much simpler code, and setting the limit to a value that is tested and known to work should also prevent bad behavior with extremely large numbers of objects.

tofumatt · 2018-05-10T20:39:18Z

I meant to update this PR to say I approve of the changes after seeing my comments addressed, so retroactive 👍 from me, sorry about that! 😄

danielbachhuber · 2018-05-10T21:36:02Z

rather than this "unlimited but not really unlimited" approach that uses -1 as a special value, I think it would be better to set a higher limit for users who have edit_posts. This should lead to much simpler code, and setting the limit to a value that is tested and known to work should also prevent bad behavior with extremely large numbers of objects.

@nylen Can you open new issue with the specifics of your proposal?

Permit unbounded requests for Pages and Shared Blocks

6dd0d83

danielbachhuber added this to the 2.9 milestone May 9, 2018

danielbachhuber requested a review from a team May 9, 2018 12:32

Fix PHPCS issue

0205923

danielbachhuber requested a review from a team May 9, 2018 13:13

rianrietveld reviewed May 9, 2018

View reviewed changes

tofumatt previously requested changes May 9, 2018

View reviewed changes

felixarntz previously requested changes May 10, 2018

View reviewed changes

Use a strict check

3cf6f5e

danielbachhuber merged commit fb804e2 into master May 10, 2018

danielbachhuber deleted the 6487-unbounded-post-requests branch May 10, 2018 19:38

nylen mentioned this pull request May 11, 2018

REST API: Progressively load data into components through pagination #6694

Closed

Lewiscowles1986 mentioned this pull request May 11, 2018

Page select UI is limited to 101 select items #6487

Closed

3 tasks

This was referenced May 11, 2018

Permit unbounded per_page=-1 requests for Categories and Tags #6722

Merged

Support for automatically iterating paginated resources in core-data #6180

Closed

jorgefilipecosta mentioned this pull request Jul 20, 2018

Page Attributes: Fix: Parent-Dropdown missing for custom post-type. #8087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permit unbounded per_page=-1 requests for Pages and Shared Blocks #6657

Permit unbounded per_page=-1 requests for Pages and Shared Blocks #6657

danielbachhuber commented May 9, 2018 •

edited

Loading

danielbachhuber commented May 9, 2018

rianrietveld left a comment

tofumatt left a comment

tofumatt May 9, 2018

danielbachhuber May 9, 2018

tofumatt May 9, 2018

danielbachhuber May 9, 2018

tofumatt May 9, 2018 •

edited

Loading

TimothyBJacobs May 9, 2018

tofumatt May 10, 2018

tofumatt May 9, 2018

danielbachhuber May 9, 2018

danielbachhuber commented May 9, 2018

tofumatt commented May 9, 2018

adamsilverstein commented May 9, 2018

danielbachhuber commented May 9, 2018

adamsilverstein commented May 9, 2018

danielbachhuber commented May 9, 2018 •

edited

Loading

adamsilverstein commented May 10, 2018

danielbachhuber commented May 10, 2018

felixarntz left a comment

felixarntz May 10, 2018

danielbachhuber May 10, 2018

danielbachhuber commented May 10, 2018

nylen commented May 10, 2018

tofumatt commented May 10, 2018

danielbachhuber commented May 10, 2018

Permit unbounded per_page=-1 requests for Pages and Shared Blocks #6657

Permit unbounded per_page=-1 requests for Pages and Shared Blocks #6657

Conversation

danielbachhuber commented May 9, 2018 • edited Loading

Description

How has this been tested?

Checklist:

danielbachhuber commented May 9, 2018

rianrietveld left a comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented May 9, 2018

tofumatt commented May 9, 2018

adamsilverstein commented May 9, 2018

danielbachhuber commented May 9, 2018

adamsilverstein commented May 9, 2018

danielbachhuber commented May 9, 2018 • edited Loading

adamsilverstein commented May 10, 2018

danielbachhuber commented May 10, 2018

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented May 10, 2018

nylen commented May 10, 2018

tofumatt commented May 10, 2018

danielbachhuber commented May 10, 2018

danielbachhuber commented May 9, 2018 •

edited

Loading

tofumatt May 9, 2018 •

edited

Loading

danielbachhuber commented May 9, 2018 •

edited

Loading