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

Navigate to entity record crash editor #64814

Closed
2 tasks done
Tropicalista opened this issue Aug 27, 2024 · 33 comments · Fixed by #67086
Closed
2 tasks done

Navigate to entity record crash editor #64814

Tropicalista opened this issue Aug 27, 2024 · 33 comments · Fixed by #67086
Assignees
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@Tropicalista
Copy link

Description

I have a custom post type which I wanna use as a pattern.

If I try something like this, the editor crashes.

	const handleEditForm = () => {
		onNavigateToEntityRecord( {
			postId: ref,
			postType: 'formello',
		} );
	};

The error is:

Uncaught TypeError: Cannot read properties of null (reading 'ownerDocument')
    at apply (rich-text.js?ver=9988fecf32c29c7254e1:2492:18)
    at applyRecord (rich-text.js?ver=9988fecf32c29c7254e1:3709:5)
    at applyFromProps (rich-text.js?ver=9988fecf32c29c7254e1:3806:5)
    at rich-text.js?ver=9988fecf32c29c7254e1:3813:7
    at commitHookEffectListMount (react-dom.development.js:23184:26)
    at commitLayoutEffectOnFiber (react-dom.development.js:23302:17)
    at commitLayoutMountEffects_complete (react-dom.development.js:24722:9)
    at commitLayoutEffects_begin (react-dom.development.js:24708:7)
    at commitLayoutEffects (react-dom.development.js:24646:3)
    at commitRootImpl (react-dom.development.js:26857:5)

Step-by-step reproduction instructions

  1. Create a custom post type (ex. Doc)
  2. Create a new post of custom post type (New doc)
  3. Create a simple block that mimic what patterns do and add function mentioned above
  4. Editor crashes

Screenshots, screen recording, code snippet

No response

Environment info

  • WP 6.6

Please confirm that you have searched existing issues in the repo.

  • Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

  • Yes
@Tropicalista Tropicalista added the [Type] Bug An existing feature does not function as intended label Aug 27, 2024
@Mamaduka Mamaduka added the Needs Testing Needs further testing to be confirmed. label Aug 28, 2024
@Mamaduka
Copy link
Member

@Tropicalista, can you provide code for a simple block that mimics the Patterns (Reusable) block? The issue might be with it.

@miminari miminari added the [Status] Needs More Info Follow-up required in order to be actionable. label Sep 10, 2024
Copy link

Help us move this issue forward. This issue is being marked stale since it has no activity after 15 days of requesting more information. Please add info requested so we can help move the issue forward. Note: The triage policy is to close stale issues that need more info and no response after 2 weeks.

@github-actions github-actions bot added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 26, 2024
@Mamaduka Mamaduka removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 27, 2024
@Tropicalista
Copy link
Author

This is solved with Gutenberg 19.4.0

@Tropicalista
Copy link
Author

This come out again with wp 6.7 RC 1.

