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

Navigation: Load the raw property on the navigation fallback #52758

Merged
merged 16 commits into from
Jul 21, 2023
Merged

Conversation

scruffian
Copy link
Contributor

What?

When loading the navigation fallback with an embed context we need to add the raw property of the title so that we have the response in the format expected by the frontend.

Why?

Without the raw property, the data resolver treats the data differently which causes errors like #52680 and #52691

How?

Updates the schema to include the raw property.

Testing Instructions

  1. Open the site editor at the root level
  2. Open the patterns section
  3. Open the headers section
  4. Click on a header
  5. Add a navigation block to the header, making sure you have two navigation blocks in the template
  6. Make sure that the two navigation blocks use different wp_navigation CPTs
  7. In trunk you should see an error, on this branch you should not.

Note

I tried adding an end to end test for this change, but it's not possible to navigate directly to the root of the site editor in the test environment - not sure why.

@scruffian scruffian added [Type] Bug An existing feature does not function as intended Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jul 19, 2023
@scruffian scruffian requested review from getdave and jeryj July 19, 2023 11:45
@scruffian scruffian self-assigned this Jul 19, 2023
@github-actions
Copy link

github-actions bot commented Jul 19, 2023

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/compat/wordpress-6.3/navigation-fallback.php
❔ phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change itself is solid. The precedent is set by the same changes to the content field which already got approval from REST API maintainers in WP Core.

I would add a PHPUnit test that asserts that making a call to the endpoint returns both rendered and raw fields.

We already have the tests so it's just a case of adding one more. These should assert on the presence of fields in the response that aren't typically included in _embed responses:

  • content
  • title.raw

We could even use the test_context_param test which is currently disabled.

Once done let's spin up that Core patch asap.

@github-actions
Copy link

github-actions bot commented Jul 19, 2023

Flaky tests detected in 2e561dc.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5616469576
📝 Reported issues:

@t-hamano
Copy link
Contributor

I have tested this PR and can confirm that the issue reported in #52691 has been resolved.

@getdave
Copy link
Contributor

getdave commented Jul 20, 2023

For the test I suggest that because we can't work with embedded results in Test setup, we instead just call the linked endpoint directly in the test with context set to embed to simulate the exact same thing.

Something like this:

public function test_adds_correct_fields() {
	$request  = new WP_REST_Request( 'GET', '/wp-block-editor/v1/navigation-fallback' );
	$response = rest_get_server()->dispatch( $request );
	$data     = $response->get_data();

	$navigation_post_id = $data['id'];

	$links = $response->get_links();
        
    // Manually request embedded data due to this issue - https://core.trac.wordpress.org/ticket/55961.
	$embeddable_post_rest_url = str_replace( 'http://localhost:8889/index.php?rest_route=', '', $links['self'][0]['href'] );

	$request  = new WP_REST_Request( 'GET', $embeddable_post_rest_url );
	$request->set_param( 'context', 'embed' );
	$response = rest_get_server()->dispatch( $request );
	$data     = $response->get_data();

    // Assertions here to test for correct fields being included.
	echo '<pre>';
	var_dump( $data );
	echo '</pre>';
}

Ideally we’d not hard code this part http://localhost:8889/index.php?rest_route=. Ideally that would be dynamically created.

$response = rest_get_server()->dispatch( $request );
$data = $response->get_data();

// Verify that the additional fields are present.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we collocate the subfield checks for fields of the same type (e.g. content, title...etc)?

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good on the test. A few tweaks required 🙏

@ramonjd ramonjd added the Needs PHP backport Needs PHP backport to Core label Jul 21, 2023
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I don't think test failures are relevant.

@getdave getdave enabled auto-merge (squash) July 21, 2023 08:57
@getdave getdave disabled auto-merge July 21, 2023 09:57
@getdave getdave merged commit 87ef1df into trunk Jul 21, 2023
48 of 49 checks passed
@getdave getdave deleted the fix/52680 branch July 21, 2023 09:57
@github-actions github-actions bot added this to the Gutenberg 16.4 milestone Jul 21, 2023
@getdave
Copy link
Contributor

getdave commented Jul 21, 2023

I've forced merged this after consulting other contributors and confirming these tests are failing for all PRs. They are unrelated to this PR and holding up an important bug fix that can crash the editor.

@ramonjd
Copy link
Member

ramonjd commented Jul 24, 2023

This PR needs a backport. Added label

@ramonjd
Copy link
Member

ramonjd commented Jul 24, 2023

RC2 for WordPress 6.3 will be cut in the next few hours. If folks don't have time, I can get one ready

tellthemachines pushed a commit that referenced this pull request Jul 24, 2023
* Navigation: Load the raw property on the navigation fallback

