#1998582: Auto-generate machine_name for Views Blocks using [viewname]_[displayname] rather than forcing manual entry solved a problem for views that we should really be solving more generically: providing an avenue for the block plugin to make an initial suggestion about what the machine name for an instance of that block plugin should be. i'm happy we got it in, i just think we didn't go far enough. and that we also got some responsibilities slightly mixed up.

this patch fixes both the responsibility mixup and makes it go whole hog: it adds a getMachineNameBase() method to BlockPluginInterface, implements it on BlockBase and ViewsBlock, removes the one-off method from ViewsBlock, guts most of the icky sticky views_form_block_form_alter() implementation, gets BlockFormController to utilize the new getMachineNameBase() method, then keep appending shit to it until it comes back unique in the exact same way the one-off ViewsBlock method used to do it.

it also removes the corresponding views test entirely. this is temporary. since the id-getting method is no longer is solely on ViewsBlock, but now incorporated onto BlockPluginInterface, then the corresponding test should also no longer be a Views test. i haven't written that test yet - at the moment, i just wanted to get this up to see what else breaks.

this is good because it means that block plugins are, once again, divorced from information about their storage. this makes princess happy because there, not all blocks are standalone, which means their uniqueness cannot be accurately determined by the block storage manager. the responsibility for determining uniqueness is instead left in the hands of the form controller, where it belongs.

API Change

The BlockFormController implements a constructor to inject the module handler,entity manager, and query factory services. These are injected using the static createInstance() method from the container during instantiation by the Entity manager.

Files: 
CommentFileSizeAuthor
#60 vdc-2022897-60.patch21.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]
#60 interdiff.txt989 bytestim.plunkett
#58 vdc-2022897-58.patch21.15 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,822 pass(es).
[ View ]
#58 interdiff.txt621 bytesdawehner
#56 block-2022897-56.patch21.12 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 57,183 pass(es).
[ View ]
#56 interdiff.txt691 bytesdawehner
#52 block-2022897-52.patch21.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,548 pass(es), 2 fail(s), and 23 exception(s).
[ View ]
#52 interdiff.txt5.97 KBdawehner
#45 block-2022897-45.patch24.53 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-2022897-45_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#45 interdiff.txt2.73 KBdawehner
#43 block-2022897-43.patch24.13 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 49 fail(s), and 21 exception(s).
[ View ]
#43 interdiff.txt691 bytesdawehner
#41 vdc-2022897-41.patch26.91 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,444 pass(es), 51 fail(s), and 44 exception(s).
[ View ]
#36 block-2022897-36.patch24.33 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]
#36 interdiff.txt2.41 KBdawehner
#33 block_block-2022897-33.patch24.18 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#33 interdiff.txt9.17 KBdawehner
#28 interdiff.txt1.36 KBsdboyer
#28 block-id-responsibility-2022897-28.patch21.58 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,175 pass(es).
[ View ]
#26 block-2022897-26.patch21.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,235 pass(es), 2 fail(s), and 23 exception(s).
[ View ]
#26 interdiff.txt2.45 KBdawehner
#23 drupal-2022897-23.patch20.86 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,038 pass(es), 2 fail(s), and 23 exception(s).
[ View ]
#19 interdiff.txt8.56 KBdawehner
#19 block-2022897-19.patch20.51 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,100 pass(es), 3 fail(s), and 23 exception(s).
[ View ]
#16 block-id-responsibility-2022897-16.patch11.34 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,125 pass(es).
[ View ]
#16 interdiff.txt1.96 KBsdboyer
#14 block-id-responsibility-2022897-14.patch10.71 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,475 pass(es).
[ View ]
#14 interdiff.txt945 bytessdboyer
#12 block-id-responsibility-2022897-12.patch10.4 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,128 pass(es).
[ View ]
#12 interdiff.txt1.25 KBsdboyer
#9 block-id-responsibility-2022897-9.patch10.41 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]
#9 interdiff.txt6.59 KBsdboyer
#7 interdiff.txt6.59 KBsdboyer
#7 block-id-responsibility-2022897-6.patch10.36 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 58,313 pass(es).
[ View ]
#1 block-id-responsibility-2022897-1.patch8.32 KBsdboyer
PASSED: [[SimpleTest]]: [MySQL] 57,761 pass(es).
[ View ]

