Theme name is included in block machine name but should be stored as a key instead

Working on this.

Preliminary patch. No reviews please, just testing with the bot.

Missed a couple form keys.

To recap:

When blocks were vanilla CMI and not yet ConfigEntity, we used config_get_storage_names_with_prefix() to load the blocks of a given theme.
Once we switched to ConfigEntity, we just used entity_load_multiple_by_properties('block', array('theme' => $theme));

However, we didn't actually store the theme in the yaml as its own key, and while we added a hack to allow $block->get('theme') to work, $block->id() always returned the theme prepended to the machine name.

Most of this is undoing the explode() calls, and adjusting to $form['machine_name'] becoming $form['id'].

Because we want to have blocks placed more than once per site (aka once per theme, like the main content block), I've kept the theme prefix for all shipped blocks, but with an underscore instead of a dot.

Site builders will need to use their best judgement when naming blocks. There is plenty of work being done to offload that burden anyway.

I don't have it in me to debug Drupal\system\Tests\Menu\BreadcrumbTest this week. Unassigning so someone else can try fixing it...

Will work on this soon, possibly this weekend.

Let's see where this gets us.

Fixed a couple stragglers

A little overzealous there, we still need the tools block for Seven.

Yay, green! Now it just needs a review.

OK I've looked over the code and everything from your patch seems fine. We do however have a bit of inconsistency throughout with the use of strtolower(), drupal_strtolower() and Unicode::strtolower(), might be worth a follow up to clean them up?

Off Topic
I noticed that if you enable another theme, eg stark, then go back to bartik and add a block into a region stark doesn't have and then re-enable stark the block doesn't land in the default region like it does the first time you enable the theme. Bit of an edge case but it should probably be consistent right?

No, that's how it works in D7. Themes can only actually be installed once, "disabling" them is a lie. See #1067408: Themes do not have an installation status

Well it's RTBC for me.

Needs a reroll

git ac https://drupal.org/files/block-2043527-14.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 59696  100 59696    0     0  30791      0  0:00:01  0:00:01 --:--:-- 34706
error: core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php: does not exist in index
error: patch failed: core/modules/block/lib/Drupal/block/Tests/BlockTest.php:133
error: core/modules/block/lib/Drupal/block/Tests/BlockTest.php: patch does not apply

Blergh

Conflicted on Plugin/Core/Entity and some added test coverage in the same hunk.
No actual changes.

Duh, I should have fixed the newly added hunk that broke the patch...

Back to RTBC.

Patch no longer applies

Rerolled.

git applyc https://drupal.org/files/block-2043527-26.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 62648  100 62648    0     0  26557      0  0:00:02  0:00:02 --:--:-- 28764
error: patch failed: core/modules/block/lib/Drupal/block/BlockFormController.php:72
error: core/modules/block/lib/Drupal/block/BlockFormController.php: patch does not apply

A newly added comment in the patch context. No change.

Patch no longer applies.

It was just s/plugin.manager.entity/entity.manager in patch context, no changes.

Approving the API change of switching to ID from machine name since a placed block is a config entity and unique loading by it's ID makes perfect sense. This patch is getting rid of code. Yay!

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
@@ -87,20 +87,17 @@ public function blockSubmit($form, &$form_state) {
+   public function generateBlockInstanceID(Query $entity_query) {

I can't for the life of me see how this code is actually called. Chatted this through with @dawehner on IRC #2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController and he agrees this is unused. So we have two options here... remove the changes since they're unused and untested or just remove the method as we are cleaning up block ID code here.

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTestBase.php
   @@ -67,45 +67,11 @@ function setUp() {
   -    $default_theme = variable_get('theme_default', 'stark');
   ...
   +    $blocks = $block_storage->loadByProperties(array('theme' => variable_get('theme_default', 'stark')));
   

   Oh, the default theme is actually stored in system_theme.default

2. +++ b/core/modules/block/lib/Drupal/block/Tests/Views/DisplayBlockTest.php
   index e8b5ade..4b68aca 100644
   --- a/core/modules/block/tests/modules/block_test/config/block.block.stark.test_block.yml
   
   --- a/core/modules/block/tests/modules/block_test/config/block.block.stark.test_block.yml
   +++ b/core/modules/block/tests/modules/block_test/config/block.block.test_block.yml
   
   +++ b/core/modules/block/tests/modules/block_test/config/block.block.test_block.yml
   +++ b/core/modules/block/tests/modules/block_test/config/block.block.test_block.yml
   @@ -1,4 +1,5 @@
   
   @@ -1,4 +1,5 @@
   -id: stark.test_block
   +id: test_block
   +theme: stark
   

   That rename does not fit with the other renames of the files, but sure, the IDs are flexible now.

Patch no longer applies.

Re-roll of #32.

Just postponed #2100203: Make config entities use dots in machine names consistently on this

Replacing few drupalPost() with drupalPostForm()

Working on fixes.

Once this is green, let's get it in. Rerolling for added tests is a pain.

Conflicted with #1879396: Add inline documentation to BlockBase::validateConfigurationForm() and BlockBase::submitConfigurationForm(), it documented some of the weirdness we're removing :)

Looks ok.
I note the comment in WebTestBase::drupalPlaceBlock

   * Note: Until this can be done programmatically, the active user account
   * must have permission to administer blocks.

Is that no longer relevant? If so we need a follow-up to remove it or if we have to re-roll again, it can be removed here while we're at it.

screenshot from manual testing

This patch didn't change the fact the current code in drupalPlaceBlock() has made this comment obselete - so out of scope :)

Committed 9fdc6a9 and pushed to 8.x. Thanks!

Since I have tests failing in config_translation due to this change, I'm trying to understand what happened here... The config file names still use the 'theme.machine_name' construct, while the id of the block is 'theme_machine_name', right? This seems to be the first case, where the id of the config entity would be different from the part of the config file name after the config prefix. Confusing for me :/ Is this intended even?!

Note that this is not a theoretical question only. This patch actually broke the translatability support of block placements because the id is not anymore equal to machine name which is not anymore equal to the identifier used in the URL and the file name.

The renames got lost since #32.

I manually checked the file renames against the patch from #41 and they match.

Committed f36755f and pushed to 8.x. Thanks!

Doh! I should have spotted this.

Yay, superb, thanks! The config_translation tests caught this. Yet another reason to get the module in core :D

Ok, well, not good enough yet. The config schema was still not updated. Now the block config files don't have block.block.*.*.yml anymore, but block.block.*.yml. Also the new theme key was not added to the schema.

Yet another reason to add the config translation module to core? :) (Which caught this again through the block translatability test).

is there an issue to add that? (would like to follow)

@larowlan: #1952394: Add configuration translation user interface module in core - already found dozens of bugs in core via that in date formats, comment local tasks, even hook_help :)

#51 looks good.

Committed 44639c1 and pushed to 8.x. Thanks!

This makes sense for a property like the block ID, but surely the region, weight and maybe other properties should be theme-specific.

Is it possible to have the same block assigned to different regions in different themes following this patch?

Yes it is possible to have the same block assigned to different regions in different themes

From the issue summary:

Remove $theme from the block machine name and store it as a key instead. @tim.plunkett and @xjm said in IRC that config entity query could be useful here

#2161591: Change default active config from file storage to DB storage is now reopening the two-year old "let's not store config in files" debate under the premise that "To determine which blocks to show on a page we [do an entity query which runs a] query on files on every page load. It's horrible, outdated and slow" :-p

Storing the theme as a property in the yaml was definitely a good thing, but removing it from the entity id & thus from the filename seems to bite us back...