Updated: Comment #0

Problem/Motivation

There is code like this scattered throughout core at the moment:

<?php
$config_id
= explode('.', $block->id());
$machine_name = array_pop($config_id);
?>
<?php
list(, $machine_name) = explode('.', $default_theme_block_id);
?>

Whose purpose is to extract the 'powered' from a 'bartik.powered' block ID in order to get the usable "machine name".

Proposed resolution

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.

Remaining tasks

Patch needs to be written.

User interface changes

n/a

API changes

TBD

None yet

Files: 
CommentFileSizeAuthor
#51 block-schema-change.patch612 bytesGábor Hojtsy
PASSED: [[SimpleTest]]: [MySQL] 58,994 pass(es).
[ View ]
#47 blocks-2043527-47.patch4.73 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,315 pass(es).
[ View ]
#43 Screenshot 2013-10-16 09.41.04.png55.02 KBlarowlan
#41 block-2043527-41.patch59.32 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es).
[ View ]
#39 block-2043527-39.patch59.26 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-2043527-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 interdiff.txt4.97 KBtim.plunkett
#36 2043527-block-36.patch58.26 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 59,123 pass(es), 46 fail(s), and 0 exception(s).
[ View ]
#36 2043527-diff-34-36.txt2.33 KBvijaycs85
#34 2043527-block-34.patch58.25 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,997 pass(es), 25 fail(s), and 6 exception(s).
[ View ]
#32 block-2043527-31.patch61.04 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,842 pass(es).
[ View ]
#32 interdiff.txt3.29 KBdawehner
#30 block-2043527-30.patch61.03 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]
#28 block-2043527-28.patch61.04 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,522 pass(es).
[ View ]
#26 block-2043527-26.patch61.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]
#23 block-2043527-23.patch58.69 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,267 pass(es).
[ View ]
#23 interdiff.txt860 bytestim.plunkett
#21 block-2043527-21.patch58.29 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,285 pass(es), 6 fail(s), and 1 exception(s).
[ View ]
#14 block-2043527-14.patch58.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,889 pass(es).
[ View ]
#14 interdiff.txt957 bytestim.plunkett
#12 block-2043527-12.patch58.08 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#12 interdiff.txt7.64 KBtim.plunkett
#10 interdiff.txt12.36 KBtim.plunkett
#9 block-2043527-9.patch54.04 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,981 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#4 block-2043527-4.patch48.53 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,209 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#4 interdiff.txt5.64 KBtim.plunkett
#2 block-2043527-2.patch42.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

Comments

Cottser’s picture

Issue summary:View changes

Grammar/phrasing tweak

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Working on this.

tim.plunkett’s picture

Status:Active» Needs review
StatusFileSize
new42.89 KB
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 30 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, block-2043527-2.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new5.64 KB
new48.53 KB
FAILED: [[SimpleTest]]: [MySQL] 57,209 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs review» Needs work

The last submitted patch, block-2043527-4.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

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

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
tim.plunkett’s picture

Will work on this soon, possibly this weekend.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new54.04 KB
FAILED: [[SimpleTest]]: [MySQL] 57,981 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Let's see where this gets us.

tim.plunkett’s picture

StatusFileSize
new12.36 KB

Status:Needs review» Needs work

The last submitted patch, block-2043527-9.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new7.64 KB
new58.08 KB
FAILED: [[SimpleTest]]: [MySQL] 58,020 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Fixed a couple stragglers

Status:Needs review» Needs work

The last submitted patch, block-2043527-12.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new957 bytes
new58.3 KB
PASSED: [[SimpleTest]]: [MySQL] 57,889 pass(es).
[ View ]

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

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

Yay, green! Now it just needs a review.

benjy’s picture

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?

tim.plunkett’s picture

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

benjy’s picture

Status:Needs review» Reviewed & tested by the community

Well it's RTBC for me.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

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
tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Blergh

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new58.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,285 pass(es), 6 fail(s), and 1 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, block-2043527-21.patch, failed testing.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new860 bytes
new58.69 KB
PASSED: [[SimpleTest]]: [MySQL] 58,267 pass(es).
[ View ]

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

benjy’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new61.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,452 pass(es).
[ View ]

Rerolled.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll
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
tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new61.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,522 pass(es).
[ View ]

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

tim.plunkett’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new61.03 KB
PASSED: [[SimpleTest]]: [MySQL] 58,022 pass(es).
[ View ]

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

alexpott’s picture

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.

dawehner’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new3.29 KB
new61.04 KB
PASSED: [[SimpleTest]]: [MySQL] 58,842 pass(es).
[ View ]
  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.

benjy’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

vijaycs85’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new58.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,997 pass(es), 25 fail(s), and 6 exception(s).
[ View ]

Re-roll of #32.

alexpott’s picture

vijaycs85’s picture

StatusFileSize
new2.33 KB
new58.26 KB
FAILED: [[SimpleTest]]: [MySQL] 59,123 pass(es), 46 fail(s), and 0 exception(s).
[ View ]

Replacing few drupalPost() with drupalPostForm()

Status:Needs review» Needs work

The last submitted patch, 2043527-block-36.patch, failed testing.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

Working on fixes.

tim.plunkett’s picture

Priority:Normal» Major
Status:Needs work» Needs review
StatusFileSize
new4.97 KB
new59.26 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-2043527-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, block-2043527-39.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new59.32 KB
PASSED: [[SimpleTest]]: [MySQL] 58,819 pass(es).
[ View ]
larowlan’s picture

Status:Needs review» Reviewed & tested by the community

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.

larowlan’s picture

StatusFileSize
new55.02 KB

screenshot from manual testing
Screenshot 2013-10-16 09.41.04.png

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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!

Gábor Hojtsy’s picture

Status:Fixed» Needs work

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?!

Gábor Hojtsy’s picture

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.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new4.73 KB
PASSED: [[SimpleTest]]: [MySQL] 59,315 pass(es).
[ View ]

The renames got lost since #32.

Xano’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed f36755f and pushed to 8.x. Thanks!

Doh! I should have spotted this.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

Status:Fixed» Needs review
StatusFileSize
new612 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,994 pass(es).
[ View ]

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).

larowlan’s picture

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

Gábor Hojtsy’s picture

@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 :)

vijaycs85’s picture

Status:Needs review» Reviewed & tested by the community

#51 looks good.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

ianthomas_uk’s picture

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?

alexpott’s picture

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

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

Anonymous’s picture

Issue summary:View changes

Scattered throughOUT

yched’s picture

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...