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

Block Patterns: Load basic core patterns from Pattern Directory #1298

Closed
wants to merge 3 commits into from

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented May 25, 2021

This ports over the pattern-directory API endpoint from Gutenberg trunk. This was added in WordPress/gutenberg#28800 & WordPress/gutenberg#26578.

I've removed most of the default patterns, and hooked up the Pattern Directory API to return the patterns marked "Core" from https://wordpress.org/patterns/.

This remote loading checks for core-block-patterns theme support and adds a new filter, should_load_remote_block_patterns. If core-block-patterns theme support is removed, it will disable all core patterns including remote patterns. If should_load_remote_block_patterns is filtered to return false, the core query blocks will be registered, but the remote request won't happen.

This is done without the blockTypes support, since WordPress/gutenberg#32113 is still in progress. If I understand the process correctly, that can be done during feature-freeze?

Trac ticket: https://core.trac.wordpress.org/ticket/53246


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@ryelle ryelle self-assigned this May 25, 2021
@desrosj
Copy link
Contributor

desrosj commented May 25, 2021

I started looking into the failing test here. It looks like if I comment out the added current_screen filter, the tests pass as expected. This is strange because REST requests are not supposed to be in the admin context. Also worth noting that the test passes when running only one specific group.

I have to step away for a bit, but I'll circle back when I return to continue looking into the issue.

@TimothyBJacobs
Copy link
Member

These errors are occurring in rest_preload_api_request which is what is causing the preload data to be empty, and causing the test to fail.

WP_REST_Response Object
(
    [links:protected] => Array
        (
        )

    [matched_route:protected] => 
    [matched_handler:protected] => 
    [data] => Array
        (
            [code] => rest_no_route
            [message] => No route was found matching the URL and request method.
            [data] => Array
                (
                    [status] => 404
                )

        )

    [headers] => Array
        (
        )

    [status] => 404
)
WP_REST_Response Object
(
    [links:protected] => Array
        (
        )

    [matched_route:protected] => 
    [matched_handler:protected] => 
    [data] => Array
        (
            [code] => rest_no_route
            [message] => No route was found matching the URL and request method.
            [data] => Array
                (
                    [status] => 404
                )

        )

    [headers] => Array
        (
        )

    [status] => 404
)

Not sure why those routes don't appear to be registered yet. It may be that the REST Server initialization caused by the _load_remote_block_patterns is too early somehow?

@TimothyBJacobs
Copy link
Member

I pushed a fix. Ideally we'd have some way of detecting when the REST API server needs to be registered built in to the base test class, but alas.

@ryelle
Copy link
Contributor Author

ryelle commented May 25, 2021

Thanks for tracking that down, @TimothyBJacobs!

ryelle and others added 3 commits May 25, 2021 20:51
These tests are in fact initializing and using the REST API, so they
need to be resetting the server instance like the rest of the REST API
tests do.
@ryelle
Copy link
Contributor Author

ryelle commented May 26, 2021

Committed this first pass in r51021.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants