Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can't save changes when having more than 25 blocks in a Widgets area #33579

Closed
htmgarcia opened this issue Jul 20, 2021 · 7 comments · Fixed by #34280
Closed

Can't save changes when having more than 25 blocks in a Widgets area #33579

htmgarcia opened this issue Jul 20, 2021 · 7 comments · Fixed by #34280
Assignees
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. REST API Interaction Related to REST API [Status] In Progress Tracking issues with work in progress

Comments

@htmgarcia
Copy link

Description

WordPress 5.8. When having more than 25 blocks in a widgets area, including "Inactive widgets", can't save changes. When updating got the error: There was an error. Invalid parameter(s): requests.
In console:
Screen Shot 2021-07-20 at 14 22 24

Step-by-step reproduction instructions

  1. Go to Appearance > Widgets
  2. Add Blocks to a widgets area (e.g. "Footer"), as example 5 blocks. Be sure the number of blocks must be 25 or lower.
  3. Click "Update" and changes are correctly saved.
  4. Repeat step 2 but be sure you have more than 25 blocks in a single widgets area.
  5. Click "Update"

Expected behaviour

The changes must be saved

Actual behaviour

Changes can't be saved

WordPress information

  • WordPress version: 5.8
  • Gutenberg version: 11.0.0
  • Are all plugins except Gutenberg deactivated? Yes
  • Are you using a default theme (e.g. Twenty Twenty-One)? Yes, Twenty Twenty-One

Device information

  • Device: Desktop
  • Operating system: MacOS Big Sur 11.4
  • Browser: Firefox 89.0.2
@talldan talldan added [Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. REST API Interaction Related to REST API labels Jul 21, 2021
@noisysocks noisysocks added this to the WordPress 5.8.1 milestone Jul 21, 2021
@Mamaduka
Copy link
Member

By default, the REST API accepts up to 25 requests in a single batch. However, this value is filterable so it can be scaled up or down based on server resources.

Source - https://make.wordpress.org/core/2020/11/20/rest-api-batch-framework-in-wordpress-5-6/

@adamziel
Copy link
Contributor

adamziel commented Jul 23, 2021

We have two ways of proceeding here:

  1. Increase the batch size. The risk is that some slower hosts won't be able to process e.g. 150 batched requests and time out or run out memory in the middle.
  2. Split e.g. 150 updates into 6 batches and send them to the server one after another. The risks: interrupted internet connection, validation error in batch 4.

In both cases there's a risk of data corruption if the processing stops in the middle.

That being said, scaling the batch size up can only take us so far. Even if we set it to 1000, it is possible to have 1001 requests to process. We still need a reusable way of handling the overflowing requests. So essentially we need to implement point 2 above. Technically, we would send ceil( NB_REQUESTS / MAX_BATCH_SIZE ) batch requests one after another and hope for the best.

On the scale of WordPress I am sure that data corruption will still happen in at least some cases, so we also need a way of handling that. This could be as simple as communicating what happened and listing steps to fix the problem, but it would mean that some sites are in a broken state at least some of the time. To avoid that, we'd need to update widgets using something like customizer changesets – nothing is published until all the changes reach the server with a valid 200-ish response. I much prefer that latter scenario as it wouldn't leave any site in a broken state.

cc @TimothyBJacobs

@TimothyBJacobs
Copy link
Member

Didn't we have sub batching implemented at some point? It seems like it might've been lost in one of the refactors.

I'm fine with increasing the batch size, it was a fairly arbitrary number, but we have to keep in mind that the batch limit isn't currently per endpoint type, it's global. So I'd prefer to be conservative.

@adamziel
Copy link
Contributor

We still need a general solution, but we can improve the situation by only batching requests for the widgets that were updated. Currently we always sent n requests even if only 1 out of n widgets was updated. Not having a /reorder endpoint makes it a bit more complex as moving the last widget to the beginning of the list affects all n widgets.

@TimothyBJacobs
Copy link
Member

Not having a /reorder endpoint makes it a bit more complex as moving the last widget to the beginning of the list affects all n widgets.

In what way? The sidebars endpoint contains a widgets property which is an ordered list of widget ids. The widgets shouldn't need to be individually updated when their position changes.

@noisysocks
Copy link
Member

Didn't we have sub batching implemented at some point? It seems like it might've been lost in one of the refactors.

I'll look into this. I might have accidentally blown it away in #28210.

@noisysocks noisysocks self-assigned this Aug 25, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Aug 25, 2021
@adamziel
Copy link
Contributor

adamziel commented Aug 25, 2021

In what way? The sidebars endpoint contains a widgets property which is an ordered list of widget ids. The widgets shouldn't need to be individually updated when their position changes.

I got confused there – thank you for clarifying @TimothyBJacobs! Sounds like we're good to only POST about the updated widget instances then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Widgets Screen The block-based screen that replaced widgets.php. Needs Testing Needs further testing to be confirmed. REST API Interaction Related to REST API [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants