An existing block instance will be overwritten by a newly created block instance if the two share the same machine_name

So what is the expected behavior in this scenario? I was thinking of writing tests for it, but I don't know what the result should be.

http://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.

Writing a test that will fail if a block with the same name already exists and it moves or disappears after the save. Assuming that the patch to fix the bug will prevent the save due to the duplicate block name.

Removed needs issue summary update tag

@dnmurray Please visit http://core.drupalofficehours.org/task/2362 (logged in) and indicate that you're working on this so those of us in the code sprint know this task isn't available.

Thanks!

Working on this hopefully will have test and fix up for review today, initial test will be appearing shortly.

Test only patch this should fail.

This fixes the test but smells very wrong.

A some what better version that allows for machine name to be given a prefix, which is the issue here as blocks are prefixed with theme name.

This sounds like a dupe of #1887654: Creating a config entity with an existing machine name shouldn't work, at least the underlying bug.

Oh, that's where that issue went. Still, though, I think this can go in as-is to fix the data loss issue. We can generalize it later with #1887654: Creating a config entity with an existing machine name shouldn't work and #2019709: Add an option to machine name elements to increment with a delta instead of failing validation, but this seems fine to me to at least prevent the blithe overwriting through the UI.

#12: block-1998664-12.patch queued for re-testing.

Patch in action:

+++ b/core/includes/form.incundefined
@@ -3871,7 +3871,14 @@ function form_validate_machine_name(&$element, &$form_state) {
+    // Certain configuration entities prefix the machine name so add these if

I don't know that form.inc should know/care about "configuration entities".

And the test coverage for this API addition to FAPI should have standalone tests, not coupled to the block UI.

Well, such tests won't work since this fix is only FAPI validation...

Ah, to clarify #17, I didn't notice that the validation is being done in form.inc. @tim.plunkett and I agreed that since the prefixing is a Block-only behavior that we probably want to remove anyway, let's just move the validation into the block form instead. (So the tests are correct, but the fix is too general.)

So the patch in https://drupal.org/node/1998664#comment-7483306 instead? Which is a block only version of the validation patch.

Field UI does this by adding a #field_prefix to the form element and then manually prepending that in the validate() step. Field UI also adds a < span dir="ltr" > because machine names are always English and, thus, should always be displayed in ltr regardless of the rest of the page. The fact that AFAIK only Field UI does this, however, is a hint IMO that #type machine_name should probably handle that automatically. So I would support adding an #id_prefix property (although it should probably be renamed, as the form type is machine_name) or re-using the #field_prefix for that, if that's possible (although probably not, because of the < span > thing). Field UI could be used as a sample implementation of that as well.

On the other hand, that does sort of seem like scope-screep, since this is a critical bug. Can't the Blocks UI simply add the prefix in validate() as well? Then we can figure out the broader issue in a follow-up.

Ahh, I'm sorry I didn't read the issue properly. The Blocks UI already adds the prefix correctly. This is about the #exists callback not getting the correct ID....
Hmm...

note that, while this will still be a problem, this becomes vastly less of a user WTF when #2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController goes in.

Isn't that an instance of #2041887: check that id does not exist before saving a new ConfigEntity ?

Related: #2041887: check that id does not exist before saving a new ConfigEntity

Back in June, when this was close, it was fine to keep this separate. At this point, it's out of date enough that we should just mark it a dupe of #1887654: Creating a config entity with an existing machine name shouldn't work

Updated the issue summary.