Add Post Content attributes as a block editor setting by tellthemachines · Pull Request #4614 · WordPress/wordpress-develop

tellthemachines · 2023-06-14T08:03:25Z

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

This PR adds the changes from WordPress/gutenberg#45299.

To test, checkout the PR locally and update the "@wordpress/editor" package to its latest version - "13.12.0". Then run the build, and use the Redux dev tools to inspect settings in the state of the core/block-editor store.

Edit: I somehow left out that this PR also adds the changes from WordPress/gutenberg#49049 🤦

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.

audrasjb · 2023-06-14T10:15:22Z

Thanks for the PR!
I added one commit to fix a few coding standards issues :)

spacedmonkey · 2023-06-14T10:17:23Z

Can you please add some unit tests to this PR.

jeremyfelt · 2023-06-14T17:35:39Z

+ * @param array  $blocks     Array of blocks.
+ * @return array Found block, or empty array if none found.
+ */
+function _wp_find_first_block( $block_name, $blocks ) {


I think this function could be useful for the general developer community as well. I know I've reinvented this wheel a few times already for extracting images/excerpts from post content on various projects. Can the pseudo-private _ prefix be removed?

Also, wp_find_ is used as a prefix only 3 times in core right now. As a developer, I generally expect WordPress functions that return data to use wp_get_.

tellthemachines · 2023-06-15T07:52:27Z

Thanks for fixing the coding standards stuff @audrasjb! I'm having issues running the composer scripts locally 😩

I've added a test for the now-renamed wp_get_first_block, now working on tests for _wp_get_post_content_block_attributes.

tellthemachines · 2023-06-16T01:56:46Z

I just added a base test case for _wp_get_post_content_block_attributes to check it returns an empty array by default (if there's no post ID and no block template). Not sure how to test this further, as it requires both a post and its block template to exist. I tried setting $post_ID =1 and switch_theme( 'block-theme' ); but that doesn't seem to work. I'm probably missing something obvious 😅 any help appreciated!

tellthemachines · 2023-06-16T02:31:01Z

Update: I got it working! Had to add a single template to the test block theme.

mukeshpanchal27

Thanks @tellthemachines, Left some nitpick feedback.

mukeshpanchal27 · 2023-06-16T06:24:16Z

+function _wp_get_post_content_block_attributes() {
+	$is_block_theme = wp_is_block_theme();
+
+	global $post_ID;


Missing docblock for global.

* @global int $post_ID

Nit-pick: It would be good to follow the pattern of putting the global declaration to the very top of the function code.

mukeshpanchal27 · 2023-06-16T06:26:49Z

+	if ( ! empty( _wp_get_post_content_block_attributes() ) ) {
+		$editor_settings['postContentAttributes'] = _wp_get_post_content_block_attributes();


_wp_get_post_content_block_attributes() call multiple time if condition true instead define a variable and use it.

Makes sense!

felixarntz

@tellthemachines Looks solid to me, +1 to having wp_get_first_block() as a generally available function.

I left a few small comments below, but none strictly a blocker (except probably the two documentation bits).

felixarntz · 2023-06-16T17:05:11Z

+function _wp_get_post_content_block_attributes() {
+	$is_block_theme = wp_is_block_theme();
+
+	global $post_ID;


Nit-pick: It would be good to follow the pattern of putting the global declaration to the very top of the function code.

tellthemachines · 2023-06-19T01:31:15Z

Thanks for the feedback folks!

Question on the tests: I added an extra assertion to test_get_block_editor_settings_theme_json_settings to check for the new setting, and in doing so realised that the test is now relying on the global set above in test_wp_get_post_content_block_attributes to pass. This isn't great as it could also alter results of any tests running after. Do we have any best practices for handling globals in tests? Should they be unset after each test?

tellthemachines · 2023-06-20T03:56:06Z

Do we have any best practices for handling globals in tests? Should they be unset after each test?

Update: @hellofromtonya has pointed me in the right direction and I've updated the tests accordingly.

andrewserong · 2023-06-20T06:46:12Z

 		do_action( 'rest_api_init', $wp_rest_server );
+
+		global $post_ID;
+		$this->orig_post_id = $post_ID;


Looks like this is throwing errors in the PHP 8.2 actions:

Would the fix be to define the orig_post_id property at the top of this class?

I wasn't sure, so I copied what's being done for the $wp_rest_server global and set it to null on teardown. It shouldn't matter because $post_ID shouldn't be set to any value at this point (if it was, test_wp_get_post_content_block_attributes wouldn't fail without it being set explicitly).

Looks good to me!

felixarntz

Thanks for the updates @tellthemachines, LGTM! Just the one nit-pick remaining, but not a big deal. Feel free to update before commit.

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

mukeshpanchal27

Thanks @tellthemachines the update. Look good to me.

mukeshpanchal27 · 2023-06-21T03:28:18Z

@tellthemachines Re-run the failed job it show Navigation timeout of 30000 ms exceeded .

tellthemachines · 2023-06-21T04:23:18Z

Thanks again for the reviews, everyone!

Committed in r55955 / b35b1fc.

Add Post Content attributes as a block editor setting

b993c63

tellthemachines requested review from dream-encode and getsource June 14, 2023 08:03

Fix coding standards issues

33a6cc9

jeremyfelt reviewed Jun 14, 2023

View reviewed changes

tellthemachines added 5 commits June 15, 2023 16:25

Remove dummy setting

4b484b3

Rename finding function and make public

4a4ea46

Add test for wp_get_first_block

405e499

Fix linting error

df0003b

fix further lint errors

e3fdf7d

Add test for _wp_get_post_content_block_attributes

1dbdbb6

tellthemachines added 2 commits June 16, 2023 12:26

Add a test to check for post content attributes

b7464cb

remove extra tab

1bbc1ab

tellthemachines requested a review from felixarntz June 16, 2023 05:53

mukeshpanchal27 reviewed Jun 16, 2023

View reviewed changes

felixarntz approved these changes Jun 16, 2023

View reviewed changes

tellthemachines added 5 commits June 19, 2023 11:00

Change order of wp_get_first_block parameters

fbd0981

Update function name and meta

a4f048a

Avoid calling function twice

727f135

Add test to check for post content attributes in editor settings

42a4d15

Remove extra whitespace

e206857

tellthemachines mentioned this pull request Jun 19, 2023

Plugin: Synchronize PHP changes for WordPress 6.3 release WordPress/gutenberg#51077

Closed

tellthemachines added 2 commits June 20, 2023 13:09

Test: handle globals in setup/teardown

67ae702

Fix lint errors

fd82019

tellthemachines added 2 commits June 20, 2023 13:57

try again

6a5e931

fix spacing

07e84c9

andrewserong reviewed Jun 20, 2023

View reviewed changes

Fix deprecation errors

c104d90

felixarntz approved these changes Jun 20, 2023

View reviewed changes

Comment thread src/wp-includes/block-editor.php Outdated

Move global to top of function.

0bcf57a

Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com>

mukeshpanchal27 approved these changes Jun 21, 2023

View reviewed changes

tellthemachines closed this Jun 21, 2023

tellthemachines mentioned this pull request Jun 26, 2023

Avoid declaring a function inside another function WordPress/gutenberg#49049

Merged

peterwilsoncc mentioned this pull request Jun 27, 2023

Backport layout block support layout definition and stabilisation changes for 6.3 #4624

Closed

2 tasks

		if ( ! empty( _wp_get_post_content_block_attributes() ) ) {
		$editor_settings['postContentAttributes'] = _wp_get_post_content_block_attributes();

Conversation

tellthemachines commented Jun 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

audrasjb commented Jun 14, 2023

Uh oh!

spacedmonkey commented Jun 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tellthemachines commented Jun 15, 2023

Uh oh!

tellthemachines commented Jun 16, 2023

Uh oh!

tellthemachines commented Jun 16, 2023

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tellthemachines commented Jun 19, 2023

Uh oh!

tellthemachines commented Jun 20, 2023

Uh oh!

andrewserong Jun 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 commented Jun 21, 2023

Uh oh!

tellthemachines commented Jun 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tellthemachines commented Jun 14, 2023 •

edited

Loading

andrewserong Jun 20, 2023 •

edited

Loading