Comments

sdboyer’s picture

Version:9.x-dev» 8.x-dev
Status:Active» Needs review
StatusFileSize
new8.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,761 pass(es).
[ View ]

here's the patch.

and also, correct version.

sdboyer’s picture

Title:Shift responsibility for creating unique id off of block plugins and onto BlockFormController» Shift responsibility for creating unique config id off of (views) block plugins and onto BlockFormController

slightly better title.

sdboyer’s picture

Issue summary:View changes

better justify removal of test coverage

jibran’s picture

Issue tags:+VDC

Tagging.

damiankloip’s picture

Yep, moving this seems like a great idea. The original patch is more of a POC than anything :) Moving the logic to the actual form controller makes alot of sense. There is no reason why it can't be reused like that.

It would be nice to not have to have the form_alter at all. Really a block plugin should be able to alter it's form, but that's a different issue entirely.

The patch is looking pretty good already, as alot of the logic was ironed out before. I will wait for tests before going for a proper review.

sdboyer’s picture

woot, green! i was worried that a buncha stuff would poke out the edges.

It would be nice to not have to have the form_alter at all.

agreed, but i couldn't quite get rid of it because those other two bits of logic there are still kinda crucial.

Really a block plugin should be able to alter it's form, but that's a different issue entirely.

ugh. i'm torn.

i certainly see the utility for this case. but allowing block plugins themselves to have a bite at altering the form is at odds with the idea that there could be different form usages for different use cases. now, we're already not particularly good about this since we present settings to the block plugin which are a product of form elements that it does not itself set, which creates an implicit tight binding between the block plugin and a particular wrapping implementation (exactly what i was trying to minimize in this patch). but letting the blocks plugins themselves reach out and alter the form? that'd be a step even worse. so, as much as i dislike that alter, i prefer it to an alternative where the block plugin itself does that work, since that makes block plugins exceed their scope, and it is logically more the domain of a module's alter to be reconciling its own specific objects with a particular UI implementation.

anyway, so, now we just need to write some replacement tests.

sdboyer’s picture

added tests (and partially reinstated the Views test i removed), and changed method name from getMachineNameBase() to getMachineNameSuggestion().

sdboyer’s picture

StatusFileSize
new10.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,313 pass(es).
[ View ]
new6.59 KB

now, with 20% more actual patch.

damiankloip’s picture

Status:Needs review» Needs work

