Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 5 weeks ago

#50620 closed defect (bug) (fixed)

REST API: regression after introducing changes how block renderer endpoint is now defined

Reported by: manooweb's profile manooweb Owned by: timothyblynjacobs's profile TimothyBlynJacobs
Milestone: 5.5 Priority: normal
Severity: normal Version: 5.5
Component: REST API Keywords: good-first-bug has-patch has-unit-tests
Focuses: Cc:

Description (last modified by SergeyBiryukov)


We developed a dynamic block with only boolean block attributes.
For this development we used WordPress 5.4.2 and everything works fine.

We are testing with the future version of WordPress 5.5 ( WordPress 5.5-beta1-48410 ) and we noticed a regression because sanitization of block attributes doesn't work anymore espacially for boolean attributes which is important to get the right value in PHP (true or false) and not a string.

Indeed there has been changes in the REST API block renderer endpoint definition introduced by this changeset [48069] to fix this ticket #48079.

As we can see in the endpoint schema we lost the 'properties' attributes which is used before to set the block attributes.

Everything is ok during the attributes validation which passes through the 'validate_callback' defined in the endpoint.

However during the REST API process we also pass through a sanitization step here

and then here for block attributes

We never pass through the condition because the 'properties' array is never defined at this point as it should be.

As every dynamic blocks need their own attributes, I don't understand how to set correctly this 'properties' array to get the values correctly sanitized.

Of course we found a workaround on our side directly in our 'render_callback' of our dynamic block function but I think it is not the best and right solution.

If you need further information, let me know.


Attachments (2)

50620.diff (1.3 KB) - added by manooweb 4 years ago.
Add sanitize_callback in block renderer endpoint
50620.1.diff (4.9 KB) - added by manooweb 4 years ago.
Add unit test

Download all attachments as: .zip

Change History (13)

#1 @manooweb
4 years ago

  • Summary changed from REST API: regression after introducing changes how block renderer endpoint is non defined to REST API: regression after introducing changes how block renderer endpoint is now defined

#2 @TimothyBlynJacobs
4 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added
  • Milestone changed from Awaiting Review to 5.5

Hi @manooweb,

Thanks for the ticket! That's a great catch. Do you want to work on a patch for it? We'd essentially want to duplicate our validate_callback that is set, but at the end call rest_sanitize_value_from_schema instead of the validate function.

#3 @manooweb
4 years ago

Hi Timothy,

You're welcome

Of course, I can try.
What do you mean excatly by duplicate

Create a sanitize_callback entry which duplicates the code of validate_callback except the call to rest_validate_value_from_schema replaced by a call to rest_sanitize_value_from_schema ?

#4 @manooweb
4 years ago

Timothy, I attached the patch. I tested manually on my WordPress environment and solve the issue on our dynamic block.

Do I also have to work on unit tests ?

4 years ago

Add sanitize_callback in block renderer endpoint

#5 @TimothyBlynJacobs
4 years ago

  • Keywords has-patch added; needs-patch needs-unit-tests removed

That looks great, thanks!

Do I also have to work on unit tests ?

You can work on the tests if you want to, but I can add them if you don't. If you are interested, this might be a helpful guide:

#6 @SergeyBiryukov
4 years ago

  • Description modified (diff)

4 years ago

Add unit test

#7 @manooweb
4 years ago

Hi Timothy,
I've just added a new patch containing the fix and thus a unit test.

Have a nice week-end 😉

This ticket was mentioned in PR #400 on WordPress/wordpress-develop by TimothyBJacobs.

4 years ago

  • Keywords has-unit-tests added

#9 @TimothyBlynJacobs
4 years ago

Thanks for the tests @manooweb! Committing with some minor changes to the tests to make them more explicit.

#10 @TimothyBlynJacobs
4 years ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 48437:

REST API: Sanitize block renderer attributes.

In [48069] the Block Renderer was changed to register a single route for all dynamic blocks. Validation was dynamically applied based on the requested block, but sanitization was not. This commit adds the same sanitization back to the block attributes.

Props manooweb.
Fixes #50620. See #48079.

khansahan9864 commented on PR #400:

5 weeks ago

so what about changes how to do that

Note: See TracTickets for help on using tickets.