Set block weight during Place Block flow so it's always at the top

Since this isn't about testing the block creation ui, yes, I think block creation can use entity calls.

That looks really great (fewer lines when using the trait).

I'm gonna review this.

in the meantime, here is a tests only patch with the new trait changes.

+++ b/core/modules/block_place/tests/src/Functional/BlockPlaceTest.php
@@ -69,8 +72,53 @@ public function testPlacingBlocksUnprivileged() {
+    $this->placeBlock($block_plugin_id, ['region' => 'sidebar_first', 'weight' => -5]);
+    $this->placeBlock($block_plugin_id, ['region' => 'sidebar_second', 'weight' => 3]);
...
+    foreach (['sidebar_first' => -6, 'sidebar_second' => 2] as $region => $expected_weight) {

There is only one fail in the test.

fail: [Other] Line 83 of core/modules/block_place/tests/src/Functional/BlockPlaceTest.php:
Failed asserting that 0 matches expected -6.

Is this because for the weight == 3 case, the default happened to be.. 0? so it was added first in that case anyway? I want to make sure we have (all) the fails we were expecting.

Reading the test, (and this might be ok), it seems the test is testing for weight of block added being a specific weight (one less than the lightest block existing in the region), and not that the block is "listed first". Listed first is probably more difficult to test.

Manual test of adding who's online block to the footer, shows that without this patch, it gets added in the middle, and with this patch, it gets added first in that region.

just whitespace nits.

https://www.drupal.org/node/608152#indenting

(while it says what to do between a class and property/method, and doesn't say there what to do between a class and use trait, a grep shows there is much more often in core a newline between the class and use trait.)

11022 → ag -B3 "class.*\n.*\n.*use.*Trait" core  | wc -l
    2322
11032 → ag -B3 "class.*\n.*use.*Trait" core  | wc -l
     470

the removed empty line before the class closing brace, though, should be put back (though also not blocking)

--

I looked but could not (re)find the issue for (proposed, and thus non-blocking) standard on ordering (alphabetical, with case complications) of use statements

--

Only potential real block here, is question about test fails in #18

hmm, bot now sets to CNW if the 2nd patch fails?

Yeah, the test only patch has to be the first one.

+++ b/core/modules/block_place/tests/src/Functional/BlockPlaceTest.php
@@ -69,8 +73,69 @@ public function testPlacingBlocksUnprivileged() {
+    $links = $this->xpath('//a[contains(@href, :href)]', [':href' => $block_library_url->toString()]);
+    $this->assertEquals(1, count($links));
...
+    $links = $this->xpath('//a[contains(@href, :href)]', [':href' => $block_add_url->toString()]);
+    $this->assertEquals(1, count($links));

Couldn't you use $this->assertSession()->linkByHrefExists() and then simply use $this->drupalGet($block_library_url)?

Ah that makes sense!
What about doing some quick comment about that?

Thank you @pwolanin!

As a change to a new experimental module, his is an rc-eligible change per: https://www.drupal.org/core/d8-allowed-changes#rc

1. +++ b/core/modules/block_place/tests/src/Functional/BlockPlaceTest.php
   @@ -69,8 +73,69 @@ public function testPlacingBlocksUnprivileged() {
   +    // Enable a standard block in first and second sidebars with set weights.
   

   with the change to use data provider, this comment probably needs to be changed and/or moved to the provider.

2. +++ b/core/modules/block_place/tests/src/Functional/BlockPlaceTest.php
   @@ -69,8 +73,69 @@ public function testPlacingBlocksUnprivileged() {
   +    $this->placeBlock($block_plugin_id, ['region' => $region, 'weight' => $weight]);
   +    if ($extra_block_at_zero) {
   +      $this->placeBlock($block_plugin_id, ['region' => $region, 'weight' => 0]);
   

   what is $extra_block_at_zero for?

For #32 and #33.

Did 1.

I think the answer to 2. is that was a way to have two blocks sometimes in the test, and that zero can be a special case sometimes, so made sense to have a test case with a block whose weight is zero.

I also added a period to the comment in the data provider. Grepping says we almost always do that (152 out of 164 have the period).

10300 → ag "Data provider for.*\." core | wc -l
     152
10301 → ag "Data provider for" core | wc -l
     164

The added test (and data for it) didn't mention anywhere what the expected weight meant (on top). So I added that, and an @see to the method where the weight of "on top" is set.

Took out the custom message in the assert (the default message having the values is nice), moved the info to comments. [edit: or ... did we fix that, so we always get the default message (with the values) now.]

[edit: re #33, I see. I think that's fine.]

+++ b/core/modules/block_place/tests/src/Functional/BlockPlaceTest.php
@@ -69,8 +73,81 @@ public function testPlacingBlocksUnprivileged() {
+    $this->assertEquals(1, count($links));
...
+    $this->assertEquals(1, count($links));

I'm not clear on why we have asserts here that the links are found. Is what we are testing (asserting) that the weight is in the urls as a query param?

Great work, love these small improvements!

patch still applies. not sure what the test fail was. sending for retesting.

Updating the component of this issue to "block_place.module", which was added at drupalcon baltimore.

I'm attempting to sort out how this issue relates to https://www.drupal.org/node/2735277, which is a "must have" to graduate the place_block module (found here: https://www.drupal.org/node/2739075).

@ted Can you take a look at this?

The status of the issue appears to be correct as-is.

I have applied the patch found in comment #35.

I can confirm that the block is first when originally placed into a region after saving. Upon saving, the weight of the block differs from that of what was attempted to be saved. This can be demonstrated by just clicking "save" on the block layout form without changing the block weight (it should be first).

Here is a video to show what I've found: https://youtu.be/n9oiXzO7QKU

It may be worthwhile revisiting this issue if #2784495: Normalize block place and outside-in experiences gets committed. The workflow will be changing some and it's not clear to me how it will impact the work done here.

As such, I'm marking this as "Postponed" and updating the issue status.

Block place was superseded by layout builder in 8.7 moving to contrib,