The above error occurred in the <ForwardRef(PostTitle)> component:

    at PostTitle (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:20825:53)
    at div
    at div
    at WritingFlow (http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:43957:3)
    at div
    at BlockTools (http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:58576:3)
    at ExperimentalBlockCanvas (http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:59136:3)
    at div
    at Resizable (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:62986:28)
    at UnforwardedResizableBox (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:64031:3)
    at ResizableEditor (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:27050:3)
    at div
    at VisualEditor (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:27266:3)
    at SlotComponent (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33416:5)
    at Slot
    at UnforwardedSlot (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33976:5)
    at EditCanvasContainerSlotSlot
    at div
    at http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:9868:3
    at div
    at div
    at div
    at InterfaceSkeleton (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:9958:3)
    at EditorInterface (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:27596:3)
    at BlockRefsProvider (http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:27719:3)
    at Provider (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33991:3)
    at http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:36995:5
    at http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:36539:3
    at BlockContextProvider (http://localhost:10005/wp-includes/js/dist/block-editor.js?ver=ba158b1bfcaf2bda05b0:37058:3)
    at EntityProvider (http://localhost:10005/wp-includes/js/dist/core-data.js?ver=c8526dfd5ea638e51031:6994:3)
    at EntityProvider (http://localhost:10005/wp-includes/js/dist/core-data.js?ver=c8526dfd5ea638e51031:6994:3)
    at http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:24561:3
    at http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:21964:3
    at Editor (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:29324:3)
    at div
    at ErrorBoundary (http://localhost:10005/wp-includes/js/dist/editor.js?ver=f84c1fcefdc6332efbd1:11688:5)
    at SlotFillProvider (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33829:3)
    at provider_SlotFillProvider (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33927:3)
    at Provider (http://localhost:10005/wp-includes/js/dist/components.js?ver=75e0765bccdbc4da087e:33991:3)
    at Layout (http://localhost:10005/wp-includes/js/dist/edit-post.js?ver=9567d2bb2ad838a6bbda:3116:11)

React will try to recreate this component tree from scratch using the error boundary you provided, ErrorBoundary.

@Mamaduka Mamaduka reopened this Oct 23, 2024
@Mamaduka
Copy link
Member

@Tropicalista, the issue is fixed in Gutenberg 19.4 but not in WP 6.7 RC1. Is that correct?

@Tropicalista
Copy link
Author

Yes it's correct. Can this be related?

@Tropicalista
Copy link
Author

Tropicalista commented Oct 25, 2024

To clarify things, onNavigateToEntityRecord works on 19.3 and up. It does not work on WP 6.7-RC1 without Gutenberg.

I'm testing with formello plugin. The code I'm using to test this up is here.

From what I see the wp 6.7 should use Gutenberg 19.3. Am I correct?

@t-hamano
Copy link
Contributor

I tested it with the following steps and I think I was able to reproduce it.

  1. Start WordPress 6.7 RC1 environment
  2. Activate the Formllo plugin
  3. Switch Gutenberg branch to release/19.5
  4. Deactivate Gutenberg plugin
  5. Create a new form from the Forms menu
  6. Add the form to the post editor
  7. Click "Edit original" from the block toolbar
  8. ❌ The editor crashes.
  9. Activate Gutenberg plugin
  10. Perform steps 6-7
  11. ✅ The editor does not crash

Interestingly, this issue does not occur in the release/19.5 branch of Gutenberg, but the editor crashes in the trunk (86a3bc9). So, the result is as follows:

  • ❌ WordPress 6.7 RC1
  • ✅ Gutenberg release/19.5 branch
  • ❌ Gutenberg trunk (86a3bc9)

It is still unclear whether this is a plugin issue or a Gutenberg issue.

77987f469265bcfce0e12a0eac1dd11e.mp4

@t-hamano
Copy link
Contributor

@Tropicalista I tested this issue again with minimal code and am unable to reproduce the issue in 6.7 RC1.

The following code creates a custom post called "Book" and tests navigating to the post via a block.

Details

PHP:

add_action(
	'init',
	function () {
		register_post_type(
			'book',
			array(
				'public'       => true,
				'label'        => 'Books',
				'show_in_rest' => true,
			)
		);
	}
);

JavaScript:

wp.blocks.registerBlockType( 'test/navigate-entity-test', {
	apiVersion: 3,
	title: 'Navigate Entity Test',
	edit: () => {
		const { onNavigateToEntityRecord } = wp.data
			.select( 'core/block-editor' )
			.getSettings();
		return wp.element.createElement(
			'div',
			wp.blockEditor.useBlockProps(),
			wp.element.createElement(
				'button',
				{
					onClick: () => {
						onNavigateToEntityRecord( {
							postId: 8, // Replace with the actual post ID
							postType: 'book',
						} );
					},
				},
				'Navigate to Custom Post Type (Books) entity'
			)
		);
	},
} );
304896d44987cad810d2895b78f4fb78.mp4

Is your code attempting to access a variable or element that may be undefined?

@t-hamano t-hamano removed the Needs Testing Needs further testing to be confirmed. label Oct 29, 2024
@ndiego
Copy link
Member

ndiego commented Oct 29, 2024

Moving this back to "In Discussion" on the 6.7 project board for the time being.

@Tropicalista
Copy link
Author

I think I found the problem.

This code works. But of course it will not display the children

export default function ReusableBlockEdit( {
	attributes: { ref },
	clientId,
	name,
	setAttributes,
} ) {
	const [ isModalOpen, setModalOpen ] = useState( false );
	const [ isDisabled, setIsDisabled ] = useState( true );
	const [ titleTemp, createTitle ] = useState( '' );

	const { onNavigateToEntityRecord } = useSelect(
		( select ) => {
			const {
				getSettings,
			} = select( blockEditorStore );

			// For editing link to the site editor if the theme and user permissions support it.
			return {
				onNavigateToEntityRecord:
					getSettings().onNavigateToEntityRecord,
			};
		},
		[ clientId, ref ]
	);

	const handleEditOriginal = () => {
		onNavigateToEntityRecord( {
			postId: ref,
			postType: 'formello_form',
		} );
	};

	const blockProps = useBlockProps();

	const { children, ...innerBlocksProps } = useInnerBlocksProps( blockProps );

	return (
		<div { ...innerBlocksProps }>
			<BlockControls>
				<ToolbarGroup>
					<ToolbarButton onClick={ handleEditOriginal }>
						{ __( 'Edit form', 'formello' ) }
					</ToolbarButton>
				</ToolbarGroup>
			</BlockControls>
			{ children }
		</div>
	);
}

IT will crash if I change the code to something like this:

	const [ blocks, onInput, onChange ] = useEntityBlockEditor(
		'postType',
		'formello_form',
		{ id: ref }
	);

	const innerBlocksProps = useInnerBlocksProps( blockProps, {
		value: blocks,
		onInput,
		onChange,
		allowedBlocks: [ 'formello/form' ],
		renderAppender: blocks?.length
			? undefined
			: InnerBlocks.ButtonBlockAppender,
	} );

Do you have any idea of what should I do to prevent this error? Is it a bug or is my code worng?

@t-hamano
Copy link
Contributor

t-hamano commented Nov 1, 2024

I'm still not sure if this is a problem with Gutenberg itself.

@Tropicalista

  • Just to be sure, does this issue occur in 6.7 RC2?
  • Could you enable SCRIPT_DEBUG and tell us the error you get?
  • Could you provide the minimal but complete Edit component code that causes the crash?

@Tropicalista
Copy link
Author

Tropicalista commented Nov 1, 2024

@t-hamano I have uploaded a demo repo that you can test here: Navifate to entity

Link to repo: https://github.com/Tropicalista/navigate-to-entity/tree/master

  • I have created a super simple post type book.
  • I have created a simple pattern
  • I have created a simple block that use navigate to entity
  • I have created a blog post to embed both
  • The pattern works, the custom post type does not.

To be clear, if I use the code you shared before, without a children block, it works.

The code of the block is very simple and you can see it here: https://github.com/Tropicalista/navigate-to-entity/blob/master/src/edit.js

@t-hamano
Copy link
Contributor

t-hamano commented Nov 3, 2024

@Tropicalista Thanks for the detailed info. I was able to track down the PR related to this issue.

To get straight to the point, I believe this is a Gutenberg issue and should be fixed in WP 6.7.

Step-by-step reproduction instructions

First, I would like to explain again how to test this issue:

  • Upload and activate this plugin. It will create a post type called "Book".
  • Create a new Book post. Make a note of its post ID.
  • Enter the following code into the post editor, replacing the ref value with the post ID you noted earlier:
    <!-- wp:test/sample {"ref":1,"postType":"book"} -->
    <p class="wp-block-test-sample">Hello from the saved content!</p>
    <!-- /wp:test/sample -->
  • Click the "Edit book" button in the block toolbar.
  • This will crash the editor. However, if you reload your browser the post will display correctly.

Screenshots, screen recording, code snippet

26e6776611982fd8665e9eb405a6dc89.mp4

When was this issue resolved and when did it reoccur?


So it looks like a regression that occurred during the WP 6.7 cycle.

@SantosGuillamot @gziolo Do you know anything about this issue?

@Tropicalista
Copy link
Author

Can be this line?

@gziolo
Copy link
Member

gziolo commented Nov 4, 2024

I performed some debugging at these are my findings.

Can be this line?

Removing the logic as follows doesn't fix the issue:

diff --git a/packages/editor/src/components/provider/index.js b/packages/editor/src/components/provider/index.js
index 50d0261006..623cb847be 100644
--- a/packages/editor/src/components/provider/index.js
+++ b/packages/editor/src/components/provider/index.js
@@ -173,17 +173,13 @@ export const ExperimentalEditorProvider = withRegistryProvider(
 						getRenderingMode,
 						__unstableIsEditorReady,
 					} = select( editorStore );
-					const { getEntitiesConfig } = select( coreStore );
 
 					return {
 						editorSettings: getEditorSettings(),
 						isReady: __unstableIsEditorReady(),
 						mode: getRenderingMode(),
 						selection: getEditorSelection(),
-						postTypeEntities:
-							post.type === 'wp_template'
-								? getEntitiesConfig( 'postType' )
-								: null,
+						postTypeEntities: null,
 					};
 				},
 				[ post.type ]

Adding an additional call for getPostTypes resolves the issue:

diff --git a/packages/editor/src/components/provider/index.js b/packages/editor/src/components/provider/index.js
index 50d0261006..dba4e86178 100644
--- a/packages/editor/src/components/provider/index.js
+++ b/packages/editor/src/components/provider/index.js
@@ -173,8 +173,9 @@ export const ExperimentalEditorProvider = withRegistryProvider(
 						getRenderingMode,
 						__unstableIsEditorReady,
 					} = select( editorStore );
-					const { getEntitiesConfig } = select( coreStore );
-
+					const { getEntitiesConfig, getPostTypes } =
+						select( coreStore );
+					getPostTypes( { per_page: -1 } );
 					return {
 						editorSettings: getEditorSettings(),
 						isReady: __unstableIsEditorReady(),

It means that the plugins shared above depend on some internal implementation of the post editor, which was never guaranteed in WordPress core. To me, it feels like the missing part is populating the list of all available post types for this functionality to be fully functional.

@SantosGuillamot
Copy link
Contributor

I have similar observations after testing it. I don't think it is a bug caused by the mentioned pull request, but it was potentially covering the original issue.

From my triaging, it definitely seems it is a problem caused because the postType is not available yet when doing the navigation. To simplify the testing I was running this code directly in the console:

wp.data.select('core/block-editor').getSettings().onNavigateToEntityRecord( { postId: 16, postType: 'book' } )

Running just that code triggers the error. However, if just before that I run the following:

wp.data.select('core').getPostType('book');

It navigates as expected.

I'm wondering if this should be handled automatically by the core function and make it wait until the postType is available. I've seen we are already calling that function here. Maybe we can reuse that logic somehow. The rendering logic seems to happen in that file.

@gziolo
Copy link
Member

gziolo commented Nov 4, 2024

I also ran a test with WordPress 6.6 and installed the Formello plugin (highlighted here). The same issue has existed for quite some time. I agree with @SantosGuillamot that the potential fix should be applied on the onNavigateToEntityRecord level.

@Tropicalista
Copy link
Author

With Gutenberg version 16.3 and up, this it's working. Why not backport?

@getdave
Copy link
Contributor

getdave commented Nov 4, 2024

The backport to 6.7 which "regressed" this fixed a performance issue so I'm not sure a simple backport is possible. We need a more tailored fix (as suggested above).

@SantosGuillamot
Copy link
Contributor

With Gutenberg version 16.3 and up, this it's working. Why not backport?

I'm still seeing the problem with Gutenberg trunk.

Some potential solutions I see to the original issue:

  • Don't load the edit-post component until the post type is loaded.

We could include a call to getPostType in this useSelect and only load the component when it is defined. Something like this:

With Gutenberg version 16.3 and up, this it's working. Why not backport?
isPostTypeLoaded: !! getPostType( currentPostType ),

I have tested it, and it seems to "work" as a quick solution. However, I believe we should try to go one step back.

  • Go one step back and load the post type before navigating.
    Ideally, we should call getPostType just before this dispatch. But I am not sure how to do that.

@getdave
Copy link
Contributor

getdave commented Nov 4, 2024

Update: I have a PoC PR in #66709.


Would this await resolveSelect pattern work?

const convertClassicMenuToBlockMenu = useCallback(
async ( menuId, menuName, postStatus = 'publish' ) => {
let navigationMenu;
let classicMenuItems;
// 1. Fetch the classic Menu items.
try {
classicMenuItems = await registry
.resolveSelect( coreStore )
.getMenuItems( {
menus: menuId,
per_page: -1,
context: 'view',

Something like:

const onNavigateToEntityRecord = useCallback(
	async ( params ) => {
		await registry
			.resolveSelect( coreStore )
			.getPostType( params.postType );
		dispatch( {
			type: 'push',
			post: { postId: params.postId, postType: params.postType },
			// Save the current rendering mode so we can restore it when navigating back.
			previousRenderingMode: getRenderingMode(),
		} );
		setRenderingMode( defaultRenderingMode );
	},
	[ registry, getRenderingMode, setRenderingMode, defaultRenderingMode ]
);

In my testing this fixes the Issue reported when using that Plugin.

@SantosGuillamot
Copy link
Contributor

I believe something like that could work and it seems to solved the issue in my testing 🙂

@getdave
Copy link
Contributor

getdave commented Nov 4, 2024

Let's work on refining #66709. It's currently targetting wp/6.7 but should it be targetting trunk?

@getdave getdave moved this from 🗣️ In Discussion / Needs Decision to 🏗️ In Progress in WordPress 6.7 Editor Tasks Nov 4, 2024
@gziolo gziolo removed the [Status] Needs More Info Follow-up required in order to be actionable. label Nov 4, 2024
@ndiego ndiego moved this from 🏗️ In Progress to 🐛 Punted to 6.7.1 in WordPress 6.7 Editor Tasks Nov 6, 2024
@Tropicalista
Copy link
Author

The fix is still not working on site editor.

Simply copy the two demo from a post and try to insert in template in site editor.

Try in playground: Navifate to entity

@mijan-xs
Copy link

In Gutenberg 19.4, this issue was resolved, but it has reappeared in version 19.6.1. Please investigate.

@getdave
Copy link
Contributor

getdave commented Nov 14, 2024

The fix is still not working on site editor.

@Tropicalista I don't think your Playground is using the Gutenberg PR which fixes the Issue?

Try this Playground which activates the Gutenberg PR.

Here's a screencapture of me showing it working:

Screen.Capture.on.2024-11-14.at.15-32-59.mp4

@Tropicalista
Copy link
Author

@getdave You are right.

I thought that this was merged because issue title says 6.7.

@Mamaduka
Copy link
Member

Has anyone figured out why the bug affects a third-party plugin but not the core Patterns block?

While #66709 is a good hotfix, it's still a compromise. There should be no need to pre-select data before navigation; the editor should do it for consumers as needed (after navigation).

@youknowriad, @jsnajdr, what do you think?

@youknowriad
Copy link
Contributor

The reason is probably because patterns are loaded on every editor, so the post type is already there. But I agree that we need to solve the root issue. (Wait for the post type at the moment that we need it) instead of adding a fetch call in the onNavigate callback because that's probably just one way to trigger the issue.

@getdave
Copy link
Contributor

getdave commented Nov 18, 2024

I would be happy to close #66709. I don't like changing from sync to async in that PR and as you say the editor should be able to request the entities on demand when it navigates rather than having to have them ready in advance.

@jsnajdr
Copy link
Member

jsnajdr commented Nov 18, 2024

This is a bug caused by useRichText returning a ref that is used on an element that is rendered conditionally. The Post Title component does this:

const { ref } = useRichText();
return (
  <PostTypeSupportCheck supportKeys="title">
    <h1 ref={ ref } />
  <PostTypeSupportCheck />
);

When navigating from a post of type post to a post of type book, the PostTypeSupportCheck briefly unmounts the h1 element because it internally fetches the post type for book. Only after the post type is loaded does it support title again.

The useRichText logic, namely the applyFromProps calls, get very confused when the ref is reset to null. Then a useLayoutEffect that calls applyFromProps has no element to work with.

I tried to fix this by setting didMountRef.current = false when the element unmounts. This fixes the crash, but it introduces another bug: the post title is not updated. When navigation from the "post" post to "book" post, the UI still shows the "post" title. Maybe @ellatrix can help here.

This all also explains why @getdave's fix in #66709 involves calling getPostType. By preloading the post type, it avoids unmounting the h1 in PostTypeSupportCheck.

The bug can also be fixed by wrapping all post title logic inside PostTypeSupportCheck, including the useRichText call. But that works around a bug in useRichText that would still remain there.

@cbravobernal
Copy link
Contributor

Cherry-picked to 6.7 branch for a 6.7.2 release.

@cbravobernal cbravobernal moved this from 📥 Todo to ✅ Done in WordPress 6.7.x Editor Tasks Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects