-
Notifications
You must be signed in to change notification settings - Fork 64
Prevent an error for a block with a large attributes object #524
Conversation
This should at least temporarily fix two recurring issues in GitHub issues and wp.org support topics.
This should be more reliable, in case the strings change.
Before, this did not allow POST requests, so add this.
Test that the filter of the endpoints works, and that the edited get_items() function works as expected.
The Rest class uses classes from Core, and has an important filter of 'rest_endpoints' that needs to work with all supported versions of Core.
- php: 5.6 | ||
env: WP_VERSION=trunk | ||
- php: 5.6 | ||
env: WP_VERSION=5.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new class Rest
uses some classes from Core, and has an important filter of 'rest_endpoints'
that needs to work with WP 5.0+
So I added more WP versions to the matrix. If this causes builds to take a lot longer, we could revert this change to .travis.yml
.
Here's a build of this branch: |
* Forked from Gutenberg, with a minor edit to allow using POST requests instead of GET requests. | ||
* Todo: delete if this is merged: https://github.com/WordPress/gutenberg/pull/21068/ | ||
* | ||
* @see https://github.com/WordPress/gutenberg/blob/c72030189017c8aac44453c1386f4251e45e80df/packages/server-side-render/src/index.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copied almost verbatim from Gutenberg.
index.js and server-side-render.js are copied into this file.
// If requestBody, make a POST request, with the attributes in the request body instead of the URL. | ||
// This allows sending a larger attributes object than in a GET request, where the attributes are in the URL. | ||
const urlAttributes = requestBody ? null : attributes; | ||
const path = rendererPath( block, urlAttributes, urlQueryArgs ); | ||
const method = requestBody ? 'POST' : 'GET'; | ||
const data = requestBody ? attributes : null; | ||
|
||
// Store the latest fetch request so that when we process it, we can | ||
// check if it is the current request, to avoid race conditions on slow networks. | ||
const fetchRequest = ( this.currentFetchRequest = apiFetch( { | ||
path, | ||
method, | ||
data, | ||
} ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change from Gutenberg. It's what allows sending a POST request.
Also, make minor edits to PHPDoc summaries.
Hi @lukecarbis, Could you please review this when you have a chance? This is taking on some tech debt, forking the But I think it's worth it to have the Repeater and large blocks reliably work. Also, I think it'll fix an issue that's been raised a few times, but I couldn't reproduce: Thanks, Luke! |
This branch fixes the "Not a Valid JSON Response" error for me, which is fantastic! Well done, and thank you! I'm still iffy about this approach technically, though. I'd much rather see the change implemented in Gutenberg / Core. Would like to discuss this more with you before merging, but aside from that, I'm happy with the PR as it is. |
Sounds good, we can talk about this whenever you'd like. I agree, fixing this in Gutenberg and Core would be better. The PHP filter in this PR would probably still be needed, even if the Core patch was merged. In case users aren't running the latest version of WordPress, the There's been no action on the Core ticket/PR or the Gutenberg PR since the day after I opened them. |
* @param WP_REST_Request $request Full details about the request. | ||
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure. | ||
*/ | ||
public function get_item( $request ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method shouldn't be needed, per the feedback in the Core patch: https://core.trac.wordpress.org/ticket/49680#comment:11
filter_block_endpoints()
will probably still be needed for years, though.
foreach ( $endpoints as $route => $handler ) { | ||
if ( 0 === strpos( $route, '/wp/v2/block-renderer/(?P<name>block-lab/' ) && isset( $endpoints[ $route ][0] ) ) { | ||
$endpoints[ $route ][0]['methods'] = [ \WP_REST_Server::READABLE, \WP_REST_Server::CREATABLE ]; | ||
$endpoints[ $route ][0]['callback'] = [ $this, 'get_item' ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be removed.
We should finally be able to fix this, with the the new server-side-render released. |
Closing in favor of GCB PR studiopress/genesis-custom-blocks#39 |
Changes
ServerSideRender
Fixes #477, #436, #511
Testing instructions
<ServerSideRender>
displays