Add non-UI mechanism for setting block weight through block forms

Yes! Can we get a screenshot?

1. +++ b/core/modules/block/src/BlockForm.php
   @@ -180,6 +180,13 @@ public function form(array $form, FormStateInterface $form_state) {
   +    // Hidden weight setting.
   

   not a sentence. oh, I see this is the pattern in this method already. ok.

2. +++ b/core/modules/block/src/Controller/BlockLibraryController.php
   @@ -119,6 +119,8 @@ public function listBlocks(Request $request, $theme) {
   +    $destination = $this->redirectDestination->get();
   
   @@ -144,7 +146,9 @@ public function listBlocks(Request $request, $theme) {
   -      $destination = $this->redirectDestination->get();
   

   this seems like an unrelated improvement.

3. +++ b/core/modules/block/src/Tests/BlockTest.php
   @@ -172,6 +172,51 @@ public function testAddBlockFromLibrary() {
   +    foreach (['7', '-9'] as $weight) {
   

   oh, these are the only two values... maybe add zero, zero sometimes is a "funny" special case.

4. +++ b/core/modules/block/src/Tests/BlockTest.php
   @@ -172,6 +172,51 @@ public function testAddBlockFromLibrary() {
   +      $links = $this->xpath('//a[contains(@href, :href)]', [':href' => $add_url->toString()]);
   +      $this->assertEqual(1, count($links), 'Found one matching link');
   

   this assert seems like a sanity check, and not anything to do with the weight.

5. +++ b/core/modules/block/src/Tests/BlockTest.php
   @@ -172,6 +172,51 @@ public function testAddBlockFromLibrary() {
   +      $this->assertEqual(t('Place block'), (string) $links[0]);
   +      $this->assertEqual($weight, $query_parts['weight'], 'Expected weight query string is in href');
   

   are these two asserting the same thing? that the weight query is in the link?

6. +++ b/core/modules/block/src/Tests/BlockTest.php
   @@ -172,6 +172,51 @@ public function testAddBlockFromLibrary() {
   +      // Ensure that the block was created.
   +      /** @var \Drupal\block\BlockInterface $block */
   +      $block = Block::load($block_id);
   +      $this->assertEqual($title, $block->label(), 'Found the block with expected title.');
   +      $this->assertEqual($weight, $block->getWeight(), 'Found the block with expected weight.');
   

   this test was copied from testAddBlockFromLibrary(), so that's where this comment came from, but I think the point in this test, is to ensure it has the weight, not only that it was created.

7. +++ b/core/modules/block/src/Tests/BlockUiTest.php
   @@ -270,6 +271,27 @@ public function testMachineNameSuggestion() {
   +   * Tests the block placement UI.
   +   */
   +  public function testBlockPlacementUI() {
   

   this seems like unrelated improved test coverage, separate issue.

I looked at every line changed in the patch, and discussed things with @pwolanin in IRC.

I think the changes here are now within scope (though related issues would be nice for the removed improvements)
and make sense to me.

Also looked at https://www.drupal.org/core/d8-allowed-changes#beta
allowed changes in beta, and I think this is 8.2.x because either
"minor API additions or ..., if the impact outweighs the disruption"
or
"Certain other issues with high impact and low disruption at committer discretion only."

so... good from me for now, rtbc.

Is this enough? When ordering blocks in the current block UI we also change the weights of the surrounding blocks.

Just retitling to hopefully cut down on confusion.

@pwolanin

Re: #2739079: Reduce visual clutter of Place Block module

the change I'm proposing has no impact on the UI of the modals, only passes data along in the query string. Check out the patch.

The fact that this patch has no visible UI change does not ipso facto imply "we don't need to look at a design". The attitude that we can write the code and 'paint it with some UX' after the fact is a recipe for wasted effort and technical debt.

Different designs imply different data models. Imagine, for example that we had a design that included a button to 'maintain relative position' when moving a block from one region to another eg. based on the blocks visible to the current user, if in region A, block 1 is third from the top, when I move it to region B, it should still be third from the top. That's not a feature I'm currently designing, but if it were I'm pretty certain that this patch would need major refactor to handle it.

See #2793053: Optimize repeated method call in BlockLibraryController for the destination optimization that was descoped in #13 (see interdiff)

(rerolls should be "easy" on either issue, context lines kind of thing.)

@pwolanin

Spoke to Emily in UX meeting and she's confident in the direction, so I'll take her word for it.

Thanks everyone!

@tkoleary, this patch has no impact on the UI whatsoever (nor the data model, which also has nothing to do with the UI anyway. Data model ≠ data modeling tools.) There's nothing to "paint with UX" because it's just internal API support for future feature additions to the form workflow for placing blocks. Any followups that actually add/change the UI will of course need some usability review according to their module stability (minimal validation in the case of Place Block).

I think @alexpott's question is addressed by the comments above; I agree with @pwolanin (and the retitle) that this functionality is sufficient for now. If we need more later, we can add it then.

This is a BC addition to internal form functionality, so beta-safe. I checked with @tim.plunkett as a subsystem maintainer for both Block and Form API and he signed off on the change. My one question was whether we should document this request option, but according to @tim.plunkett we don't do that elsewhere either.

Committed ea28d66 and pushed to 8.3.x and 8.2.x. Thanks!