Just checked into this and it looks like get_the_title() doesn't do any escaping, so this is Dangerous™. I'll have it fixed in a branch and add tests for it.

Inline comments must end in full-stops, exclamation marks, or question marks

There is a space after esc_html

Text domain ''gutenberg' is missing

I only see the date, no time

Huh, seems right. It used to say "timestamp" which is definitely wrong, but maybe it should be using a better date function. I'll check it out...

This is not valid for phpcs, see: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions

PHPCS warnings, see https://travis-ci.org/WordPress/gutenberg/jobs/403535665

Ah, cheers. They looked weird before with bizarre indentation but I guess that was the right way.

Yes, it looks a little bit strange.
You can check it local: https://wordpress.org/gutenberg/handbook/reference/coding-guidelines/#php

Type coercion already happens behind the block API (asType). We should drop the ad hoc validation here. See Gallery or Columns for examples of core blocks using RangeControl with no other type validation than the one built in.

I'm not a big fan of setAlignment and setCommentsToShow, as they just duplicate setAttributes.

I also don't love toggleAttribute, but don't feel strongly enough to oppose.

The Gallery block, for instance, has a lot of specific functions that just call setAttributes. I think the notion is explicitly setting one prop per control. It's usually the way I'd create functions for a component. 🤷‍♂️

The alternative is an inline function passed as a prop, right?

<RangeControl
	label={ __( 'Number of comments' ) }
	value={ commentsToShow }
	onChange={ this.setCommentsToShow }
	min={ MIN_COMMENTS }
	max={ MAX_COMMENTS }
/>

or

<RangeControl
	label={ __( 'Number of comments' ) }
	value={ commentsToShow }
	onChange={ ( value ) => ( this.setAttributes( 'commentsToShow', value ) ) }
	min={ MIN_COMMENTS }
	max={ MAX_COMMENTS }
/>

Hm, in both I only see drawbacks. 😄 Pick your poison, I say.

This poses the same interaction problems as when the Archives block was being developed. We should use the more robust Disabled element. See 480338c

Nice, I didn't code this bit but will do. 👍

(minor): edit,

I think we should have an eslint rule for that if it's important, but I'll change this one. 😄

Nop, totally minor. I prefer the shorthand. ¯_(ツ)_/¯

I do too… but we should have rules over preferences 😉

I've filed #8009.

I realize that the way we need to wrangle alignment types and do the getEditWrapperProps dance is suboptimal, but I'd rather have that addressed separately. Can we keep getEditWrapperProps here in line with other core blocks and revert exposing the constants?

Related is
#7908 (comment); before this the code copied what many other elements did and ignored centre-alignment, which caused the centre alignment option to do nothing.

I feel like it's better to do this as a starting point here and convert everything else over in #7908. This would be the way we'd want things to go, right?

This would be the way we'd want things to go, right?

Maybe, maybe not. :) But it deserves its own PR. I'd undo here.

Per my other comment, we should get rid of this, but worth noting that this would've been a disapproved import (importing private entities across packages).

I think I'm lost–how is it a private entity?

Because it's not a top-level export of editor, i.e. you wouldn't be able to get it via

import { DEFAULT_CONTROLS } from '@wordpress/editor';

and we avoid importing across blocks in any other fashion (i.e. a module in core-blocks importing from editor). There were some exceptions to this due to circular dependencies, but I think they're all but solved.

Aha, gotcha.

Has there been discussion about a custom rule to catch that for us or has it been difficult to implement? Seems quite easy to miss in a review.

There are rules against import { Bla } from 'editor'; (and any other package), but none for deep access. I'm assuming we don't yet forbit deep access because of those unresolved circular dependencies we had-and-may-still-have. Someone may correct me on this. :)

That won't actually work (eslint throws an error), but I have a PR incoming.

That won't actually work (eslint throws an error), but I have a PR incoming.

Yeah, might be the same thing I've encountered before, where it was solved by including the unicode equivalent character of the slash.

Uncanny! Do you have a matcher for lint?

I actually have no idea how this notification came through to my inbox 😅

Whoa, is that what that is? If it works I'll add a comment for it this time 😅

zomg that worked, thanks. I'll add it and add a comment this time 😉

_draft_or_post_title already calls esc_html, though I can accept if you'd rather also escape here for reviewers peace of mind.

I think I originally added the esc_html (or it was here already?) because we weren't using _draft_or_post_title. I suppose now that it's being used we can omit the extra escape.