* Update lib/compat/wordpress-6.3/navigation-fallback.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update lib/compat/wordpress-6.3/navigation-fallback.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Add a test for these properties

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* add more comments

* add necessary method

* Fix php coding standards error

---------

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
@tellthemachines
Copy link
Contributor

I just cherry-picked this PR to the update/packages-RC2 branch to get it included in the next release: c98607b

@tellthemachines tellthemachines removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jul 24, 2023
ramonjd added a commit to ramonjd/wordpress-develop that referenced this pull request Jul 24, 2023
Updated typo in wp_add_fields_to_navigation_fallback_embeded_links
Added tests (failing)
tellthemachines added a commit that referenced this pull request Jul 24, 2023
* Filter out patterns that are not allowed in the inserter (#52675)

* Remove autofocus and improve placeholder text consistency. (#52634)

* Do not navigate to the styles pages unless you're in a random listing page (#52728)

* Patterns: Don't override the rootClientID  in create menu - only set if undefined (#52713)

* Footnotes: store in revisions (#52686)

* Fix: Block toolbar obscuring document tools when Top Toolbar is enabled (#52722)

* Update toolbar width

* Site editor needs specific width

* fixes top toolbar width for post editor when not in fullscreen

* remove the body rule

---------

Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>

* Site Editor: Fix site link accessibility issues (#52744)

* Add id to pattern inserted notice to stop multiple notices stacking (#52746)

* Global Styles: Don't use named arguments for 'sprintf' (#52782)

* Footnotes: Use static closures when not using '' (#52781)

* removes check for active preview device type to enable the fixed toolbar preference (#52770)

* Parser / Site Editor: Ensure autop is not run when freeform block is set to core/html (#52716)

* Parse / Site Editor: Ensure autop is not run when freeform block is set to core/html

* Switch to equals freeform instead of not equals core/html

* Rename core/test-freeform to core/freeform in tests

* Adding @SInCE annotation for relevant 6.3 changes. (#52820)

* Navigation: Load the raw property on the navigation fallback (#52758)

* Navigation: Load the raw property on the navigation fallback

* Update lib/compat/wordpress-6.3/navigation-fallback.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update lib/compat/wordpress-6.3/navigation-fallback.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Add a test for these properties

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* Update phpunit/class-gutenberg-rest-navigation-fallback-controller-test.php

Co-authored-by: Dave Smith <getdavemail@gmail.com>

* add more comments

* add necessary method

* Fix php coding standards error

---------

Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>

* Allow styles to be changed dynamically through editor settings (#52767)

* ResizableFrame: Fix styling in Firefox (#52700)

* ResizableFrame: Fix styling in Firefox

* Remove unused class

* Patterns: Fix empty general template parts category (#52747)

---------

Co-authored-by: Glen Davies <glen.davies@automattic.com>

---------

Co-authored-by: Carolina Nymark <myazalea@hotmail.com>
Co-authored-by: Andrea Fercia <a.fercia@gmail.com>
Co-authored-by: Riad Benguella <benguella@gmail.com>
Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com>
Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com>
Co-authored-by: James Koster <james@jameskoster.co.uk>
Co-authored-by: Andrei Draganescu <andrei.draganescu@automattic.com>
Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com>
Co-authored-by: Andrei Draganescu <me@andreidraganescu.info>
Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
Co-authored-by: Ramon <ramonjd@users.noreply.github.com>
Co-authored-by: Ben Dwyer <ben@scruffian.com>
Co-authored-by: Dave Smith <getdavemail@gmail.com>
Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com>
Co-authored-by: Lena Morita <lena@jaguchi.com>
Co-authored-by: Kai Hao <kevin830726@gmail.com>
Co-authored-by: Glen Davies <glen.davies@automattic.com>
getdave pushed a commit that referenced this pull request Jul 26, 2023
#52758 fixes the response from the fallback-navigation endpoint that did not have title.raw in the response, meaning we had to sometimes rely on title.rendered. This removes the extra 'or' statements that were added as hot fixes. The ones that still rely on title.rendered are due to the value needing to be checked within an object, or somewhere that the title property is not rendered via react.
jeryj added a commit that referenced this pull request Jul 28, 2023
#52758 fixes the response from the fallback-navigation endpoint that did not have title.raw in the response, meaning we had to sometimes rely on title.rendered. This removes the extra 'or' statements that were added as hot fixes. The ones that still rely on title.rendered are due to the value needing to be checked within an object, or somewhere that the title property is not rendered via react.
@mikachan mikachan added the [Block] Navigation Affects the Navigation Block label Aug 2, 2023
@SiobhyB SiobhyB removed the Needs PHP backport Needs PHP backport to Core label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging this pull request may close these issues.

8 participants