Moving this stuff further upstream is looking really good!

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -27,12 +27,29 @@ public function form(array $form, array &$form_state) {
+    // If creating a new block, calculate a safe default machine name.
+    if ($entity->isNew()) {
+      $machine_base = $entity->getPlugin()->getMachineNameSuggestion();
+      $block_ids = array_map(function ($block_id) {
+        $parts = explode('.', $block_id);
+        return end($parts);
+      }, array_keys(\Drupal::entityManager()->getStorageController('block')->load()));
+
+      // Iterate through potential IDs until we get a new one. E.g.
+      // 'views_block__MYVIEW_PAGE_1_2'
+      $count = 1;
+      $machine_default = $machine_base;
+      while (in_array($machine_default, $block_ids)) {
+        $machine_default = $machine_base . '_' . ++$count;
+      }
+    }

I think maybe this should be abstracted out also? I could see this wanting to be custom in some cases. Maybe just a generateMachineName() method or something.

+++ b/core/modules/block/lib/Drupal/block/BlockPluginInterface.phpundefined
@@ -102,4 +102,15 @@ public function submit($form, &$form_state);
+  public function getMachineNameSuggestion();

I guess suggestion is what it is :) I'm not crazy about the name, but I can't think of anything better...

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.phpundefined
@@ -96,5 +96,9 @@ public function testBlockInterface() {
+    // Ensure the machine name suggestion is correct.
+    // In truth, this is actually testing BlockBase.

Wraps a bit early (sorry).

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.phpundefined
@@ -113,4 +113,18 @@ function testBlockSearch() {
+   * Test that the BlockFormController populates machine name correctly.

Tests

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.phpundefined
@@ -113,4 +113,18 @@ function testBlockSearch() {
+    $this->assertFieldByName('machine_name', 'displaymessage_2', 'Block form appends _2 to plugin-suggested machine name when an instance already exists.');

This tested more than just once before I think? Generating the first suffixed machine name after the base suggestion, is different to the ones after that imo. Basically the test should create another one and check it has '_3' suffix like before.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -114,33 +114,10 @@ protected function addContextualLinks(&$output, $block_type = 'block') {
+  public function getMachineNameSuggestion() {

Needs {@inheritdoc}

I see what you mean about the form altering stuff, it's a tricky one...

sdboyer’s picture

Status:Needs work» Needs review
StatusFileSize
new6.59 KB
new10.41 KB
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]

I think maybe this should be abstracted out also? I could see this wanting to be custom in some cases. Maybe just a generateMachineName() method or something.

i wouldn't have an issue with shifting it into a method, but anything that's in the BlockFormController is kinda inherently difficult to make custom "in some cases." well...if you mean "in the case of certain blocks within a site" as opposed to "a sitewide use case", wherein one could just swap out the BlockFormController wholesale. so, please splain a bit more how you see that being used before i do it.

This tested more than just once before I think? Generating the first suffixed machine name after the base suggestion, is different to the ones after that imo. Basically the test should create another one and check it has '_3' suffix like before.

yeah, i saw that that. IMO, that test is not meaningful; there is no more reason to check that it works three times than there is to check that it works four, ten, or a hundred times.

Needs {@inheritdoc}

it's actually there, the diff just formatted weirdly and stuck it up in the middle of the removal of the old method.

there's also a behavior change here - one that i think is a Good Thing, but i know there's been a lot of ugh-ing around this bug before, so i don't know if folks will be happy with the solution. when there's a default set on the machine name field, the js no longer automagically makes the machine name field chase the label field. this means that the block plugin, in tandem with the BlockFormController, is now much more significantly in charge of the machine_name, as the user has to opt-in to manually editing the machine name in order to change it. and because this code now guarantees the machine name for ALL blocks to be unique right from when one first visits the add form, that form, in its initial state, will no longer cause another block instance of that type to be overridden and just magically disappear.

now, if the user manually edits the machine name and sets it to a value that already exists, we still get the overwrite behavior. i have absolutely NFI why the hell a machine name field doesn't have validation that prevents that from happening - i suspect it's because the machine name's element-level validation is run on the un-theme-prefixed value before we do the crazy form_set_value() magic in BlockFormController::validate().

i'm hoping that we may have just not fixed that validation issue in the past because doing so would have created a situation where the user would be *forced* to edit the machine name of a block that was being added again. this patch makes that no longer a concern.

there's also a bit of a hidden gotcha here: for things OTHER than views, this initial suggestion is basically meaningless, as the js processor on the machine_name field almost immediately overwrites the default we provide on the backend.

sdboyer’s picture

here, i made a screencast that should be much more intelligible than that long block of text:

https://dl.dropboxusercontent.com/u/8238539/machine_name_flow.swf

damiankloip’s picture

Not sure I agree, the first block that is saved will not do anything in the while loop, the second one will be our first suffixed name, the third will test that the name has incremented correctly. No?

The new views unit test could use phpunit, but I'm not sure its worth the bother :-)

The patch is looking good though.

sdboyer’s picture

StatusFileSize
new1.25 KB
new10.4 KB
PASSED: [[SimpleTest]]: [MySQL] 58,128 pass(es).
[ View ]

Not sure I agree, the first block that is saved will not do anything in the while loop, the second one will be our first suffixed name, the third will test that the name has incremented correctly. No?

oh...if that's how it works, then yes, adding a third would be worthwhile. but i don't think that's how it works. first block that's saved skips the while loop, second block hits the while loop and tries appending a _2. if that still isn't unique, third pass appends a _3. the original views test was a little misleading because it had a _1 at the end of it as part of the display id itself, and it kinda made things seem like there were...i dunno, other things happening. but no, second time increments same as third time. i've updated the inline comments slightly to make it less confusing.

The new views unit test could use phpunit, but I'm not sure its worth the bother :-)

it definitely should be - we should inject a mocked view object into the block plugin and have it return off of that. i tried it, but i couldn't fucking get the system to pick up the test. i think i had it in the wrong namespace.

damiankloip’s picture

I wasn't getting confused by the 1 in the display id. All of our views displays have a number appended, and I did the logic and tests in the other issue. For me, the creation of the third is where the logic gets properly tested. As it will iterate over the while loop more than once.

I can have a stab at the unit test if you like?

sdboyer’s picture

Title:Shift responsibility for creating unique config id off of (views) block plugins and onto BlockFormController» Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController
StatusFileSize
new945 bytes
new10.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,475 pass(es).
[ View ]

hmm, ok. i can see that - the second one appends, the third one increments. it should really just be a copy/paste of the preceding lines...done that in this version of the patch.

i'd be THRILLED if you took a swing at the unit test.

damiankloip’s picture

Leave it with me.

sdboyer’s picture

StatusFileSize
new1.96 KB
new11.34 KB
PASSED: [[SimpleTest]]: [MySQL] 58,125 pass(es).
[ View ]

i just realized that the form alter implemented by views is now not doing shit - it does nothing if there's a default value set on the machine name field, which is exactly what we've started doing with this patch.

now, i don't necessarily see this as a problem. what'd be nice, though, is if the machine name field was still in its "collapsed" state - e.g., what's suggested over in #2019697-4: Allow user to customize views block instance machine name when the form comes up. it isn't for views blocks because there is no label field, and form_process_machine_name() bails out before doing its magic if the declared source element isn't present in the form. we could, and maybe should, push to a followup the refactoring of that function in order to support this pattern.

meanwhile, though, we gotta either get rid of the alter or make it disable the machine name field again. from what i've read, there's no reason to have it there if we reliably have a default, so i've just pulled it out in this latest patch.

i also took a stab at making the description text on the field slightly better. i'm sure i've violated some standards on wording...but it was bugging me. gotta try :)

damiankloip’s picture

Getting rid of the form alter would be great! I will have to actually test the patch to see what you mean I think - usually the best way.

This is actually going to be really hard to unit test. Mainly for two reasons; The use of the global constants in BlockBase (We should really try to fix this somehow), and the fact that the ViewsBlock constructor makes a call to views_get_view(). Maybe you should reinstate the original DUTB test that you had?

sdboyer’s picture

there's actually nothing additional to be done to the patch in #16 re: testing - because the functionality was genericized, what was in ViewsBlockTest is now in BlockUITest.

the only question is whether or not it's OK to expose Views' machine names, per my question in #16. if we're gonna refactor the machine name field to be more hidden-ish, that should be a follow-up.

dawehner’s picture

Issue tags:+phpunit
StatusFileSize
new20.51 KB
FAILED: [[SimpleTest]]: [MySQL] 58,100 pass(es), 3 fail(s), and 23 exception(s).
[ View ]
new8.56 KB

Provided some unit tests for the new code and some other nitpicks.

Status:Needs review» Needs work
Issue tags:-phpunit, -VDC, -happy princess API

The last submitted patch, block-2022897-19.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

#19: block-2022897-19.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+phpunit, +VDC, +happy princess API

The last submitted patch, block-2022897-19.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new20.86 KB
FAILED: [[SimpleTest]]: [MySQL] 58,038 pass(es), 2 fail(s), and 23 exception(s).
[ View ]

Forgot to move the config directory of block_test.

Status:Needs review» Needs work

The last submitted patch, drupal-2022897-23.patch, failed testing.

ParisLiakos’s picture

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.phpundefined
@@ -0,0 +1,97 @@
+  protected function getBlockMockWithMachineName($machine_name) {

awesome!
maybe get that in UnitTestCase? or maybe a block base..looks like a nice reusable helper

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.45 KB
new21.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,235 pass(es), 2 fail(s), and 23 exception(s).
[ View ]

maybe get that in UnitTestCase? or maybe a block base..looks like a nice reusable helper

Well, why not.

No clue why the tests fail.

Status:Needs review» Needs work

The last submitted patch, block-2022897-26.patch, failed testing.

sdboyer’s picture

StatusFileSize
new21.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,175 pass(es).
[ View ]
new1.36 KB

god dammit, three wasted hours. i don't know why i didn't check the obvious thing first - the 'block_test' module designated a location of the 'block_test_theme' relative to itself, so the theme just needed to move accordingly. UGH.

oh well, should go green.

sdboyer’s picture

Status:Needs work» Needs review
dawehner’s picture

Status:Needs review» Reviewed & tested by the community

I know that alex will hate that we moving is done in that issue but it was needed to get phpunit tests running.

xjm’s picture

Status:Reviewed & tested by the community» Needs review

Awesome, thanks @sdboyer and da*. I finally had a chance to read through this, and there's a couple points that I think need more feedback.

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -224,4 +224,13 @@ public function submit($form, &$form_state) {
    +    return preg_replace('/[^a-z0-9_.]+/', '', strtolower($definition['admin_label']));

    Don't we have transliteration that we could use for this instead? :) Machine name form elements already support transliteration by default.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
    @@ -7,13 +7,40 @@
    -class BlockFormController extends EntityFormController {
    +class BlockFormController extends EntityFormController implements EntityControllerInterface {

    So, I talked to @berdir about why this change was necessary. We need the entity manager to get the storage controller in order to check existing machine names.

  3. +++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
    @@ -234,4 +266,30 @@ public function submit(array $form, array &$form_state) {
    +    $block_ids = array_map(function ($block_id) {
    +      $parts = explode('.', $block_id);
    +      return end($parts);
    +    }, array_keys($this->storageController->load()));

    So, per @Berdir, this loads every existing block instance into memory. Is there a better way we can do this? Berdir pointed out that we could probably use the config listAll() method (which means we'd need the config storage instead).

    Also an inline comment here would be ducky.

  4. +++ b/core/modules/block/lib/Drupal/block/BlockPluginInterface.phpundefined
    @@ -102,4 +102,15 @@ public function submit($form, &$form_state);
    +   * @return string

    The @return needs a description.

  5. +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.phpundefined
    @@ -0,0 +1,54 @@
    +// @todo Remove once the constants are replaced with constants on classes.
    +if (!defined('BLOCK_LABEL_VISIBLE')) {
    +  define('BLOCK_LABEL_VISIBLE', 'visible');
    +}

    Oh, ah. Is there an existing issue to move this constant? If not, let's file one.

  6. +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.phpundefined
    @@ -0,0 +1,71 @@
    + * Contains \Drupal\Tests\block\BlockFormControllerTeste.

    Let's remove that trailing e.

    +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.phpundefined
    @@ -0,0 +1,71 @@
    +// @todo Once this once the constant got moved.
    +if (!defined('BLOCK_REGION_NONE')) {
    +  define('BLOCK_REGION_NONE', -1);
    +}

    What.

  7. +++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.phpundefined
    @@ -0,0 +1,71 @@
    +   * Tests the unique machine name generator.
    +   *
    +   *
    +   * @see \Drupal\block\BlockFormController::getUniqueMachineName()

    Extra blank line here. :)

  8. +++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/ViewsBlockTest.phpundefined
    @@ -49,38 +49,18 @@ protected function setUp() {
    +   * @see \Drupal\views\Plugin\Block::getmachineNameSuggestion().

    I think there's a typo here for getMachineNameSuggestion().

xjm’s picture

Issue tags:+Needs followup
dawehner’s picture

StatusFileSize
new9.17 KB
new24.18 KB
FAILED: [[SimpleTest]]: [MySQL] 58,025 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Don't we have transliteration that we could use for this instead? :) Machine name form elements already support transliteration by default.

I think also the machine name uses regular expressions and javascript but yes, let's try to use the transliteration service (even people could say that this is out of scope, as this just moves code).

So, per @Berdir, this loads every existing block instance into memory. Is there a better way we can do this? Berdir pointed out that we could probably use the config listAll() method (which means we'd need the config storage instead).

Berdir came up with a wonderful idea: use the entity query. Yes this still loads all of them internally, but you can at least swap it out and improve later.

Oh, ah. Is there an existing issue to move this constant? If not, let's file one.

Here is one: #2029675: Convert BLOCK_LABEL_VISIBLE to a constant on the block interface.

#2029677: Convert BLOCK_LABEL_VISIBLE to a constant on the block interface for the other constant.

Once once once.
Once more: #2029709: Convert FIELD_LOAD_CURRENT to a constant on some class/interface

Additional there unit tests had to be adapted based on the changes.

Status:Needs review» Needs work

The last submitted patch, block_block-2022897-33.patch, failed testing.

amateescu’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -224,4 +226,29 @@ public function submit($form, &$form_state) {
+    // @todo This is basically the same as done in
+    // \Drupal\system\MachineNameController::transliterate(), so it might make
+    //sense to provide a common service for the two.

Missing a space for the last line. And I agree that we could use a helper somewhere in Core for this, but that's not bound to api freeze so we're good with a @todo :)

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -27,12 +68,17 @@ public function form(array $form, array &$form_state) {
+      '#default_value' => $entity->id() ?: $machine_default,

We might want to switch this condition to use isNew() as above.

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.php
@@ -234,4 +280,31 @@ public function submit(array $form, array &$form_state) {
+   * Generate a unique machine name for a block.

Generates..

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.41 KB
new24.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,000 pass(es).
[ View ]

This should fix all of the remaining errors.

xjm’s picture

Issue tags:-Needs followup
ParisLiakos’s picture

Status:Needs review» Reviewed & tested by the community
YesCT’s picture

Issue tags:+RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockBaseTest.phpundefined
@@ -0,0 +1,65 @@
+// @todo Remove once the constants are replaced with constants on classes.
+if (!defined('BLOCK_LABEL_VISIBLE')) {
+  define('BLOCK_LABEL_VISIBLE', 'visible');
+}

This is now a constant on the BlockInterface

+++ b/core/modules/block/tests/lib/Drupal/block/Tests/BlockFormControllerTest.phpundefined
@@ -0,0 +1,92 @@
+    $this->assertEquals('test_2', $block_form_controller->getUniqueMachineName($blocks['test']));
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test']));
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_1']));
+    $this->assertEquals('other_test_3', $block_form_controller->getUniqueMachineName($blocks['other_test_2']));
+
+    $last_block = $this->getBlockMockWithMachineName('last_test');
+    $this->assertEquals('last_test', $block_form_controller->getUniqueMachineName($last_block));

I think we could do with some comments as to what exactly is being tested here... for example 'lest_test' is testing that non duplicate machine names are not being altered by getUniqueMachineName... that's not exactly apparent.

And this needs a reroll...

git ac https://drupal.org/files/block-2022897-36.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 24917  100 24917    0     0  14435      0  0:00:01  0:00:01 --:--:-- 18791
error: patch failed: core/modules/block/lib/Drupal/block/BlockBase.php:8
error: core/modules/block/lib/Drupal/block/BlockBase.php: patch does not apply
error: patch failed: core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php:112
error: core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php: patch does not apply
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new26.91 KB
FAILED: [[SimpleTest]]: [MySQL] 56,444 pass(es), 51 fail(s), and 44 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, vdc-2022897-41.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new691 bytes
new24.13 KB
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 49 fail(s), and 21 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, block-2022897-43.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.73 KB
new24.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-2022897-45_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The parent class changed.

Just a general question, until when things like a changed constructor is an api change?

xjm’s picture

@dawehner A constructor change is an API change for sure. Consult a maintainer in advance when you think it's necessary. :)

xjm’s picture

dawehner’s picture

People are doing it all over the place, so I don't think it is a good investment of the time of maintainers to think about it now.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

This was RTBC once or twice before, and the added documentation in the test is very clear.

jibran’s picture

#45: block-2022897-45.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+phpunit, +VDC, +happy princess API, +RTBC July 1

The last submitted patch, block-2022897-45.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new5.97 KB
new21.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,548 pass(es), 2 fail(s), and 23 exception(s).
[ View ]

Let's hope this still applies.

Many of the previous moved files got already moved.

jibran’s picture

dawehner’s picture

Ups, yeah you are totally right on this point.

Status:Needs review» Needs work

The last submitted patch, block-2022897-52.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new691 bytes
new21.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,183 pass(es).
[ View ]

That is potentially green now.

damiankloip’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -234,4 +286,36 @@ public function submit(array $form, array &$form_state) {
+    $query = $this->entityQueryFactory->get('block');
+    $query->condition('id', $suggestion, 'CONTAINS');

That's a nice addition, even though it does load all of them anyway :) In the future though...

What's the deal with the @returns and mock objects? We use the PHPUnit mock and not the object/interface we're mocking?

Otherwise, this looks great.

dawehner’s picture

StatusFileSize
new621 bytes
new21.15 KB
PASSED: [[SimpleTest]]: [MySQL] 57,822 pass(es).
[ View ]

From a technical point of view it is basically both.

xjm’s picture

People are doing it all over the place, so I don't think it is a good investment of the time of maintainers to think about it now.

Regardless, that's the core maintainers' preference:
https://drupal.org/node/2045345

This issue is an RTBC July 1, though.

tim.plunkett’s picture

StatusFileSize
new989 bytes
new21.22 KB
PASSED: [[SimpleTest]]: [MySQL] 57,913 pass(es).
[ View ]

Reroll, and a comment fix.

One problem with this though.

I enabled the Archive view, and went to place it.
As expected, the suggested machine name was views_block__archive_block_1.
After placing it and trying to place a second, the machine name suggestion was not views_block__archive_block_2 like I expected, but views_block__archive_block_1_2 instead. The third one was views_block__archive_block_1_3.

tim.plunkett’s picture

OHHH, #60 is because the display's machine name is block_1, so that's correct. Just confusing.
I think this is RTBC, any takers?

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Actually since I just indented the @todo and made no changes, RTBC.

xjm’s picture

Let's get the API change info in the summary. :)

tim.plunkett’s picture

Where is the API change here? We're removing a form_alter, that shouldn't count.

sdboyer’s picture

@tim.plunkett - change in the BlockStorageController constructor signature for additional service injection.

xjm’s picture

Yeah, it's really minor, but that's not a reason to stick our fingers in our ears, sing LALALALALALA and pretend it's not happening. ;)

xjm’s picture

Issue summary:View changes

Updated issue summary.

alexpott’s picture

Considering this API change is in the constructor and this is primarily called by EntityManager::getFormController() this is an approved change.

alexpott’s picture

Issue tags:+Approved API change

that'll teach me for not using autocomplete!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed 8c384bc and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary:View changes

Add api change