In general I like seeing it but in this case I'll leave a comment explaining that _draft_or_post_title does the escaping. No need to run it twice.

This function doesn't toggle anything on its own, it creates a function which performs the toggle. More accurately, it might be named createToggleAttribute.

toggleAttribute returns a new function every invocation, causing React to do render reconciliation more often than we need it to. I wonder if we should pre-bind these functions in the constructor:

constructor() {
	// ...

	this.toggleDisplayAvatar = this.createToggleAttribute( 'displayAvatar' );
}

Yeah, this has been pointed out elsewhere that this pattern is bad for unneeded re-renders, though obviously the code is nice.

Pre-binding them in the constructor is a good pattern actually 👍

IMO we should JSDoc any constant, even just to establish the convention in encouraging the practice for less-obvious constants.

I'm cool with that 👍

Makes me wonder: Is there any case where we want ServerSideRender to not be disabled? Should this be built-in to the render logic of that component?

I wondered that too, we should probably open an issue for it. I suppose building it in and then having some escape hatch prop that doesn't disable it would be good.

Why do we need this context?

I bet we don't; I inherited this CSS and I guess I didn't refactor enough. It can go. 👍

As I understand it, scoping in PHP won't work as expected with what we have here. This would need to be accessed by global, which we're not doing.

Simpler demonstration:

Before:

<?php

$extra = 'bar';

function do_foo() {
	echo 'foo' . $extra;
}

do_foo();

⇒ php test.php
foo%        

After:

<?php

$extra = 'bar';

function do_foo() {
	global $extra;
	echo 'foo' . $extra;
}

do_foo();

⇒ php test.php
foobar%     

Though, as written, it's more of a constant, which is exemplified in define.

Though, above all, does this really need to be in the global scope? I think we're over-optimizing at the risk of outside breakage (anything else in the running WordPress page could access this variable, intentionally or not).

Yeah I didn't think using define was right because I wanted a constant scoped to this file–and the constants created by define are global... right?

I'll admit that my PHP is wildly rusty and I'm totally unclear what the best practice is for this.

I just want a file-scoped constant. I can remove it if that's a pain to make.

and the constants created by define are global... right?

Yes, and so is the previous variable assignment, as can be demonstrated:

diff --git a/core-blocks/latest-comments/index.php b/core-blocks/latest-comments/index.php
index 33c21e41f..092fe08eb 100644
--- a/core-blocks/latest-comments/index.php
+++ b/core-blocks/latest-comments/index.php
@@ -9,6 +9,7 @@
 // appearing with no title.
 require_once( ABSPATH . 'wp-admin/includes/template.php' );
 
+$default_comments_to_show = 5;
 define( 'GUTENBERG_LATEST_COMMENTS_BLOCK_DEFAULT_TO_SHOW', 5 );
 
 /**
diff --git a/lib/load.php b/lib/load.php
index f1a8b09c6..f8a4b7985 100644
--- a/lib/load.php
+++ b/lib/load.php
@@ -34,3 +34,5 @@ require dirname( __FILE__ ) . '/register.php';
 foreach ( glob( dirname( __FILE__ ) . '/../core-blocks/*/index.php' ) as $block_logic ) {
 	require $block_logic;
 }
+
+var_export( $default_comments_to_show ); exit;

Related: http://php.net/manual/en/language.variables.scope.php

Yeah, I didn't realise that 😢

If the define call is too global I'm fine with just hard-coding it in. 🤷‍♂️

Since it's prefixed enough it shouldn't be a huge concern.

Alternatively, we could consider enhancing our JSON schema support to handle minimum and maximum, rely on the validation behavior, and assign the default only once in the block registration.

https://spacetelescope.github.io/understanding-json-schema/reference/numeric.html#range

In fact, since we use REST validation under the hood, I wonder if this is already possible.

It's already supported:

https://github.com/WordPress/wordpress-develop/blob/8f95800d52c1736d651ae6e259f90ad4a0db2c3f/src/wp-includes/rest-api.php#L1169-L1209

Our usage will trigger the default to take its place when outside bounds:

There's also enum to take care of the align valid options:

https://github.com/WordPress/wordpress-develop/blob/8f95800d52c1736d651ae6e259f90ad4a0db2c3f/src/wp-includes/rest-api.php#L1120-L1125

https://spacetelescope.github.io/understanding-json-schema/reference/generic.html#enumerated-values

Huh, nice! Okay, I'll look at that.