Problem Description:

Block plugins have a hard dependency on block entities, making it impossible to reuse the block plugin system for blocks whose configurations are not stored in independent entities (for example, for ones stored inline into Panels layouts). We need to fix that so that block plugins only depend on a passed in configuration array: whether that configuration is stored in an independent entity or not should not be a concern for block plugin implementations, hook_block_view_alter(), or theme('block').

Patch summary:

- Within the block CMI files, puts plugin-relevant configuration into a 'settings' key.
- Makes $block_entity->view() a very thin wrapper to $block_plugin->build(). Makes hook_block_view_alter() and theme('block') act only on the plugin, and not receive any entity information (except for a follow up needed for #1989568: Remove block config ID from being used as an HTML ID or template suggestion).
- Moves visibility conditions (e.g., pages, roles) into the entity-level access controller.
- Reorganizes the entity's form controller and plugin's form methods to each be responsible for their respective settings.

Followup Issues:

Original Report:

In the process of moving block configuration to be configuration entities, I feel we ended up tightly coupling too much of the block system with a supporting entity structure, and we should fix that before we get too much further into feature freeze. This is a first patch that is designed to point the way. Ultimately Block Entities will end up as typed data and we'll not abuse the export properties method to get our data, but for the time being this works very nicely. I'm sure many many tests will fail or fatal on this.

Eclipse

ToDO

update original blocks as plugins change notice... https://drupal.org/node/1880620
#1871696: Convert block instances to configuration entities to resolve architectural issues change notice (not sure if this is a todo for this issue)

Files: 
CommentFileSizeAuthor
#200 1927608-200.patch124.21 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 55,505 pass(es).
[ View ]
#200 1927608-interdiff.txt722 bytesEclipseGc
#199 1927608-199.patch124.2 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#199 1927608-interdiff.txt18.13 KBEclipseGc
#192 interdiff.txt1.61 KBeffulgentsia
#191 1927608-191.patch112.81 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,915 pass(es).
[ View ]
#191 interdiff.txt759 byteseffulgentsia
#188 1927608-188.patch112.03 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]
#186 1927608-186.patch112.06 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 55,987 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#185 1927608-185.patch112.21 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 55,709 pass(es).
[ View ]
#185 1927608-interdiff.txt2.18 KBEclipseGc
#181 1927608-181.patch112.31 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 55,646 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#181 1927608-interdiff.txt12.21 KBEclipseGc
#179 1927608-179.patch111.29 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 55,577 pass(es).
[ View ]
#179 interdiff.txt759 byteseffulgentsia
#178 1927608-178.patch111.27 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#178 interdiff.txt876 byteseffulgentsia
#176 1927608-176.patch111.26 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 55,753 pass(es), 85 fail(s), and 1,091 exception(s).
[ View ]
#176 interdiff.txt10.89 KBeffulgentsia
#174 1927608-174.patch106.72 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 55,781 pass(es), 83 fail(s), and 1,071 exception(s).
[ View ]
#174 interdiff.txt6.74 KBeffulgentsia
#173 1927608-173-do-not-test.patch107.41 KBeffulgentsia
#168 1927608-168.patch107.35 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#165 1927608-165.patch107.34 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 55,581 pass(es).
[ View ]
#165 1927608-interdiff.txt5.78 KBEclipseGc
#163 main-page-content.png21.79 KBtim.plunkett
#162 1927608-162.patch107.39 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es).
[ View ]
#162 1927608-interdiff.txt10.12 KBEclipseGc
#161 1927608-161.fail_.patch109.47 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 55,366 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#161 1927608-161.succeed.patch109.41 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 55,215 pass(es).
[ View ]
#161 1927608-interdiff.txt2.09 KBEclipseGc
#160 1927608-160.patch108.3 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 54,948 pass(es).
[ View ]
#160 1927608-interdiff.txt6.01 KBEclipseGc
#158 1927608-155.patch107.78 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 54,923 pass(es).
[ View ]
#158 1927608-interdiff.txt1.65 KBEclipseGc
#156 test.patch332 bytesEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 54,637 pass(es).
[ View ]
#155 1927608-155.patch108.92 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 54,547 pass(es), 155 fail(s), and 17 exception(s).
[ View ]
#155 1927608-interdiff.txt4.13 KBEclipseGc
#153 1927608-153.patch109.31 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 54,373 pass(es), 166 fail(s), and 17 exception(s).
[ View ]
#153 1927608-interdiff.txt3.63 KBEclipseGc
#153 1927608-interdiff2.txt32.71 KBEclipseGc
#149 1927608-149.patch110.74 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 54,332 pass(es), 165 fail(s), and 18 exception(s).
[ View ]
#149 1927608-interdiff.txt31.38 KBEclipseGc
#143 block-1927608-143.patch97.72 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 54,544 pass(es), 160 fail(s), and 20 exception(s).
[ View ]
#140 block-1927608-140.patch144.33 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-140.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#124 block-1927608-124.patch144.67 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 54,196 pass(es).
[ View ]
#124 interdiff.txt9.09 KBtim.plunkett
#115 1927608-113.patch143.82 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 54,155 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#115 1927608-interdiff.txt9.37 KBEclipseGc
#115 1927608-113-perm.patch143.93 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 54,181 pass(es).
[ View ]
#115 1927608-interdiff-perm.txt587 bytesEclipseGc
#109 block-1927608-109.patch142.6 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,858 pass(es).
[ View ]
#109 interdiff.txt6.41 KBtim.plunkett
#107 block-1927608-107.patch138.28 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,990 pass(es), 9 fail(s), and 5 exception(s).
[ View ]
#107 interdiff.txt6.46 KBtim.plunkett
#105 blocks-1927608-105.patch134.79 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#105 interdiff.txt26.08 KBtim.plunkett
#99 blocks-1927608-99.patch124.96 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,914 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#99 interdiff.txt19.78 KBtim.plunkett
#96 1927608-96.patch107.71 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 54,037 pass(es).
[ View ]
#96 1927608-interdiff.txt536 bytesEclipseGc
#92 1927608-92.patch107.66 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 53,985 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#92 1927608-interdiff.txt14.06 KBEclipseGc
#91 block-1927608-90.patch97.51 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,875 pass(es).
[ View ]
#88 block-1927608-88.patch98.1 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#88 interdiff.txt3.54 KBtim.plunkett
#86 interdiff.txt11.67 KBtim.plunkett
#86 block-1927608-86.patch96.4 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,846 pass(es), 2 fail(s), and 184 exception(s).
[ View ]
#86 block-1927608-86-whitespace-do-not-test.patch91.75 KBtim.plunkett
#82 blocks-1927608-82.patch90.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 53,863 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#75 block-1927608-75.patch8.78 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,684 pass(es).
[ View ]
#66 block-1927608-66.patch8.74 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#63 1927608-63.patch41.13 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 53,118 pass(es).
[ View ]
#63 1927608-interdiff.txt4.35 KBEclipseGc
#55 1927608-55.patch40.79 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#55 1927608-interdiff.txt1.17 KBEclipseGc
#52 1927608-52.patch40.74 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 53,060 pass(es).
[ View ]
#52 1927608-interdiff.txt3.27 KBEclipseGc
#50 1927608-50.patch40.13 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 53,062 pass(es).
[ View ]
#50 1927608-interdiff.txt816 bytesEclipseGc
#49 1927608-interdiff.txt23.56 KBEclipseGc
#47 1927608-47.patch20.89 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 53,158 pass(es).
[ View ]
#47 1927608-interdiff.txt782 bytesEclipseGc
#42 1927608-42.patch20.87 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 53,044 pass(es), 1 fail(s), and 12 exception(s).
[ View ]
#42 1927608-interdiff.txt921 bytesEclipseGc
#39 1927608-39.patch20.85 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 53,002 pass(es), 1 fail(s), and 189 exception(s).
[ View ]
#39 1927608-interdiff.txt623 bytesEclipseGc
#37 1927608-37.patch20.83 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 53,128 pass(es), 7 fail(s), and 445 exception(s).
[ View ]
#35 1927608-35.patch20.83 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 53,009 pass(es), 7 fail(s), and 441 exception(s).
[ View ]
#35 1927608-interdiff.txt643 bytesEclipseGc
#33 1927608-33.patch20.87 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,683 pass(es), 1 fail(s), and 189 exception(s).
[ View ]
#31 1927608-31.patch20.87 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]
#31 1927608-interdiff.txt559 bytesEclipseGc
#29 1927608-29.patch20.76 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,472 pass(es), 2 fail(s), and 191 exception(s).
[ View ]
#29 1927608-interdiff.txt612 bytesEclipseGc
#27 1927608-27.patch20.34 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,513 pass(es), 1 fail(s), and 195 exception(s).
[ View ]
#27 1927608-interdiff.txt1.81 KBEclipseGc
#25 1927608-25.patch19.45 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,400 pass(es), 32 fail(s), and 180 exception(s).
[ View ]
#25 1927608-interdiff.txt762 bytesEclipseGc
#22 1927608-22.patch19.45 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,404 pass(es), 32 fail(s), and 182 exception(s).
[ View ]
#22 1927608-interdiff.txt654 bytesEclipseGc
#20 1927608-20.patch19.2 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 51,047 pass(es), 921 fail(s), and 2,854 exception(s).
[ View ]
#20 1927608-interdiff.txt10.47 KBEclipseGc
#17 1927608-17.patch9.63 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,176 pass(es), 159 fail(s), and 2,031 exception(s).
[ View ]
#13 1927608-13.patch9.87 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 52,518 pass(es).
[ View ]
#13 1927608-interdiff.txt563 bytesEclipseGc
#11 1927608-11.patch9.81 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,195 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#9 1927608-9.patch12.43 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,271 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#7 1927608-7.patch9.11 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,194 pass(es), 22 fail(s), and 2 exception(s).
[ View ]
#4 1927608-4.patch9.13 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 52,203 pass(es), 26 fail(s), and 52 exception(s).
[ View ]
#1 1927608-1.patch7.82 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 51,990 pass(es), 197 fail(s), and 2,145 exception(s).
[ View ]

Comments

EclipseGc’s picture

StatusFileSize
new7.82 KB
FAILED: [[SimpleTest]]: [MySQL] 51,990 pass(es), 197 fail(s), and 2,145 exception(s).
[ View ]
tim.plunkett’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -53,7 +54,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
+        'subject' => check_plain($configuration['label']),

So the $entity->label() is no longer used as the label anywhere? That differs from every other entity, config or otherwise.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -123,8 +123,7 @@ public function getPlugin() {
+        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->getExportProperties());

getExportProperties is only used by CMI to find the properties to write to file, I don't know that it is appropriate to use here.

+++ b/core/modules/node/node.moduleundefined
@@ -2030,7 +2030,8 @@ function node_form_block_form_alter(&$form, &$form_state) {
+  $configuration = $block->getConfig();

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -486,7 +486,8 @@ function overlay_block_access($block) {
+    $configuration = $block->getConfig;

I *really* wanted to remove all calls to getConfig(). I don't know why we should ever need it.

+++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
@@ -77,7 +77,7 @@ public function form($form, &$form_state) {
-    $this->entity->set('label', filter_xss_admin($this->view->getTitle()));
+    $this->configuration['label'] = filter_xss_admin($this->view->getTitle());

If this works, I'll be happy.

Status:Needs review» Needs work

The last submitted patch, 1927608-1.patch, failed testing.

EclipseGc’s picture

StatusFileSize
new9.13 KB
FAILED: [[SimpleTest]]: [MySQL] 52,203 pass(es), 26 fail(s), and 52 exception(s).
[ View ]

OK, used the failing aggregator tests to eliminate some problems. Let's see how this goes.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review

testbot please!

Status:Needs review» Needs work

The last submitted patch, 1927608-4.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new9.11 KB
FAILED: [[SimpleTest]]: [MySQL] 52,194 pass(es), 22 fail(s), and 2 exception(s).
[ View ]

This fixes a few more things I think. I made some guesses at stuff I've previous fixed base on changes I made here.

Block::get() & Block::set() methods have been added or updated further to actually get/set values in the plugin configuration. They also update their own internal values as expected, so inspecting the entity should still be useful, but the actual values should be coming from the plugin configuration at this point.

Based upon a lot of screwing around with injecting the values into the createInstance() method in the getPlugin() method I think it's pretty clear now that the block system's constructor based injection is faulty and needs further tweaking. I've included a work around for the moment so the getPlugin() method is full of a bit of nasty. Having it working at all gives me a good baseline for fixing the constructor injection in general though.

In previous patches I was making block.module use the plugin configuration. Since block.module is actually providing the entity I _THINK_ it makes sense for block module to expect to call the entity's methods. So places where it was calling get() on the entity previous have been restored. This is an experiment. We'll see where it goes.

Internally within the plugins themselves there should be no references to the entity. This is also true for any hook that the plugin type is implementing. Hooks from the entity are obviously the entity's domain and I don't really care.

Tim and I have discussed a number of his points from #2. I will ultimately address them in this issue, but I'd like to get a proof of concept working before I go all manifesto on this issue.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-7.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new12.43 KB
FAILED: [[SimpleTest]]: [MySQL] 52,271 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

This should get everything except for the cache block tests working. I've not used state() before, so I need to spec-up.

Status:Needs review» Needs work

The last submitted patch, 1927608-9.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new9.81 KB
FAILED: [[SimpleTest]]: [MySQL] 52,195 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

The get() method on the entity wasn't working for 'settings' appropriately. I tweaked this further and I think I can delete most of the test changes I made. Let's see how test bot likes this. If all goes well, this should come back green.

Status:Needs review» Needs work

The last submitted patch, 1927608-11.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new563 bytes
new9.87 KB
PASSED: [[SimpleTest]]: [MySQL] 52,518 pass(es).
[ View ]

Found my error.

EclipseGc’s picture

Tim and I have discussed many of his comments in irc, I just want to formalize my responses here.

So the $entity->label() is no longer used as the label anywhere? That differs from every other entity, config or otherwise.

The entity label will be used when dealing with the actual entity. The block has to have the ability to stand on it's own though, unsupported by any entity. A bit of code has moved around in my newest patch, and at this point, block module is trying to use the entity anywhere that it makes sense we are in block.module's use case. To be clear, block.module's use case is not the only implementation of blocks, and should not be treated as such.

getExportProperties is only used by CMI to find the properties to write to file, I don't know that it is appropriate to use here.

Since the config object is not available to us from the entity, this is the closest thing we have. I've outlined a few times now what the use case was doing before the conversion and it was essentially:

<?php
$config
= config('blocks.block.theme_name.machine_name');
$plugin = $blockManager->createInstance($config->get('plugin_id'), $config->get());
?>

This injected the configuration object's values, not the configuration object. This is because we do not want to rely on some specific methodology to retrieve our values. The storage mechanism is completely detached from the plugin implementation, and this gives us flexibility moving forward to completely rewrite and restructure the block system as we desire w/o having to retool the actual plugins to match some new storage standard. In addition to this, the plugin factories only support an array of values being passed here (again to help enforce this disconnected storage philosophy). The vast majority of plugin types should be working this way. If they're not it's either a really special case, or a bug.

What's currently in core injects the block entity itself into the plugin, and that requires us to break the factory interface, and creates a host of other issues. the getExportProperties() method on the entity is what the storage controller uses to write the config file that is loaded, so this is as close an analog to $config->get() as anything that currently exists. The big downside to this for blocks specifically is that "settings" is a special case that consumes all the block specific settings. In raw CMI we could just write these to the top level of the file and reference them. We can't do that here, and most of this patch is actually special casing for that particular wrinkle. I'm actually pretty happy with that special case code, at this point since it's simultaneously managing the entity values and the plugin configuration, and really that should be the entity class's job. It should be making concessions to the plugin, not vice versa.

I *really* wanted to remove all calls to getConfig(). I don't know why we should ever need it.

Anything (hook, method, etc) that should be the plugin's domain should call getConfig() anything outside the plugin's domain can call getConfig() or a local replacement/substitution method as necessary. getConfig() is the plugin's methodology, and since configuration should be protected, anyone else is going to have to extend that, or in the case of the entity, keep it's own values up to date and matching the plugin's values.

If this works, I'll be happy.

It works :-)

Eclipse

EclipseGc’s picture

I also just want to mention that it may make sense to abstract some of the changes I've made here for Block Entities into a "PluginSupportEntity" abstract class that manages configuration for any plugin extending the PluginBase. The set/get/settings management we're doing here is pretty generic and if we push the settings thing as "the way" to do stuff, then I think this makes sense. It's also totally a followup.

EclipseGc’s picture

#13: 1927608-13.patch queued for re-testing.

EclipseGc’s picture

StatusFileSize
new9.63 KB
FAILED: [[SimpleTest]]: [MySQL] 52,176 pass(es), 159 fail(s), and 2,031 exception(s).
[ View ]

I don't think we need the save method since the set()/get() methods should be keeping both sets of values up to date at this point. Let's see if testbot agrees.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-17.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review

#13: 1927608-13.patch queued for re-testing.

Since 17 failed, I'm going to requeue 13. I'll also be adding a follow up set of tests that still lack the save method, so this next patch will be... wonky.

Eclipse

EclipseGc’s picture

StatusFileSize
new10.47 KB
new19.2 KB
FAILED: [[SimpleTest]]: [MySQL] 51,047 pass(es), 921 fail(s), and 2,854 exception(s).
[ View ]

I supplied the interdiff from 13. The only different between 13 and 17 is the Block::save() method. Apologies for missing it in the last patch.

I've moved the rendering responsibilities around here. The block is responsible for rendering itself now, I've not added the blockBuild() method to the interface yet. Once we get working code, I'll update the interfaces appropriately. Removed a bunch of stuff from the Render controller.

Since the ID is really a denizen of the entity, I left the block_view_$name_alter (confusing) on the render controller. and moved the block_view_alter and block_view_$id_alter (really that's the $plugin_id) to the plugin blockBuild() method.

I chose blockBuild() as the name here kind of mimicking FormInterface. Since I want blocks to implement FormInterface ultimately it should make sense to have a buildForm() and buildBlock() method as necessary.

The rest of the changes are mostly driven from minor issues in moving from passing the entity to the alters to passing the plugin to the alters.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-20.patch, failed testing.

EclipseGc’s picture

StatusFileSize
new654 bytes
new19.45 KB
FAILED: [[SimpleTest]]: [MySQL] 52,404 pass(es), 32 fail(s), and 182 exception(s).
[ View ]

Ok, was too ambitious in removing the save() method from the entity. Re-adding it. Let's see what the tests look like now.

EclipseGc’s picture

Status:Needs work» Needs review

blah

Status:Needs review» Needs work

The last submitted patch, 1927608-22.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new762 bytes
new19.45 KB
FAILED: [[SimpleTest]]: [MySQL] 52,400 pass(es), 32 fail(s), and 180 exception(s).
[ View ]

one small error, let's see where this goes.

Status:Needs review» Needs work

The last submitted patch, 1927608-25.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new1.81 KB
new20.34 KB
FAILED: [[SimpleTest]]: [MySQL] 52,513 pass(es), 1 fail(s), and 195 exception(s).
[ View ]

timplunkett helped me track down some weirdness I'd missed that blocks does for caching stuff. Let's see how this looks now.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-27.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new612 bytes
new20.76 KB
FAILED: [[SimpleTest]]: [MySQL] 52,472 pass(es), 2 fail(s), and 191 exception(s).
[ View ]

This should get the currently failing test passing and if it kills all the exceptions, then we'll try removing #block_config all together. I think this change makes that likely very simple.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-29.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new559 bytes
new20.87 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: failed to clear checkout directory.
[ View ]

This should fix the xss test. Will tackle the other tomorrow.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-31.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new20.87 KB
FAILED: [[SimpleTest]]: [MySQL] 52,683 pass(es), 1 fail(s), and 189 exception(s).
[ View ]

Same interdiff, but merged to head.

Status:Needs review» Needs work

The last submitted patch, 1927608-33.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new643 bytes
new20.83 KB
FAILED: [[SimpleTest]]: [MySQL] 53,009 pass(es), 7 fail(s), and 441 exception(s).
[ View ]

I think this will pass, which should give us a good baseline to start cleaning up other stuff.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-35.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new20.83 KB
FAILED: [[SimpleTest]]: [MySQL] 53,128 pass(es), 7 fail(s), and 445 exception(s).
[ View ]

This is passing for me locally, updated to head, not sure what's up, let's see how testbot feels about this one.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-37.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new623 bytes
new20.85 KB
FAILED: [[SimpleTest]]: [MySQL] 53,002 pass(es), 1 fail(s), and 189 exception(s).
[ View ]

took a lot of staring before I figured out what I'd done wrong. Let's see if testbot likes this better.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-39.patch, failed testing.

xjm’s picture

From IRC:

and while the argument comes off like architectural purity (I admit, that is totally what it sounds like when I listen to my own argument) it's also not difficult to achieve and would shut me the hell up.

:D

So, to that end, I'm trying to grok this patch. Here's a few spots where some docs would clarify things:

+++ b/core/modules/block/block.moduleundefined
@@ -327,7 +327,8 @@ function _block_get_renderable_region($list = array()) {
+        '#entity' => $block,
+        '#block' => (object) $block->getPlugin()->getConfig(),

+++ b/core/modules/node/node.moduleundefined
@@ -2027,7 +2027,8 @@ function node_form_block_form_alter(&$form, &$form_state) {
+  $configuration = $block->getConfig();
+  $visibility = $configuration['visibility'];

+++ b/core/modules/overlay/overlay.moduleundefined
@@ -486,7 +486,8 @@ function overlay_block_access($block) {
+    $configuration = $block->getConfig();
+    if (!in_array($configuration['region'], $regions_to_render)) {
       return FALSE;
     }

I've found the getConfig() method to seem a bit redundant and confusing, and we've discussed removing it in the past. This patch seems to add back a lot of uses of it. Maybe that's the point (because it's how you're decoupling the block from its storage, through use of that method), but what's the status of those other related issues about that method?

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,28 @@ public function submit($form, &$form_state) {
+      // Label needs special handling.
+      $build['#block']->label = check_plain($this->configuration['label']);

Why does it need special handling? Is that referring to the check_plain()?

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -73,15 +37,12 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      list(, $name) = explode('.', $entity->id());

Can we slap a comment on here that says "Extract the entity ID machine name from the configuration object ID" or whatever? The comment wasn't there beforehand, but we're rewriting the whole hunk here already.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -123,8 +123,16 @@ public function getPlugin() {
+        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin);

Let's use the new Drupal::service() pattern here since we can now. :)

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -138,6 +146,14 @@ public function getPlugin() {
+   * Overrides \Drupal\Core\Entity\EntityInterface::label().
+   */
+  public function label($langcode = NULL) {
+    $configuration = $this->getPlugin()->getConfig();
+    return $configuration['label'];
+  }

So it seems like, in general, we're switching things up so that the entity wraps the block API instead of vice versa. Conceptually, I guess that makes sense.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
+      // Support geting plugin specific settings with the get() method.

Nitpick: "plugin-specific settings" (with a hyphen).

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
-   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::getExportProperties();
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::set();
...
+  public function set($property_name, $value) {
+    if (!in_array($property_name, $this->exportProperties()) && $property_name != 'settings') {
+      $settings = $this->get('settings');
+      $settings[$property_name] = $value;
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set('settings', $settings);
+    }
+    elseif ($property_name == 'settings' && is_array($value)) {
+      $settings = $this->get('settings');
+      foreach ($value as $key => $key_value) {
+        $this->getPlugin()->setConfig($key, $key_value);
+        $settings[$key] = $key_value;
+      }
+      parent::set('settings', $settings);
+    }
+    else {
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set($property_name, $value);
+    }

This looks like a really interesting/important hunk of code. We're overriding the setter? That seems unusual, so let's describe why.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,42 @@ public function get($property_name, $langcode = NULL) {
+  protected function exportProperties() {
+    return array(

Docs, my brother.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,28 @@ public function submit($form, &$form_state) {
+  public function buildBlock() {
+    $build = array();

Docs please. Also, shouldn't it be blockBuild() instead of buildBlock(), or otherwise something tat is unambiguously different from blockBuild()?

EclipseGc’s picture

StatusFileSize
new921 bytes
new20.87 KB
FAILED: [[SimpleTest]]: [MySQL] 53,044 pass(es), 1 fail(s), and 12 exception(s).
[ View ]

I don't think this will fix the template suggestions error. That's a weird one cause it succeeds in the browser but fails at the command line, but this will hopefully fix the outstanding exceptions.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1927608-42.patch, failed testing.

xjm’s picture

@@ -44,12 +44,7 @@ function testBlockThemeHookSuggestions() {
     $variables = array();
-    $variables['elements']['#block'] = $block;
-    $variables['elements']['#block_config'] = $block->getPlugin()->getConfig() + array(
-      'id' => $block->get('plugin'),
-      'region' => $block->get('region'),
-      'module' => $block->get('module'),
-    );
+    $variables['elements'] = $block->getPlugin()->buildBlock();
     $variables['elements']['#children'] = '';
     // Test adding a class to the block content.

So, this change is causing the test to fail with Drupal style.

After this change, the block isn't built properly because the menu API is returning an empty menu to SystemMenuBlock::buildBlock(). Specifically, in menu_tree_page_data() on line 1204, menu_tree_get_path() is called with the path never having been set, so it overwrites the menu name with NULL in the following hunk:

<?php
// Check if the active trail has been overridden for this menu tree.
1204   $active_path = menu_tree_get_path($menu_name);
1205   // Load the menu item corresponding to the current page.
1206   if ($item = menu_get_item($active_path)) {
?>

That seems unstable to me, and the test passes when I make the following change:

diff --git a/core/includes/menu.inc b/core/includes/menu.inc
index 3b68bf0..7568c89 100644
--- a/core/includes/menu.inc
+++ b/core/includes/menu.inc
@@ -1201,7 +1201,8 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
   $language_interface = language(LANGUAGE_TYPE_INTERFACE);

   // Check if the active trail has been overridden for this menu tree.
-  $active_path = menu_tree_get_path($menu_name);
+  $active = menu_tree_get_path($menu_name);
+  $active_path = !empty($active) ? $active : $menu_name;
   // Load the menu item corresponding to the current page.
   if ($item = menu_get_item($active_path)) {
     if (isset($max_depth)) {

EclipseGc’s picture

Status:Needs work» Needs review

I've found the getConfig() method to seem a bit redundant and confusing, and we've discussed removing it in the past. This patch seems to add back a lot of uses of it. Maybe that's the point (because it's how you're decoupling the block from its storage, through use of that method), but what's the status of those other related issues about that method?

Yes that is indeed the point. The storage method should always adapt itself to the plugin type. As far as removing this method, that's never been on my todo list, there are in fact other issues for abstracting it out so that it can be more pervasive amongst plugins. The entire condition system works on this premise as well. This should be pretty central to most plugin types.

Why does it need special handling? Is that referring to the check_plain()?

Yes, and I think this could expose some other issues here, with metadata and xss and translation, but this seemed the most logical way to handle it w/o it exploding into a bigger issue for the time being.

So it seems like, in general, we're switching things up so that the entity wraps the block API instead of vice versa. Conceptually, I guess that makes sense.

Yes, that is the whole point, and probably a clearer way to say what I said in response to your first point.

This looks like a really interesting/important hunk of code. We're overriding the setter? That seems unusual, so let's describe why.

Yes the setter is very interesting, I think that many of the changes made to the Block entity might be generically useful for any sort of 1 to 1 config entity to plugin toolset, but that's kind of outside the scope of this patch at the moment. You will get the docs you've asked for, I promise. :-)

...shouldn't it be blockBuild() instead of buildBlock(), or otherwise something tat is unambiguously different from blockBuild()?

Well, I'm specifically getting the block system ready to move to FormInterface which does formBuild() and so I was following that paradigm, which I still think is a good idea. I'm open to suggestions here, but that's what I was going for, and I'll admit to having confused myself a few times already wondering whether I'd named it blockBuild() or buildBlock(). I think one of the earlier patches even has it wrong.

Everything else you commented didn't seem to need an answer, just needed doing. Soon as I get it green, I'll do all of the missing docs and interface updates and such, and then I'll begin the process of cleaning out some of the cruft this patch makes unnecessary any longer.

Eclipse

EclipseGc’s picture

StatusFileSize
new782 bytes
new20.89 KB
PASSED: [[SimpleTest]]: [MySQL] 53,158 pass(es).
[ View ]

ALL RIGHTY THEN... I tried to be lazy and reusing logic that was mostly identical, but yeah... that's not going to work in this unit tests and is unnecessary to test what we're testing so, this (I hope) returns all green now.

Eclipse (Huge thanks to xjm for bug hunting this).

xjm’s picture

Well, I'm specifically getting the block system ready to move to FormInterface which does formBuild() and so I was following that paradigm, which I still think is a good idea. I'm open to suggestions here, but that's what I was going for, and I'll admit to having confused myself a few times already wondering whether I'd named it blockBuild() or buildBlock(). I think one of the earlier patches even has it wrong.

Right, but we had the pattern of access() -> blockAccess(), etc. So that means we now have one method that doesn't follow that pattern while the rest do.

Yes, and I think this could expose some other issues here, with metadata and xss and translation, but this seemed the most logical way to handle it w/o it exploding into a bigger issue for the time being.

We definitely need a more thorough explanation in the comment, then, and maybe an @todo and followup issue?

<out_of_scope>
Is there any chance we could at least give getConfig()/setConfig() different names in a followup, or something? There's been an @todo in the patch to that end since December, and even after three months, I still don't understand why there are those two methods AND the settings() method on top of the annotation. I also still think it's weird for the admin label to be set in getConfig().

The issue around all that is #1764380: Merge PluginSettingsInterface into ConfigurablePluginInterface; all the comments there are relevant as well.
</out_of_scope>

EclipseGc’s picture

StatusFileSize
new23.56 KB

OK, this is just an interdiff while I figure out what's wrong.

EclipseGc’s picture

StatusFileSize
new816 bytes
new40.13 KB
PASSED: [[SimpleTest]]: [MySQL] 53,062 pass(es).
[ View ]

\Drupal:: not just Drupal::

I tried to address all the outstanding documentation issues in this. The patch size has inflated a bit because xjm's point about method names is pretty valid, so build() is still in the interface, but the BlockBase now defines an abstract blockBuild() method which all blocks had to conform to. Otherwise this is just a cleanup of 47. Hopefully this passes and looks good to everyone.

Eclipse

xjm’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -486,7 +489,8 @@ public function buildBlock() {
    +      // The label is the only string we actually render, so we need to handle
    +      // it specially.
           $build['#block']->label = check_plain($this->configuration['label']);

    Ah, this makes more sense. Let's add an @todo and followup issue here for translation and sanitization for this?

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -497,4 +501,14 @@ public function buildBlock() {
    +   * Returns a plugin specific render array.

    plugin-specific :) Though I'd actually suggest:
    Builds the renderable array of the block's content.

  3. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -187,6 +187,19 @@ public function get($property_name, $langcode = NULL) {
    +   * The plugin configuration is the canonical source for any configuration
    +   * within the plugin. The plugin does not care about the storage mechanism,
    +   * so the entity must maintain both its own properties as well as the
    +   * configuration of the plugin.
    +   *
    +   * Any property that exists in Block::exportProperties() can be handled by
    +   * the parent method, with the exception of the settings method. Any property
    +   * that is not documented in Block::exportProperties() is to be considered an
    +   * internal configuration property of the plugin and should be maintained on
    +   * the entity within the settings property. Setting of the settings property
    +   * directly is supported, but all set calls to the settings property are
    +   * addative and will not remove settings values.

    This helps a lot, thank you.

    Nitpicks: "All set() calls" should have parens, and "additive" is misspelled.

  4. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -209,6 +222,9 @@ public function set($property_name, $value) {
    +   * An array of keys of propeties, of this entity, that should be exportable.

    How about "Returns a list of properties to export for this entity." Then, the docblock also needs an @return, which is where you document that it's an array of property keys.

  5. +++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
    @@ -159,14 +175,58 @@ public function get($property_name, $langcode = NULL) {
    +    if (!in_array($property_name, $this->exportProperties()) && $property_name != 'settings') {
    +      $settings = $this->get('settings');
    +      $settings[$property_name] = $value;
    +      $this->getPlugin()->setConfig($property_name, $value);
    +      parent::set('settings', $settings);
    +    }
    +    elseif ($property_name == 'settings' && is_array($value)) {
    +      $settings = $this->get('settings');
    +      foreach ($value as $key => $key_value) {
    +        $this->getPlugin()->setConfig($key, $key_value);
    +        $settings[$key] = $key_value;
    +      }
    +      parent::set('settings', $settings);
    +    }
    +    else {
    +      $this->getPlugin()->setConfig($property_name, $value);
    +      parent::set($property_name, $value);
    +    }
    +  }

    Let's get a couple inline comments here too.

EclipseGc’s picture

StatusFileSize
new3.27 KB
new40.74 KB
PASSED: [[SimpleTest]]: [MySQL] 53,060 pass(es).
[ View ]

Solving 51

xjm’s picture

Thanks @EclipseGc!

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -198,17 +198,23 @@ public function get($property_name, $langcode = NULL) {
+      // If the property is not a property of the entity, assume it belongs in
+      // the settings property of the entity, and set the plugin configuration
+      // value appropriately.
       $settings = $this->get('settings');
       $settings[$property_name] = $value;
       $this->getPlugin()->setConfig($property_name, $value);
       parent::set('settings', $settings);
     }
     elseif ($property_name == 'settings' && is_array($value)) {
+      // If we are updating the settings property and the value passed was an
+      // array, add each of the key value pairs to the plugin configuration and
+      // set the entity's 'settings' property.
       $settings = $this->get('settings');
       foreach ($value as $key => $key_value) {
         $this->getPlugin()->setConfig($key, $key_value);
@@ -217,13 +223,17 @@ public function set($property_name, $value) {

@@ -217,13 +223,17 @@ public function set($property_name, $value) {
       parent::set('settings', $settings);
     }
     else {
+      // If we're updating a property of the entity, just add the value to the
+      // plugin configuration as well.

Reading this, I still don't understand why. For now, maybe leave it and someone who understands the problem space better than I can help clarify the comments.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -217,13 +223,17 @@ public function set($property_name, $value) {
+   * @return array

You still need to describe the return structure here. :) http://drupal.org/node/1354#return

sdboyer’s picture

ok, bit of a review.

in some respects, i think this patch makes the logic between blocks and their config even a bit more circuitous...but that's because it's a step on the way towards decoupling things, and that means a little more abstraction. so yeah, i think we're probably headed in a good direction.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -123,8 +123,16 @@ public function getPlugin() {
-        $this->instance = drupal_container()->get('plugin.manager.block')->createInstance($this->plugin, $this->settings, $this);
-        $this->settings += $this->instance->getConfig();
+        $properties = $this->getExportProperties();
+        unset($properties['settings']);
+        $this->instance = \Drupal::service('plugin.manager.block')->createInstance($this->plugin);
+        $this->instance->getConfig();
+        foreach ($properties as $key => $value) {
+          $this->instance->setConfig($key, $value);
+        }
+        foreach ($this->settings as $key => $value) {
+          $this->instance->setConfig($key, $value);

Ugh.

Though still probably an improvement over the circular situation where the entity contains info about getting the plugin, and the plugin then also contains the entity.

also, are the assorted calls to getExportProperties() enough to cause any kind of appreciable slowdown? it's not the cheapest way to access this config information.

in any case, it seems like this is a step in the right direction.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.php
@@ -159,14 +175,68 @@ public function get($property_name, $langcode = NULL) {
+   * The plugin configuration is the canonical source for any configuration
+   * within the plugin. The plugin does not care about the storage mechanism,
+   * so the entity must maintain both its own properties as well as the
+   * configuration of the plugin.
+   *
+   * Any property that exists in Block::exportProperties() can be handled by
+   * the parent method, with the exception of the settings method. Any property
+   * that is not documented in Block::exportProperties() is to be considered an
+   * internal configuration property of the plugin and should be maintained on
+   * the entity within the settings property. Setting of the settings property
+   * directly is supported, but all set() calls to the settings property are

i get the general reasoning here, but am having difficulty understanding specifically why the 'settings' property is special. it doesn't help that the initial line, "The plugin configuration is the canonical source for any configuration within the plugin" is almost a self-referential definition.

also - s/settings method/settings property/ in the second paragraph.

EclipseGc’s picture

StatusFileSize
new1.17 KB
new40.79 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Just the minor documentation cleanups xjm and sdboyer pointed out.

Eclipse

tim.plunkett’s picture

+++ b/core/modules/block/block.moduleundefined
@@ -327,7 +327,8 @@ function _block_get_renderable_region($list = array()) {
+        '#block' => (object) $block->getPlugin()->getConfig(),

@@ -534,7 +536,7 @@ function block_rebuild() {
-  $variables['block'] = (object) $variables['elements']['#block_config'];
+  $variables['block'] = (object) $variables['elements']['#block'];

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+        '#block' => (object) $this->getConfig(),

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
@@ -44,12 +44,7 @@ function testBlockThemeHookSuggestions() {
+    $variables['elements']['#block'] = (object) $block->getPlugin()->getConfig();

These (object) casts are obviously hacks, and we should keep track of them in some way, like an @todo or something.

+++ b/core/modules/block/block.moduleundefined
@@ -534,7 +536,7 @@ function block_rebuild() {
-  $variables['block'] = (object) $variables['elements']['#block_config'];
+  $variables['block'] = (object) $variables['elements']['#block'];

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+        '#block_config' => array(
+          'id' => $this->configuration['plugin'],
+          'region' => $this->configuration['region'],
+          'module' => $this->configuration['module'],
+          'label' => check_plain($this->configuration['label']),

Now we can ditch #block_config completely! Yay.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+      // @Todo: Give the implications of this line further thought in #1941244.

@todo, lowercase with no colon.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -489,4 +472,44 @@ public function submit($form, &$form_state) {
+   * Builds the renderable array of the block's content.
+   *
+   * @return array
+   *   A renderable array representing the content of the block.
+   *
+   * @see \Drupal\block\BlockRenderController
+   */
+  abstract public function blockBuild();

Is this in the interface? If not, why not? If so, it doesn't need to be here.

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -23,42 +23,6 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
-    // @todo \Drupal\block\Tests\BlockTest::testCustomBlock() assuemes that a
-    //   block can be rendered without any of its wrappers. To do so, it uses a
-    //   custom view mode, and we choose to only add the wrappers on the default
-    //   view mode, 'block'.
-    if ($view_mode != 'block') {
-      return array();

Where is this logic being handled now?

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -23,42 +23,6 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
-      '#block' => $entity,
-      '#weight' => $entity->get('weight'),
-      '#theme_wrappers' => array('block'),
-      '#block_config' => array(
-        'id' => $entity->get('plugin'),
-        'region' => $entity->get('region'),
-        'module' => $entity->get('module'),
-        'label' => check_plain($entity->label()),

I'm not sure how I feel about this being moved out of here. Ideally we'd be moving toward EntityRenderController, not away from it (we currently only use the interface, not the base class)

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -159,14 +175,69 @@ public function get($property_name, $langcode = NULL) {
+      // If the property is not a property of the entity, assume it belongs in
+      // the settings property of the entity, and set the plugin configuration
+      // value appropriately.
+      $settings = $this->get('settings');
+      $settings[$property_name] = $value;
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set('settings', $settings);
+    }
+    elseif ($property_name == 'settings' && is_array($value)) {
+      // If we are updating the settings property and the value passed was an
+      // array, add each of the key value pairs to the plugin configuration and
+      // set the entity's 'settings' property.
+      $settings = $this->get('settings');
+      foreach ($value as $key => $key_value) {
+        $this->getPlugin()->setConfig($key, $key_value);
+        $settings[$key] = $key_value;
+      }
+      parent::set('settings', $settings);
+    }
+    else {
+      // If we're updating a property of the entity, just add the value to the
+      // plugin configuration as well.
+      $this->getPlugin()->setConfig($property_name, $value);
+      parent::set($property_name, $value);

This whole thing is still a huge mess of crazy. I'm not sure how okay I am with this going in. It doesn't have any @todo's, and I can't imagine anyone volunteering to clean it up otherwise.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -179,11 +250,25 @@ public function getExportProperties() {
+    $this->settings = array_diff_key($this->getPlugin()->getConfig(), $this->getExportProperties());

Inline comment, please. I always have to think about what these array functions do, so some simple language explaining the intent more than the action would be good.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
@@ -35,14 +35,7 @@ public function __construct(array $namespaces) {
     $this->discovery = new CacheDecorator($this->discovery, 'block_plugins:' . language(LANGUAGE_TYPE_INTERFACE)->langcode, 'cache_block', CacheBackendInterface::CACHE_PERMANENT, array('block'));
-  }
-
-  /**
-   * Overrides \Drupal\Component\Plugin\PluginManagerBase::createInstance().
-   */
-  public function createInstance($plugin_id, array $configuration = array(), Block $entity = NULL) {
-    $plugin_class = DefaultFactory::getPluginClass($plugin_id, $this->discovery);
-    return new $plugin_class($configuration, $plugin_id, $this->discovery, $entity);
+    $this->factory = new DefaultFactory($this);

This is the only part of the patch I truly love :) Yay for this.

Status:Needs review» Needs work

The last submitted patch, 1927608-55.patch, failed testing.

xjm’s picture

Is this in the interface? If not, why not? If so, it doesn't need to be here.

Oooh, ooh! I know the answer!

The idea here is that this method is not part of the public interface for interacting with blocks--it's an internal method for block plugins to add their own customized build content without having to re-add the wrappers each time. (We are using the same pattern for other methods on this class as well.) So making it abstract here forces child blocks to declare it, without cluttering the public interface.

This whole thing is still a huge mess of crazy. I'm not sure how okay I am with this going in. It doesn't have any @todo's, and I can't imagine anyone volunteering to clean it up otherwise.

Yeah, this is really my feeling about that hunk as well.

tim.plunkett’s picture

Ah, it's only called internally, it should be protected. That would make sense, since interfaces cannot house protected methods.

xjm’s picture

Ah, it's only called internally, it should be protected. That would make sense, since interfaces cannot house protected methods.

I bet this is true of the other blockWhatsit() methods as well. Might be interesting to change them to protected and make sure nothing blows up. Edit: Out of scope here; let's file a novice issue.

EclipseGc’s picture

Status:Needs work» Needs review

These (object) casts are obviously hacks, and we should keep track of them in some way, like an @todo or something.

I think that's probably pretty fair. I'll see what I can do about filing a followup.

Now we can ditch #block_config completely! Yay.

I _THINK_ that's possible at this point yes. I wasn't brave enough to do it in this patch since it's not in my critical fix list, but I did everything I could to fix it, and I think a novice followup is probably in order.

Is this in the interface? If not, why not? If so, it doesn't need to be here.

No, build() is in the interface, just like form() and validate() and submit() but not blockForm(), blockValidate() and blockSubmit() so this is following the same convention. The big difference here is that every block MUST provide a blockBuild() method, but not every block might need to provide blockForm() method, so instead of providing an empty array response here (like other methods in this paradigm might) we could get away with just making that an abstract method.

Where is this logic being handled now?

It's actually not being handled at all. There weren't tests for that use case, and I generally hate view modes, so it got thrown away. My apologies, I failed to really read the comment. That being said, I'm still not stoked with the idea of a view mode for a block, because the plugin has no notion of that. What could be done is that the theme_wrapper could be made a configurable element of the block. I've been a proponent of this publicly since my original scope/explanation post for the initiative, and it's how panels works, so we have some good examples to follow here. If we wanted to follow that up with allowing the Block entity to swap wrappers by a view mode, that might be a very cool use case specific to the entity, but I don't really see view_mode as being anything but extraneous to blocks.

I'm not sure how I feel about this being moved out of here. Ideally we'd be moving toward EntityRenderController, not away from it (we currently only use the interface, not the base class)

This is kind of where we differ here. My point has always been that the entity is purely for storage. If the entity wants to do additional things the plugins don't do, then I'm fine with that being in the entity's controllers, but the plugin's basic functionality should not depend on the entity in any way. As such this logic had to move to the plugin. There may be aspects of it that should stay with the RenderController. Case in point, the entity_id based alter was clearly a function of the entity, and was left in the render controller while the main block_view and block_view_$plugin_id were moved to the plugin's domain because those can and should function regardless of the entity. I'm very open to other such arguments so long as the plugin continues to function without an entity doing any sort of work for it.

This whole thing is still a huge mess of crazy. I'm not sure how okay I am with this going in. It doesn't have any @todo's, and I can't imagine anyone volunteering to clean it up otherwise.

I'm not sure what is a mess or crazy about that. You have to maintain the plugin's configuration. To do otherwise is asking for problems, and I'll happily maintain an if/elseif/else statement that's this simple. I don't see any todos here except perhaps abstracting this up to it's own abstract class for other one to one entity/plugin situations. If the plugin expects configuration, and the entity mirrors the properties of that configuration, you HAVE to do this. I'm open to any suggestions, but the caveat there is that we have to maintain the plugin's configuration.

I'll try to fix the code things you pointed out here and get another patch up. Thanks for the review!

Eclipse

EclipseGc’s picture

I'm pretty sure we can't make protected abstract methods. Happy to be proven wrong, but I'm positive I tried that before and php shot me down. (61's a crosspost with everything that happened after 56).

Eclipse

EclipseGc’s picture

Issue summary:View changes

Expanded description of problem/solution and added followup issues.

EclipseGc’s picture

Issue summary:View changes

adding another followup

EclipseGc’s picture

StatusFileSize
new4.35 KB
new41.13 KB
PASSED: [[SimpleTest]]: [MySQL] 53,118 pass(es).
[ View ]

Tim and I discussed his comments in IRC and came to the following conclusions:

view_modes really don't have a place in blocks
the set() method is weird. We have to preserve its basic functionality of maintaining both the entity's properties and the plugin's configuration, but we should revisit exactly how that's done and if it can be done more simply (check issue summary for followups)
#block_config can be removed at this point (I guess testbot has the final word on that, but this patch removes it)

Did I miss anything in the code?

Eclipse

msonnabaum’s picture

Ok, I found this patch pretty difficult to comprehend, and I haven't read any of the comments here yet, so feel free to let me know if I'm misunderstanding what the code is doing.

Calling code:

<?php
>
-function
custom_block_block_view_alter(array &$build, Block $block) {
+function
custom_block_block_view_alter(array &$build, BlockInterface $block) {
  
// Add contextual links for custom blocks.
-  if ($block->getPlugin() instanceof CustomBlockBlock) {
+  if (
$block instanceof CustomBlockBlock) {
?>

This is an improvement.

<?php
>
   if (
$regions_to_render = overlay_get_regions_to_render()) {
-    if (!
in_array($block->get('region'), $regions_to_render)) {
+   
$configuration = $block->getConfig();
+    if (!
in_array($configuration['region'], $regions_to_render)) {
       return
FALSE;
     }
   }
?>

This is a regression. What's config mean on a block? I just want to know the region.

<?php
>
-       
'#block' => $block,
+       
'#block' => (object) $block->getPlugin()->getConfig(),
?>

Another regression. This is putting quite a burden on the calling code, having to know that much about your object.

Implementations:

<?php
>
  
/**
-   * Implements \Drupal\block\BlockBase::build().
+   * Implements \Drupal\block\BlockBase::blockBuild().
    */
-  public function build() {
+  public function
blockBuild() {
     return array(
drupal_get_form('search_block_form'));
   }
?>

It may just be naming, but as it is, the distinction/relationship between build and blockBuild makes no sense.

Internals:

This patch removes the Entity dependency from BlockBase, so instead, the plugin manager's factory returns an unconfigured block. It's now up to the caller to make it usable by calling setConfig, with whatever input it has. In the case of Entity\Block, it's coming from the config entity's properties.

I'm not crazy about the factory being so generic that all it can do is return an unusable object. That may be too much of an abstraction. In the case where a block would need to be setup with data from somewhere other than a config entity, and thus not discoverable by block.module, as long as it implements BlockInterface, it could do whatever it wanted with its own factory. I don't see that as a huge limitation.

Whatever this abstraction buys us though, it's really coming at the cost of complexity within Entity\Block.

<?php
>
 
/**
   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::get();
   */
 
public function get($property_name, $langcode = NULL) {
   
// The theme is stored in the entity ID.
   
$value = parent::get($property_name, $langcode);
    if (
$property_name == 'theme' && !$value) {
      list(
$value) = explode('.', $this->id());
    }
    elseif (!
in_array($property_name, $this->exportProperties())) {
     
// Support geting plugin-specific settings with the get() method.
     
$settings = parent::get('settings');
      if (isset(
$settings[$property_name])) {
       
$value = $settings[$property_name];
      }
    }
    return
$value;
  }
?>

This is super ugly.

<?php
>
   *
Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::set();
   *
   *
The plugin configuration is the canonical source for any configuration
  
* within the plugin. The plugin does not care about the storage mechanism,
   *
so the entity must maintain both its own properties as well as the
  
* configuration of the plugin.
   *
?>

This is just adding complexity and making the relationship even harder to reason about.

<?php
>

 
/**
   * Implements \Drupal\Core\Entity\EntityInterface::save().
   */
 
public function save() {
   
$this->settings = array_diff_key($this->getPlugin()->getConfig(), $this->getExportProperties());
    return
parent::save();
  }
?>

Ouch.

I can't say I'm terribly happy with the current state, as the relationship of Entity and Plugin to Blocks is very confusing, but I'm not sure the complexity introduced in this patch is worth what it buys us in flexibility.

EclipseGc’s picture

This is a regression. What's config mean on a block? I just want to know the region.

A block's configuration is roughly the same as an entity's properties. It's not a regression, we can still do all the same stuff we could do before. That hook also, no longer has the ability to update values of an entity and save them arbitrarily.

Another regression. This is putting quite a burden on the calling code, having to know that much about your object.

The burden IS on the calling code. That's sort of the point, the plugin is meant to provide only what it needs, and leaves the burden of implementation on the implementation, it doesn't take the burden of implementation details upon itself. We also likely have to completely change how blocks are cached and served in the long term, so that code will move yet again.

It may just be naming, but as it is, the distinction/relationship between build and blockBuild makes no sense.

block*() method names are all methods for classes that extend BlockBase so that the abstract class can do the sane things every implementation needs, and the class can provide stuff that's only specific to this plugin with block*() methods. Build follows this same paradigm.

This patch removes the Entity dependency from BlockBase, so instead, the plugin manager's factory returns an unconfigured block. It's now up to the caller to make it usable by calling setConfig, with whatever input it has. In the case of Entity\Block, it's coming from the config entity's properties.

The plugin is only unconfigured for block.module's use case. This is mostly due to the broken constructor followup issue I've filed. Ultimately we'd want to pass the entity's values to the plugin during construction, but there is further cleanup that needs doing for that to happen.

In the case where a block would need to be setup with data from somewhere other than a config entity, and thus not discoverable by block.module, as long as it implements BlockInterface, it could do whatever it wanted with its own factory. I don't see that as a huge limitation.

No, it can't. block module instantiates blocks through the BlockManager, currently the manager forces a block entity on every single block plugin. There is no ability to use a block plugin w/o a block entity in D8 right now. Even a custom block that doesn't extend BlockBase, and just implements BlockInterface will have to be in a Plugin/block/block directory structure with annotations to be found at which point the factory implementation for BlockManage will be expecting a block entity to exist to give to your block plugin. Furthermore all subsequent alter calls in the Render Controller expect to pass an entity as well. The entire block process, front to back, requires an entity.

Whatever this abstraction buys us though, it's really coming at the cost of complexity within Entity\Block.

Yes it does, because the Block Entity is a big part of the implementation, and that is where the complexity belongs. The plugin should be as uninterested with implementation as possible so that it can be reused across multiple implementations w/o the developer having to code around some core implementation assumptions. Every major contrib page layout tool, for at least the last 3 major versions, has been forced to code around core page layout assumptions, and that is exactly what I'm trying to prevent.

Eclipse

EclipseGc’s picture

Issue summary:View changes

adding another followup

tim.plunkett’s picture

StatusFileSize
new8.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Right now, we typehint with Drupal\block\Plugin\Core\Entity\Block in certain places, and that is not good, since it precludes other entities from using block plugins (think Display Suite or Panels). But when I look again, there is only one place that it is a problem: the BlockBase::__construct()/BlockManager::createInstance() pairing. And AFAIK, that's the part that irks @EclipseGc most.

The rest is hook_block_view_alter(), block.module or custom_block.module menu callbacks, or the Block entity controllers, which is all actually fine.

So to enable other modules or even core to properly access a block plugin, we just need a common interface and a PluginBag.

This is a WIP and I need to add tests to help illustrate my approach. Please don't tear it apart just yet.

xjm’s picture

Priority:Normal» Major

Also, thanks to a couple helpful souls and Tim's proof-of-concept there, I'm finally understanding what the real concerns are here. This issue needs to have a higher priority.

EclipseGc’s picture

Oh man, my bad. I totally forgot that issue existed. Yes definitely duplicate, but there's a lot of great conversation captured over there.

Eclipse

podarok’s picture

#66 looks pretty good

EclipseGc’s picture

Since people are now reviewing that patch, I am not ok with the approach. The objective is to remove the injection of an entity into the plugin. This was my first order of business (read the patch in 13) and the rest is all followup and fixing rendering so that we again, don't need an entity. I will get more detailed today, but this is just a quick post to make clear than I'm not ok with any proposal that continues passing the entity to the plugin.

Eclipse

EclipseGc’s picture

Speaking generically for a moment, embedding an entity that represents the configuration for a plugin into the plugin is a really bad idea. Not only is it a circular dependency (entity contains the plugin contains the entity) but it also provides the entity to every method of the plugin, and encourages passing the entity to any hooks the plugin type might create for its own uses. This is, by definition, tight coupling since an entity of some sort always has to be present for the plugin to work. The entity should be part of the implementation that wraps a plugin type. This distinction allows for plugin types to be reused outside of a single implementation. It also enforces separation of responsibility and concerns by forcing the plugin to always be able to perform its job regardless of the storage mechanism of its configuration.

Further reading from people-not-me can be found at the issue I should have posted this patch to initially (it's pretty short and well worth the read): http://drupal.org/node/1890534

I am not alone in my desire for the code to be this way. More reading on why this shouldn't be this way can be found in my detailed response to what was wrong with the issue that introduced this: http://drupal.org/node/1871696#comment-6948302

Eclipse

tim.plunkett’s picture

#66: block-1927608-66.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Blocks-Layouts, +Block plugins

The last submitted patch, block-1927608-66.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new8.78 KB
PASSED: [[SimpleTest]]: [MySQL] 53,684 pass(es).
[ View ]

Needed reroll because of #1875182: Pass plugin definition to plugin instances instead of discovery object.

This may not solve every concern of architectural purity, but it unblocks contrib or core displays from being able to use block plugins at all. Without this, Display Suite or Panels or any other module cannot use block plugins.

tim.plunkett’s picture

This would enable us to have stuff like

class KeyValueBlockContainer implements BlockContainerInterface
class PanelsPane implements BlockContainerInterface
class DisplaySuiteThingie implements BlockContainerInterface

and we can flesh out the interface once we have more implementations. This removes the coupling to Entity, and allows SCOTCH to make progress with displays.

msonnabaum’s picture

Status:Needs review» Reviewed & tested by the community

#75 makes the code more clear and adds flexibility, so it seems like an obvious win over what we have currently.

EclipseGc’s picture

Status:Reviewed & tested by the community» Needs work

Really? we're just going to rtbc this when it's pretty clear we do not have consensus on the approach or the scope of the patch? Let's discuss this a bit further yet because I am not on board and after the long conversation in IRC today, that should have been more than obvious. I filed this issue, I've worked a lot on this issue, and that patch doesn't actually solve the things I'm trying to solve. I'll detail the issues further either tonight or tomorrow, but this is anything but rtbc imo.

Eclipse

tim.plunkett’s picture

Yes, I spent about 4 hours today discussing this. @msonnabaum and @merlinofchaos also tried reasoning with you.

You say that the patch doesn't suit your needs, fine, write some tests that fail with it.

EclipseGc’s picture

tim.plunkett and I spent at least another hour discussing this tonight and I think we have some resolution and explanation as to what's happened thus far in this patch/issue.

Conflicting Values:

The plugin system currently expects arrays for configuration. There are a number of plugin types that break this, and while I can deal with that, having blocks (which is in my own back yard) be amongst them is upsetting. In addition to that, the existing entity controllers needed some love and could actually gain benefits to both the entity and plugins by fixing certain things there. With these values in hand, I set forth to return the block plugin to what I would characterize as the "natural state".

Tim (and others both here and in irc) have valued an interface/class for configuration over an array for many obvious reason that I have 0 problems with acknowledging. That said, it is not the plugin system's "natural state" and you have to do some weird things to get plugins to work that way. It makes code flow more difficult to read and sprinkles oddities through your plugin type.

Now, as I said I have no problems with an interface for configuration, but I'd prefer to avoid special-snowflake-syndrome, and enough other plugin types are doing something similar that we might be better off either moving to an interface for configuration whole hog, or at least providing another interface the plugin manager might use that expects this approach since the existing plugin factories do not.

With all that said, the agreed upon resolution to this is as follows:

Proposed Resolution:

1.) Work on a PluginConfigurationInterface and the corresponding plugin/manager/etc classes to make it work
2.) Move the BlockManager and all blocks over to it (this will require us to actually touch every block's form submission since it's currently using arrays for configuration in those)
3.) Move the Render Controller work in this patch into a follow up
4.) iterate on 1 & 2 until we have general happiness
5.) get the render/form/constructor followups in asap afterwards with an option to maybe do the constructor work sooner if it streamlines other work

Tim and I have both agreed to this as our next steps. I already have 1/2 well underway locally and will try to start putting out a patch for this in the next 2 days, and hopefully we can build this new toolset quickly for block's sake and move on to our follow ups.

Eclipse

EclipseGc’s picture

So, after much discussion and what we thought was a resolution, tim.plunkett approached me again with a... counter counter counter proposal. TL:DR we all still agree on an approach, but it's changed from the above proposal.

Tim spoke with Sam, and after some discussion came to the conclusion that thinking about the rendering process of the block was an important perspective to maintain with regard to the implications of the block entity's interactions with the block plugins. A huge factor in his initial conversion work was the fact that views needed to control the title of the block, but those titles need to function within any use case of a block plugin, not just when paired with block entities, and this affected how he viewed the entity. His proposal to me was to essentially lobotomize the block entity in favor of a settings array as a property of the entity, and only locate entity specific properties on the entity, not properties that could be valid in both cases. These sorts of properties would be machine_name, weight, region, and ultimately perhaps a few more. Any setting that's a setting of the plugins is only located in the block entity settings property, and this made instantiation of block plugins (which we have argued about a lot) very simple since the settings property is the only thing necessary to properly instantiate the block. I am totally on board with the approach and have reviewed a preliminary patch tim produced to make sure we're on the same page. He asked me to post this change of course so that he could focus on finishing that patch. We anticipate a little bit more back and forth on the next patch but have fundamentally removed the big blocking issues in the discussion.

What about the above proposal

This doesn't actually mean we're reversing anything in the above proposal, just that we're not going to try to build a Plugin system shim to get interface based configuration in before we fix blocks. This should open us up to move forward with the Display work in the rest of scotch and hopefully make any outstanding problems with the block system more obvious. Meanwhile, any improvements to the plugin system as a whole can incur the appropriate refactoring in systems using it when the time comes. Whether the block system is used as the test bed for such improvement will be punted into the implementation of any issue that might strive to solve these perceived deficits.

Eclipse

PS: tim.plunkett approved this message

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new90.05 KB
FAILED: [[SimpleTest]]: [MySQL] 53,863 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This introduces three main @todos.

- Block visibility is controlled by the plugin, but vertical tabs seem to be bound to being top-level elements of the form.
There are some workarounds to make this all work.
@EclipseGc says this isn't a big problem, since block visibility is changing completely anyway.

- Bartik and Overlay attempt to manipulate the block plugin based on the region it is placed in.
This introspection should not be allowed, according to sdboyer.

Thankfully/sadly, neither of the bartik/overlay hacks have test coverage, so this doesn't "break" anything.

EDIT: In case anyone thinks I can't count, Overlay and Bartik were #2 and #3 :)

EclipseGc’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -73,15 +37,16 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+      drupal_alter(array('block_view', "block_view_$name"), $build[$entity_id], $plugin);

I really think we should move hook_block_view_alter() the generic alter hook out of the render controller and into the block base as well.

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -92,32 +81,22 @@ class Block extends ConfigEntityBase {
+    $this->pluginBag = new BlockPluginBag(drupal_container()->get('plugin.manager.block'), array($this->plugin), $this);

Should probably be:

<?php
$this
->pluginBag = new BlockPluginBag(\Drupal::service('plugin.manager.block'), array($this->plugin), $this);
?>
+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -126,26 +105,7 @@ class Block extends ConfigEntityBase {
+    return $this->pluginBag->get($this->plugin);

Uhhh YAY!

+++ b/core/modules/menu/menu.moduleundefined
@@ -410,11 +410,12 @@ function _menu_parents_recurse($tree, $menu_name, $indent, &$options, $exclude,
+    $content = &$build['content']['#content'];

This is a little odd...

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -364,15 +362,19 @@ protected function drupalCreateContentType($settings = array()) {
+  protected function drupalPlaceBlock($plugin_id, array $settings = array()) {

YAY!

This seems like a good first pass. Generally ++ to the approach.

My only real issue with what I'm seeing here is that I'd like to put the BlockPluginBag through its paces a bit and just confirm for myself that it's doing what we will need in the long term. That aside, this seems really sane.

Eclipse

webchick’s picture

Not a full review, but a quickie... (Hooray for you two being on the same page btw!)

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -554,10 +554,10 @@ function aggregator_filter_xss($value) {
- * Implements hook_preprocess_HOOK() for block.tpl.php.
+ * Implements hook_preprocess_HOOK() for block-plugin.tpl.php.

+++ b/core/modules/block/block.moduleundefined
@@ -88,6 +89,14 @@ function block_help($path, $arg) {
+    'block_plugin' => array(
+      'variables' => array(
+        'content' => NULL,
+        'configuration' => NULL,
+        'plugin_id' => NULL,
+      ),
+      'template' => 'block-plugin',

At a glance, I don't like this at all. It's pushing implementation details way too far down the stack. Why should a themer have to care that blocks are plugins? They just need to know they're blocks. Is this absolutely required for this change? If so, why?

Also, I don't quite follow all the back and forth above, but bear in mind the phase of the release cycle we're in right now. It's fine to make incremental steps towards a vision, but each of those steps need to move core forward on its own, independently. We can't be introducing big changes that require dozens of follow-up issues to be complete anymore. From glancing at the patch, I don't believe this to be the case here, but the end of #80 scared the crap out of me, so figured I'd reiterate that. ;)

tim.plunkett’s picture

StatusFileSize
new91.75 KB
new96.4 KB
FAILED: [[SimpleTest]]: [MySQL] 53,846 pass(es), 2 fail(s), and 184 exception(s).
[ View ]
new11.67 KB

The naming of that is admittedly crap. Also, the new hooks hook_block_plugin_view_alter/hook_block_plugin_access are due to the same naming. (which have some @todos in the api.php docs, if you could fill them in @EclipseGc)

That is a bikeshed for #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin. But the idea is that "block plugin" needs a new name, since it will be used by both block and display entities.

And yes, part of the reason I abandoned my idealistic and high-level approach was because it would be very follow-up heavy. This should theoretically allow SCOTCH to stop caring about blocks altogether...

Attached is also a patch excluding whitespace changes, for easier reviewing. The file is almost the same size, but the diffstat is in the 400s, not 600s.

tim.plunkett’s picture

StatusFileSize
new3.54 KB
new98.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

EntityAccessController is bizarre, NodeAccessController has to work around them in the same exact way. I'll open an issue for that and link it here.
Also, I added docs for those hooks anyway.

Any other changes needed better make this patch smaller, I'm not going over 100K!

EclipseGc’s picture

@webchick

Each of the things I enumerated in 80 would have left core as functional or more so than it is currently. The message about incremental improvement is 100% heard, we're keeping it in mind, I promise. That being said, Tim's current patch is actually fixing plugin instantiation as well as all my points in point 5 of 80. For the record I'm completely ok with that. :-) It just means we get to mark two of my followups as redundant (wish that were a status).

In short, I think we're definitely moving core in the right direction and in the way that this phase of development expects. Tim's patch would actually fix a number of problems that exist in head just in general, as well as opening up Display work that sam is already actively doing in the princess branch. Hopefully we can iterate through this pretty quickly cause I'm very excited about the patch.

Eclipse

tim.plunkett’s picture

StatusFileSize
new97.51 KB
PASSED: [[SimpleTest]]: [MySQL] 53,875 pass(es).
[ View ]

Whoops, didn't rebase properly, that included the last two MAINTAINERS.txt commits... :\

EclipseGc’s picture

StatusFileSize
new14.06 KB
new107.66 KB
FAILED: [[SimpleTest]]: [MySQL] 53,985 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

ok, guess I'm to blame for pushing it over 100K.

Most of my changes here are code documentation except for a minor fix in the access controller and some test additions. If this passes, and there are none opposed, I am enthusiastically RTBC on this. Thanks for your work and for putting up with me tim. I do appreciate it.

Eclipse

PS: I think tim intends on swapping the build() blockBuild() methods right now since they're backwards which will push the file size up further, but is a change we both agree upon and isn't functional, so we've postponed it until there is agreement on this patch.

Status:Needs review» Needs work

The last submitted patch, 1927608-92.patch, failed testing.

tim.plunkett’s picture

#92: 1927608-92.patch queued for re-testing.

EDIT: Random test failures.

The last submitted patch, 1927608-92.patch, failed testing.

EclipseGc’s picture

StatusFileSize
new536 bytes
new107.71 KB
PASSED: [[SimpleTest]]: [MySQL] 54,037 pass(es).
[ View ]

OK, must have had bleed through from my installed site locally. sorry about that.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review

oops

sdboyer’s picture

i've talked this through with Kris - while this is kinda orthogonal o what's happening in princess, this shouldn't hamper it overmuch. the most salient issue continues to be *what* we inject into plugins - an array, or an entity.

this code goes the route of hiding the entity details from the plugin by making the entity simply report its relevant settings as an array, but still having external code interact with the entity. to a large extent, this is unavoidable in our current context as we still need to interact with the entities from the outside in order to keep blocks, as they exist right now, working.

as we've discussed all this at length, i've found it convincing that we could, and should, treat ConfigEntities like a dumb CRUD bag we are, of course, discovering elsewhere that some of the contracts inherent to entities are incompatible with this (ComplexDataInterface, TypedDataInterface, as well as D8MI wanting to decorate entities... etc.) - but i'm hopeful that we'll disentangle the simpler CRUD required for ConfigEntities from these more traditional entity constructs.

so, on princess, i've gone the route of ALWAYS injecting entities, but inverting the block plugin manager's factory-ish patterns so that calling code never/barely touches the entity - only the plugin. this addresses what has always been my primary concern, which is that i felt it confusing to force calling code to interact with configuration for a thing like it is the thing itself. i do feel that this is a preferable approach in general, and have included a set of interfaces (ConfigEntityAwarePluginInterface, EmbeddedPluginConfigInterface, ConfigDrivenMapperInterface). the biggest reason for this is that it gives us the *option* (as opposed to the requirement, like now) of having standalone blocks (i.e, not embedded in a single display) for free.

some of the separations in here are a bit uncomfortable (for example, the idea of two separate access hooks, one on the block entity and one on the block plugin, makes no sense in the context of SCOTCH), but i don't see anything that isn't pretty easily changed as needed. so, we can keep on rolling.

tim.plunkett’s picture

StatusFileSize
new19.78 KB
new124.96 KB
FAILED: [[SimpleTest]]: [MySQL] 53,914 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Okay, here it is with the method names swapped to match all of the other foo()/blockFoo() pairs. Consistency++

webchick’s picture

The key thing though is we need to leave core in a shippable state with every patch we commit from here out, and

/**
- * Implements hook_preprocess_HOOK() for block.tpl.php.
+ * Implements hook_preprocess_HOOK() for block-plugin.tpl.php.
  */
-function aggregator_preprocess_block(&$variables) {
-  if ($variables['block']->module == 'aggregator') {
+function aggregator_preprocess_block_plugin(&$variables) {
+  if ($variables['configuration']['module'] == 'aggregator') {

That's not a shippable state.

If that means postponing this on #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin then so be it.

webchick’s picture

To clarify, what's not shippable is:

s/block.tpl.php/block-plugin.tpl.php/g

I don't care about the internals of those preprocess functions as much, as they should affect relatively few people.

webchick’s picture

In IRC we discussed:

s/block-plugin.tpl.php/block-wrapper.tpl.php/g

This works for me. It's analogous to comment-wrapper.tpl.php and keeps block.tpl.php matching with what node.tpl.php and comment.tpl.php and others do.

tim.plunkett’s picture

Assigned:EclipseGc» tim.plunkett

I think you meant

s/block.tpl.php/block-wrapper.tpl.php
s/block-plugin.tpl.php/block.tpl.php

Will work on that, and confirm in IRC

webchick’s picture

Yes, sorry. I want block.tpl.php and node.tpl.php and comment.tpl.php to all look roughly the same. Sounds like #103 gets us there. Woohoo! :)

tim.plunkett’s picture

StatusFileSize
new26.08 KB
new134.79 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

I fully expect this to fail somewhere, but I jsut wanted to post something.

Status:Needs review» Needs work

The last submitted patch, blocks-1927608-105.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new6.46 KB
new138.28 KB
FAILED: [[SimpleTest]]: [MySQL] 53,990 pass(es), 9 fail(s), and 5 exception(s).
[ View ]

Okay, went through the test coverage, fixed the obvious stuff. Will need to see the results.

Status:Needs review» Needs work

The last submitted patch, block-1927608-107.patch, failed testing.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new6.41 KB
new142.6 KB
PASSED: [[SimpleTest]]: [MySQL] 53,858 pass(es).
[ View ]

One more pass...

                  __________
              _.-'|         \,._
           .-'    |         |   ~'=-.
        .-'       |         |       |
        | ___ []  | _______ |  []   |___.-----.
        ||___|    ||       ||       |___|__(*=/
        |_..._____||_______||.._____|   \##*----=
        [__        \__|||__,"     _/
           ((-))______--_____((-))
           | I |             | I |
           |   |             |   |
           || ||             || ||
           || ||             || ||    ____
           ((-))             ((-))   | ___\_
          -|===|=============|===|---|O___|_
           |   |             |   |   |____/
           /O.O\             /O.O\
          `====='           `====='
          -"^-^"-           -"^-^"-
EclipseGc’s picture

Status:Needs review» Reviewed & tested by the community

Enthusiastically RTBC

sdboyer’s picture

big +1 to webchick's request, as defined in #103, and glad that we went for that. should've raised it in my response, as well. i've got a 'block-ng' template in princess, but only as a way of letting things temporarily coexist without breakages while we convert things over. it's functionally more equivalent to what had originally been the "block-plugin.tpl.php"; i see no reason to think that what is now "block-wrapper.tpl.php" will continue to exist in over there.

xjm’s picture

Status:Reviewed & tested by the community» Needs review
+++ b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.phpundefined
@@ -0,0 +1,220 @@
+  /**
+   * Test configuration and subsequent form() and build() method calls.
+   *
+   * This test is attempting to ensure the existing block plugin api and all
+   * functionality that is expected to remain consistent. The arrays that are
+   * used for comparison can change, but only to include elements that are
+   * contained within BlockBase or the plugin being tested. Likely these
+   * comparison arrays should get smaller, not larger, as more form/build
+   * elements are moved into a more suitably responsible class.
+   */
+  public function testBlockInterface() {

I think it would be better to use a test plugin here, rather than the user block, to decouple the test from a specific implementation. Otherwise we have to maintain this relative to that plugin.

What are we testing for the absence of?

(Still reviewing.)

EclipseGc’s picture

OK, fair enough. I'll fix that. I really like this plugin though because it's doing a bunch of stuff, so I'm just going to copy it into a test module instead. Any objections to that?

webchick’s picture

No, it'd be a lot better if the test plugin were explicit about what it's testing. For example, a block that exposed a simple "Hello $foo" string, and had a config form to change the value of the string, or whatever. Because who's online does a lot, I can't tell what of those form properties are actually integral to making sure the architecture works, and what of those are just crap form api barfs in.

EclipseGc’s picture

StatusFileSize
new587 bytes
new143.93 KB
PASSED: [[SimpleTest]]: [MySQL] 54,181 pass(es).
[ View ]
new9.37 KB
new143.82 KB
FAILED: [[SimpleTest]]: [MySQL] 54,155 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Ok, as requested, a test block that just has a display_message configuration. It's very simple, and hopefully solves the above issues.

I had a sneaky suspicion that my site was bleeding through on something, so I'm providing two patches here+ an interdiff between the first patch and tim's patch and an interdiff between the first patch and the second patch. The second patch is just adding the role_permission table because I'm being paranoid. If the first patch passes, that's the better one to use.

Eclipse

EclipseGc’s picture

Yup, that's what I thought it'd do. Any further review?

Eclipse

webchick’s picture

I think both xjm and effulgentsia wanted to give this a spin before marking it RTBC.

effulgentsia’s picture

i see no reason to think that what is now "block-wrapper.tpl.php" will continue to exist in over there.

I see no reason for it to exist here either then. Why do we need a template and view_alter() hook for the entity at all? How about just hook_block_view_alter() and theme('block'), both invoked by the plugin, and that's it?

tim.plunkett’s picture

Two reasons:

  1. We need it for zebra striping.
  2. I was under the impression that hook_entity_view() and hook_entity_view_alter() were part of the contract of EntityRenderControllerInterface.

Sam wants to kill zebra striping in displays later, so I'm not sure about that.
But if you can hook_entity_view_alter all other entity types, why not this one?

effulgentsia’s picture

We need it for zebra striping.

We'll probably end up killing zebra striping either in the princess branch (cause princesses like unicorns, not zebras) or in #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors, but why is zebra striping relevant to this discussion? If the plugin sets up #theme => block, then block.tpl.php can still print out $zebra like any other template.

I was under the impression that hook_entity_view() and hook_entity_view_alter() were part of the contract of EntityRenderControllerInterface.

I don't think so. I think we expect all content entities to invoke them, but I wouldn't expect all (or frankly, even any) config entities to invoke them.

tim.plunkett’s picture

Block plugins cannot know what region they're in. That was one of the main things @sdboyer stressed to me.
And to continue doing the zebra striping and allowing themers to inspect regions, we need this wrapper.

Think of the wrapper as a remnant of the region-based page display. That's why it will hopefully be going away.

effulgentsia’s picture

I discussed this more with tim.plunkett and he explained the motivation of wanting the block.tpl.php and block-wrapper.tpl.php split now, so as to allow sandboxes/contrib to begin working properly with fully decoupled block plugins not wrapped in a block entity. I can appreciate that, so given that...

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.php
@@ -73,15 +37,16 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
-      drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build[$entity_id], $entity);
+      drupal_alter(array('block_view', "block_view_$name"), $build[$entity_id], $plugin);

Can we rename these alters to hook_block_wrapper_view_alter(), so as to allow the plugin to use hook_block_view_alter() instead of hook_block_plugin_view_alter()? I think it's important for these to be consistent with their corresponding theme functions.

Tim even suggested (only half seriously) that we could go all the way and rename the 'block' entity type to 'block_wrapper', and thus resolve #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin. But I suggest deferring that discussion and work to that issue.

EclipseGc’s picture

If that's a serious suggestion (and given that he officially suggested it over on #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin I assume it is. Then prepping the alters and tpls (tpls already being done) seems like a good compromise in order to prevent even more wide spread change later. I'm ++ to this idea. Changing the whole entity out here seems a bit beyond the scope, but doing the other parts, since we've altered them heavily already, seems very logical to me.

++

Eclipse

tim.plunkett’s picture

StatusFileSize
new9.09 KB
new144.67 KB
PASSED: [[SimpleTest]]: [MySQL] 54,196 pass(es).
[ View ]

Rerolling for #1947814: New config entities often save as langcode: und incorrectly anyway, it just did s/LANGUAGE_NOT_SPECIFIED/language_default()->langcode

I swapped the view_alters, and started fixing the hook_block_access hook_block_plugin_access calls as well, but hook_block_access isn't invoked directly by us, and is dependent on the entity type name, so that will be part of #1890534: It's confusing that "Block/$block" sometimes refers to the entity and sometimes to the plugin.

effulgentsia’s picture

Thanks. An unrelated question:

+++ b/core/modules/block/block.module
-      'render element' => 'elements',
+      'variables' => array(
+        'content' => NULL,
+        'configuration' => NULL,
+        'plugin_id' => NULL,
+      ),
...
+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
+    if ($content = $this->blockBuild()) {
+      $build = array(
+        '#theme' => 'block',
+        '#content' => $content,
+        '#configuration' => $this->configuration,
+        '#plugin_id' => $plugin_id,
+      );
+    }

Is there a reason we're changing theme('block') from acting on a render element to acting on a bunch of variables? Doing so creates the wtf of having a '#PROPERTY' (in the case above, #content) set to a render array, which is weird, and I can't tell what the benefit is to justify the wtf.

tim.plunkett’s picture

I did that out of personal preference. render element didn't seem to make sense here when I first started. Also, IIRC, it added yet another layer of nesting, $variables['elements']['content']['#content'] instead of $variables['content']['#content'].

alexpott’s picture

Tagging... after a conversation with @timplunkett in IRC about #1076132: hook_block_view_MODULE_DELTA_alter fails with menu blocks

effulgentsia’s picture

Status:Needs review» Needs work

Discussed the block.tpl.php vs. block-wrapper.tpl.php with sdboyer and Snugug, and that's not gonna fly. I'll open a spin-off issue that details why and moves us forward, but in the meantime, setting this to "needs work".

sdboyer’s picture

Status:Needs work» Postponed
Issue tags:+If SCOTCH fails

at @tim.plunkett's suggestion, we're gonna postpone this and mark it with 'if SCOTCH fails'. as @effulgentsia said, splitting off to the two templates is not the direction princess is going - we're keeping it all in a single template.

there are other relevant aspects here - namely, whether block plugins expect to interact with an array or an entity for config - but this patch went the all-array route, and i've gone the all-config-entity route in princess. i could still change things, and am open to doing so, but a) it seemed more elegant than continued reliance on anonymous arrays (assuming that we can escape some of the contracts associated with entities for ConfigEntities), and b) i think it gives us standalone blocks for free with fewer contortions. the proof will come when we create the forms for managing displays and are actually doing this CRUD, and at that point, we could still flop over to using arrays pretty easily, due to the groundwork done in this patch.

right now, though, it's best to look at this as a step forward if we DON'T use displays.

EclipseGc’s picture

Postponing this on the grounds of a new tpl being a minor wtf for front-enders seems a very very shallow reason to do so, especially given the sheer amount of stuff this fixes. We're trading one very minor wtf (that's hopefully temporary) for what is currently a very permanent set of about a dozen other wtfs. Please reconsider.

Eclipse

effulgentsia’s picture

I don't think this needs to be postponed until princess is committed or rejected, but I do think it needs to be rerolled to not have theme('block-wrapper'). That's not just a minor wtf: it's knowingly introducing something that will confuse themers working on Twig or updating their contrib themes, and we're 99% sure we'll end up reverting it, so it's a lot of confusion that is not needed by this issue. It was added to the scope of this issue with the belief that it's in the service of where SCOTCH will end up, but we now think that was an incorrect assumption. I'll reroll this when I have a chance, but leaving this postponed for now, because I think there are a couple dependent issues we'll want to work through first; I'll detail what I think they are when I have a chance.

sdboyer’s picture

Status:Postponed» Needs work
Issue tags:-If SCOTCH fails

yeah, i'll relent. having looked through the patch again, there's certainly benefit beyond that which is introduced by the template bifurcation. we pull that out, and it's worth having it go ahead.

some things will need refactoring, to be sure - the block plugin bag, for example, will need to be retooled on displays. but that's fine. the only big thing this really raises has to do with the interaction pattern around injecting entities vs. arrays into plugins, but again, i can still work either way in princess.

sorry to have jumped so quickly to postpone.

effulgentsia’s picture

To reunify block.tpl.php and block-wrapper.tpl.php, but retain the excellent back-end decoupling work that's been done here, we need to remove some variables from the theme('block') pipeline. Here's one: #1968322: Remove unused $id and $zebra variables from templates. I'll post others shortly. If any of those issues get stuck, I'll roll a patch here with some temporary shims, but I'm also hoping that most of those issues can land quickly and not need that.

effulgentsia’s picture

Here's the next one: #1968360: Remove per-region block markup. Please help spread the word about that issue to themers, so that if anyone has a concern with it, they can speak up.

webchick’s picture

Alex, could you post about this to http://groups.drupal.org/core maybe? That'd hit both planet and twitter.

webchick’s picture

and probably cross-post to http://groups.drupal.org/design-drupal and http://groups.drupal.org/theme-development for that matter.

I'd do it myself but I'm kinda mystified on why we're doing this, and figure you could explain it better.

webchick’s picture

For clarity, "this" == removing these template variables that've been around since at least Drupal 6, iirc.

Re-unifying these into a single template file gets a big "hell yeah!" from me. :D

EclipseGc’s picture

@webchick,

The short answer is that css selector support is robust enough in all the browsers we're supporting to replicate this functionality with the exception of zebra striping in IE, which is a.) covered by progressive enhancement and b.) no one zebra stripes blocks anyway so... Basically this is just a cleanup to remove the need to have 2 tpls.

Eclipse

effulgentsia’s picture

effulgentsia’s picture

StatusFileSize
new144.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1927608-140.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Just rerolled for HEAD changes. Not ready for review yet.

effulgentsia’s picture

While we wait for themer input on the other two issues, here's a no-brainer spin-off: #1969276: Refactor block plugins to make room for a BlockBase::build() implementation. Some might think it's silly to spin that off from this issue, but I work by applying patches to the codebase, and then using a git GUI to see what files are changed, and cutting that list down by a couple dozen is helpful for me.

tim.plunkett’s picture

Another spin-off from this patch: #1970208: Views blocks are missing contextual links

In this issue I cleaned up how contextual links were added to blocks, to remove one-off code from custom_block. It turns out that Views needs this.

effulgentsia’s picture

StatusFileSize
new97.72 KB
FAILED: [[SimpleTest]]: [MySQL] 54,544 pass(es), 160 fail(s), and 20 exception(s).
[ View ]

Still not ready for review, but here's a reroll that merges HEAD, is rebased on top of #1968360: Remove per-region block markup, and removes everything related to block-wrapper.

EclipseGc’s picture

Status:Needs work» Needs review

let's see if that passes now that #1968360: Remove per-region block markup is in.

Eclipse

Status:Needs review» Needs work

The last submitted patch, block-1927608-143.patch, failed testing.

tim.plunkett’s picture

That didn't apply because #1968360-44: Remove per-region block markup wasn't complete

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:-Configuration system, -Blocks-Layouts, -Configurables, -Block plugins

#143: block-1927608-143.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Blocks-Layouts, +Configurables, +Block plugins

The last submitted patch, block-1927608-143.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new31.38 KB
new110.74 KB
FAILED: [[SimpleTest]]: [MySQL] 54,332 pass(es), 165 fail(s), and 18 exception(s).
[ View ]

Attempted to fix everything except the openid stuff. Ran into a couple hiccups. There was some logic in the BlockBase::validate/submit methods that expected to work with the entity's setup for the form. Fixing that lead me to the fact that I couldn't get the visibility form elements to move within the form appropriately, I screwed with that and screwed with it, and finally decided we wanted to move visibility to its own property on the entity ultimately anyway, and we've fixed every other controller in this patch thus far, so I went ahead and put together a provisional fix for that as well, so block visibility is in the entity access controller for blocks now.

Hopefully this passes everything except the openid tests.

Eclipse

tim.plunkett’s picture

Status:Needs review» Needs work
+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -23,12 +23,10 @@ public function __construct(array $configuration, $plugin_id, array $plugin_defi
-      'label' => '',
-      'admin_label' => $plugin_definition['admin_label'],

@@ -218,7 +159,7 @@ public function form($form, &$form_state) {
-      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $this->configuration['admin_label'],
+      '#default_value' => $this->configuration['label'],

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockStorageUnitTest.phpundefined
@@ -139,9 +138,11 @@ protected function renderTests() {
     $expected[] = '  <div id="block-test-block"  class="block block-block-test">';
-    $expected[] = '      ';
-    $expected[] = '<div class="content">';
-    $expected[] = '  </div>';
+    $expected[] = '';
+    $expected[] = '    <h2 class="">Test block html id</h2>';
+    $expected[] = '  ';
+    $expected[] = '  <div class="content">';
+    $expected[] = '      </div>';

When I read these code changes, I thought "hm, I wonder how that still passes tests?"

Well it doesn't, you just changed the tests.

That doesn't have anything to do with visibility or fixing test failures, this has to be reverted.

EclipseGc’s picture

well, I changed it cause the configuration was wrong, and once I corrected it the title started showing up. If we don't want the title, then we need to be setting the configuration appropriately. I'm cool with whatever that needs to look like, but the label/admin_label in configuration wasn't the way it was suppose to be, and now that it is, we're getting block titles in places we weren't before.

At least, that's my perception here. Am I crazy?

Eclipse

tim.plunkett’s picture

The two renders in that test are as follows:

    $entity = $this->controller->create(array(
      'id' => 'stark.test_block',
      'plugin' => 'test_html_id',
    ));

    // Test the rendering of a block.
    $output = entity_view($entity, 'block');

That does not set a title, the test_html_id block plugin has no custom default title or anything, so the block should not output the title.

Then we take that same entity:

    $entity->set('label', 'Powered by Bananas');
    $output = entity_view($entity, 'block');

We provide a label, and when rendering, the block has a label.

Combining the concept of admin_label and label is out of scope for this issue. That used to be called "subject", and had/has more than one issue about it.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new32.71 KB
new3.63 KB
new109.31 KB
FAILED: [[SimpleTest]]: [MySQL] 54,373 pass(es), 166 fail(s), and 17 exception(s).
[ View ]

Logic accepted!

Hopefully this patch is more agreeable. Instead of setting the label default to the definition admin_label, it's being set to the empty string. I don't see why we need admin_label at the moment in the configuration. This logic allows instantiation to continue as it did in 149, the only changes to the block rendering tests should be whitespace in nature at this point I think. I'm providing two interdiffs the first from 149 and the second from 143. Hopefully that helps show exactly what I did here and why.

Also, with the failures in the previous patch I fixed a couple of block hook implementations that were looking for settings visibility.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-153.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new4.13 KB
new108.92 KB
FAILED: [[SimpleTest]]: [MySQL] 54,547 pass(es), 155 fail(s), and 17 exception(s).
[ View ]

ok, hopefully this one fixes everything except the OpenId tests. Working on those now.

Eclipse

EclipseGc’s picture

StatusFileSize
new332 bytes
PASSED: [[SimpleTest]]: [MySQL] 54,637 pass(es).
[ View ]

Trying something...

EclipseGc’s picture

I'm very disappointed this passed. I can't get HEAD's openid tests to pass locally for me, so I must have some configuration issue with my MAMP or something. I'll have to resolve that before I can resolve these tests. If anyone else what's to step in on this, please feel free.

Eclipse

EclipseGc’s picture

StatusFileSize
new1.65 KB
new107.78 KB
PASSED: [[SimpleTest]]: [MySQL] 54,923 pass(es).
[ View ]

Ok, I think this has a chance of passing :-D Apparently the block_view alteration in openid.module was completely wrong.

Eclipse

tim.plunkett’s picture

+++ b/core/modules/menu/menu.moduleundefined
@@ -399,13 +399,13 @@ function _menu_parents_recurse($tree, $menu_name, $indent, &$options, $exclude,
-function menu_block_view_alter(array &$build, Block $block) {
+function menu_block_view_system_menu_block_alter(array &$build, BlockInterface $block) {
   // Add contextual links for system menu blocks.
-  if ($block->getPlugin() instanceof SystemMenuBlock) {
-    foreach (element_children($build['content']) as $key) {
-      $build['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['content'][$key]['#original_link']['menu_name']));
+  if (isset($build['#content'])) {
+    foreach (element_children($build['#content']) as $key) {
+      $build['#content']['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['#content'][$key]['#original_link']['menu_name']));

This would need the same changes as openid just had, but its masked by the isset($build['#content']). I really hope tests fail for this, or we need new ones.

+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.phpundefined
@@ -20,7 +20,78 @@ class BlockAccessController extends EntityAccessController {
+    // @todo \Drupal\Core\Entity\EntityAccessController::viewAccess() casts to a
+    //   Boolean, which incorrectly implies that access was always explicitly
+    //   granted or denied. To work around this call
+    //   \Drupal\Core\Entity\EntityAccessController::access() directly and check
+    //   the return type.

If #1947892: Improve DX with EntityAccessControllerInterface goes in, we need to revisit this before commit.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -20,20 +19,17 @@
-  /**
-   * Overrides \Drupal\Component\Plugin\PluginBase::__construct().
-   */
-  public function __construct(array $configuration, $plugin_id, array $plugin_definition, Block $entity) {
+  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {

I guess this needs {@inheritdoc} now

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings =  array();

double space here

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings =  array();

@@ -49,11 +214,16 @@ public function submit(array $form, array &$form_state) {
+    $settings =  array();

These have an extra space. Also they are completely unnecessary

+++ b/core/modules/block/lib/Drupal/block/BlockFormController.phpundefined
@@ -38,7 +193,17 @@ public function validate(array $form, array &$form_state) {
+    $settings['values'] = &$form_state['values']['settings'];
+    $entity->getPlugin()->validate($form, $settings);

@@ -49,11 +214,16 @@ public function submit(array $form, array &$form_state) {
+    $settings['values'] = &$form_state['values']['settings'];
+    $entity->getPlugin()->submit($form, $settings);

This could use an inline comment

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -0,0 +1,69 @@
+   * @param Plugin\Core\Entity\Block $entity

missing \Drupal\block\

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -0,0 +1,69 @@
+   * Overrides \Drupal\Component\Plugin\PluginBag::initializePlugin().

+++ b/core/modules/block/lib/Drupal/block/BlockStorageController.phpundefined
@@ -53,4 +39,13 @@ public function loadByProperties(array $values = array()) {
+   * Overrides \Drupal\Core\Config\Entity\ConfigStorageController::preSave().

+++ b/core/modules/block/lib/Drupal/block/Plugin/Core/Entity/Block.phpundefined
@@ -92,32 +74,34 @@ class Block extends ConfigEntityBase {
+   * Overrides \Drupal\Core\Config\Entity\ConfigEntityBase::__construct();

@@ -162,6 +127,14 @@ public function uri() {
+   * Overrides \Drupal\Core\Entity\Entity::label();

+++ b/core/modules/block/tests/lib/Drupal/block_test/Plugin/block/block/TestBlockInstantiation.phpundefined
@@ -0,0 +1,69 @@
+   * Overrides \Drupal\block\BlockBase::settings().
...
+   * Overrides \Drupal\block\BlockBase::blockAccess().
...
+   * Overrides \Drupal\block\BlockBase::blockForm().
...
+   * Overrides \Drupal\block\BlockBase::blockSubmit().
...
+   * Implements \Drupal\block\BlockBase::blockBuild().

{@inheritdoc}

+++ b/core/modules/block/tests/config/block.block.stark.test_block.ymlundefined
@@ -1,11 +1,11 @@
-label: ''
region: '-1'
-weight: ''
-module: block_test
-status: '1'
-visibility: {  }
plugin: test_html_id
settings:
+  label: ''
+  weight: ''
+  module: block_test
+  status: '1'
+  visibility: {  }
   cache: '1'
   admin_label: 'Test block html id'

I moved these all around, but I'm not sure that they were re-exported since all of the recent changes were made. All of these should be checked again.

One of the easiest ways is to copy them into into the sites/default/files/config_1234/staging and looking at the diffs

EclipseGc’s picture

StatusFileSize
new6.01 KB
new108.3 KB
PASSED: [[SimpleTest]]: [MySQL] 54,948 pass(es).
[ View ]

ok, this should have addressed all of tim's concerns except for the contextual links issue, the block CMI files and the access issue that's contingent on another issue.

Eclipse

EclipseGc’s picture

StatusFileSize
new2.09 KB
new109.41 KB
PASSED: [[SimpleTest]]: [MySQL] 55,215 pass(es).
[ View ]
new109.47 KB
FAILED: [[SimpleTest]]: [MySQL] 55,366 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This should add tests for the missing contextual links. Assuming this looks right I think the only outstanding thing will be reworking the config files.

Eclipse

EclipseGc’s picture

StatusFileSize
new10.12 KB
new107.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,502 pass(es).
[ View ]

And this should fix the config files.

Eclipse

tim.plunkett’s picture

Assigned:effulgentsia» EclipseGc
Status:Needs review» Needs work
StatusFileSize
new21.79 KB
+++ b/core/profiles/standard/config/block.block.bartik.content.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Main page content'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.bartik.footer.ymlundefined
@@ -1,9 +1,16 @@
+  label: Footer
...
-label: 'Footer menu'

+++ b/core/profiles/standard/config/block.block.bartik.help.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'System Help'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.bartik.powered.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Powered by Drupal'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.seven.content.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'Main page content'

@@ -14,9 +22,3 @@ visibility:
-label: ''

+++ b/core/profiles/standard/config/block.block.seven.help.ymlundefined
@@ -1,8 +1,16 @@
+  label: 'System Help'

@@ -14,9 +22,3 @@ visibility:
-label: ''

The new settings:label needs to match the original top level label.
Otherwise we end up with something like this:
main-page-content.png

The minimal and core/modules/block/tests/config blocks weren't converted.

Curious, how did you convert these? Some things are quoted, others are not. Was it by hand, or copying out of your active config?

EclipseGc’s picture

I deleted all the blocks and reconfigured them by hand, and then copied and pasted and tried to sanity check.

We can't have blank labels, the way to code works expects a label at all times but we can make it not visible. I missed that on a couple apparently. That's a technical limitation of the way the system has been evolving. Thoughts?

Eclipse

EclipseGc’s picture

StatusFileSize
new5.78 KB
new107.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,581 pass(es).
[ View ]

ok, I think I fixed all the label visibility issues and added the missing block configurations.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
tim.plunkett’s picture

Assigned:EclipseGc» effulgentsia

I'm pretty damn happy with this thing considering what we've gone through. Thanks especially to @EclipseGc, @webchick, and @effulgentsia.

I'd really like to have @effulgentsia be the one to RTBC this, as a good deal of this code is still mine.

I just have one question remaining about the patch:

+++ b/core/modules/block/block.moduleundefined
@@ -499,7 +499,7 @@ function _block_get_renderable_block($element) {
-    $element += entity_view($block, 'block');
+    $element = NestedArray::mergeDeep($element, entity_view($block, 'block'));

I don't remember this change, why do we need it exactly? I can see why the original code might not work in some cases, but $element = entity_view($block, 'block') + $element; should be fine here, right?

EclipseGc’s picture

StatusFileSize
new107.35 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

Rerolling against head.

Eclipse

Status:Needs review» Needs work
Issue tags:-Configuration system, -Blocks-Layouts, -Configurables, -Block plugins

The last submitted patch, 1927608-168.patch, failed testing.

EclipseGc’s picture

Status:Needs work» Needs review

#168: 1927608-168.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Configuration system, +Blocks-Layouts, +Configurables, +Block plugins

The last submitted patch, 1927608-168.patch, failed testing.

effulgentsia’s picture

I think this patch is getting very close. Thanks for all the patience and persistence.

+++ b/core/modules/block/block.api.php
+function hook_block_plugin_access(\Drupal\block\BlockInterface $block) {
+  if ($block->getPluginID() == 'my_plugin_id') {
+    return FALSE;
+  }
+}

Hm. I was happy that in the last 50 comments or so we were able to remove the distinction of 'block' vs. 'block_plugin' from hook_block_view_alter(). So, I'm sad to see the distinction re-emerge for access. Also, in hook_block_view_alter(), 'block' in the hook name refers to the block plugin. But the above makes it different for access. Would it be possible to remove hook_block_plugin_access() from this patch (since it's a new hook that doesn't exist in HEAD), and have a follow up for figuring out how to add it with sane terminology?

+++ b/core/modules/block/block.module
@@ -499,7 +499,7 @@ function _block_get_renderable_block($element) {
-    $element += entity_view($block, 'block');
+    $element = NestedArray::mergeDeep($element, entity_view($block, 'block'));

I agree with #167. Let's remove this hunk and see if anything breaks.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -500,9 +270,20 @@ public function blockSubmit($form, &$form_state) {}
+    drupal_alter(array('block_view', "block_view_$name"), $build, $this);

block.api.php still lists hook_block_view_ID_alter(), but the above fails to call it. I see 3 options:

1) Inject $instance_id into the plugin during construction.
2) Add it as a parameter to the build() method.
3) Add a more generic parameter to build(), like $extra_alters that could be an array.

Any other ideas or thoughts on these?

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php
@@ -45,12 +45,9 @@ function testBlockThemeHookSuggestions() {
+    $variables['elements']['content']['#children'] = '';

I think $variables['elements']['content'] = array() would be cleaner.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -364,15 +362,20 @@ protected function drupalCreateContentType($settings = array()) {
-  protected function drupalPlaceBlock($plugin_id, array $values = array(), array $settings = array()) {
-    $values += array(
+  protected function drupalPlaceBlock($plugin_id, array $settings = array()) {

Why couple $values and $settings here, when the rest of this issue is all about decoupling?

+++ b/core/profiles/minimal/config/block.block.stark.admin.yml
@@ -1,9 +1,16 @@
-label: Administration
-weight: '1'
+label: ''
status: '1'
+settings:
+  label: Administration
+  weight: '1'
+  status: '1'

Why move 'weight' inside settings? Why leave a (empty) 'label' outside settings? Any why add a copy of 'status' inside settings?

effulgentsia’s picture

StatusFileSize
new107.41 KB

Just a merge of HEAD. Same errors as #168. Will follow up with additional patches and interdiffs.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new6.74 KB
new106.72 KB
FAILED: [[SimpleTest]]: [MySQL] 55,781 pass(es), 83 fail(s), and 1,071 exception(s).
[ View ]

This is 90% just indentation changes.

Status:Needs review» Needs work

The last submitted patch, 1927608-174.patch, failed testing.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new10.89 KB
new111.26 KB
FAILED: [[SimpleTest]]: [MySQL] 55,753 pass(es), 85 fail(s), and 1,091 exception(s).
[ View ]

I discussed #172 with EclipseGc and tim.plunkett. Here's a fix for all but the last two parts. I also cleaned up the documentation of block.api.php to match similar wording and naming used by hook_node_view_alter(), hook_form_alter(), and hook_node_access().

I still want to do the rest of #172, but want to get this green first.

Status:Needs review» Needs work

The last submitted patch, 1927608-176.patch, failed testing.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new876 bytes
new111.27 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

silly typo

effulgentsia’s picture

StatusFileSize
new759 bytes
new111.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,577 pass(es).
[ View ]

And another one.

effulgentsia’s picture

Status:Needs review» Needs work

Yay, green! Back to needs work for the last two items in #172.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new12.21 KB
new112.31 KB
FAILED: [[SimpleTest]]: [MySQL] 55,646 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

I expect this to pass, but if it does we have a problem, so checking.

I did move all the configuration file stuff into this, based upon conversations with tim and alex we'll be punting on the drupalPlaceBlock() issue. It works as is, and shouldn't hold this patch up.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-181.patch, failed testing.

EclipseGc’s picture

All the failing tests should be easy fixes I think, I'm pretty sure that block re-ordering is only working on the administrative side. Do we have a test that should be proving block rendering reordering is working? I'll dig into that shortly.

Eclipse

tim.plunkett’s picture

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new2.18 KB
new112.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,709 pass(es).
[ View ]

This should fix the tests and get this passing. Since the block re-ordering on the front end isn't working in HEAD currently, I think that's safe to leave for a follow up and there appears to be an issue already per #184.

Eclipse

EclipseGc’s picture

StatusFileSize
new112.06 KB
FAILED: [[SimpleTest]]: [MySQL] 55,987 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

merging with head. custom_block.module had a conflict with some use statements that I could not find a use case for in the actual code. Removed all of them, ran tests locally and got a green light. Let's see if the full test suite is ok with it too.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 1927608-186.patch, failed testing.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new112.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,884 pass(es).
[ View ]

This just moves TestBlockInstantiation to the new and improved block plugin location per #1987298: Shorten directory structure and PSR-0 namespacing for plugins.

effulgentsia’s picture

Assigned:effulgentsia» Unassigned
Issue tags:+Avoid commit conflicts

I'm happy with #188. #167 asked me to RTBC it, and I might later tonight, but am holding off for now to give @tim.plunkett a chance for a final review of the latest changes.

Since I think we're within hours of an RTBC here, I'm restoring the "Avoid commit conflicts" tag that I removed in #131. Fortunately, this patch does not conflict with #1875992-114: Add EntityFormDisplay objects for entity forms.

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

I'm just going to go ahead and RTBC at this point. It seems we're finally all happy!

effulgentsia’s picture

StatusFileSize
new759 bytes
new112.81 KB
PASSED: [[SimpleTest]]: [MySQL] 55,915 pass(es).
[ View ]

Thanks! Just some docs fixes. Leaving at RTBC.

effulgentsia’s picture

StatusFileSize
new1.61 KB

Ignore the interdiff.txt in #191. That patch is correct, but the interdiff is this one. Apologies.

effulgentsia’s picture

Issue summary:View changes

Updated issue summary.

effulgentsia’s picture

Issue summary:View changes

Updated issue summary.

effulgentsia’s picture

And I updated the issue summary. Please correct/improve it as needed.

EclipseGc’s picture

looks good to me, I'm on the RTBC train here, and the issue summary changes seem fine.

Eclipse

alexpott’s picture

Nearly all of this is pretty minor. Overall I think the direction of the patch is excellent and makes lots of sense. I especially like the BlockPluginBag...

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.phpundefined
@@ -106,7 +106,7 @@ public function testRecentNodeBlock() {
-    $block->set('settings', array('block_count' => 10));
+    $block->getPlugin()->setConfig('block_count', 10);

Love it!

… and now the review :)

+++ b/core/modules/block/block.api.phpundefined
@@ -11,113 +11,90 @@
- * Perform alterations to a specific block.
+ * Provide a block-specific alteration instead of the global hook_block_view_alter().
  *

The function doc seems a bit wordy and is longer than 80 chars... plus we have the @see hook_block_view_alter() at the end… not sure this change is necessary.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -21,19 +19,17 @@
abstract class BlockBase extends PluginBase implements BlockPluginInterface {

The BlockBase class implements two methods access() and blockAccess() - I'm wondering if we still need two functions here. Also the docblock for the access() method needs work as it still referes to functionality that has been moved to BlockAccessController.

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -222,179 +137,29 @@ public function access() {
     $definition = $this->getDefinition();
-    $form['id'] = array(
-      '#type' => 'value',
-      '#value' => $entity->id(),
-    );
     $form['module'] = array(
       '#type' => 'value',
       '#value' => $definition['module'],
     );

+    $definition = $this->getDefinition();

the second $definition = $this->getDefinition(); is unnecessary (afaics).

+++ b/core/modules/block/lib/Drupal/block/BlockPluginBag.phpundefined
@@ -0,0 +1,69 @@
+      if (!$module || module_exists($module)) {

Probably should be using Drupal::moduleHandler() here.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockInterfaceTest.phpundefined
@@ -0,0 +1,111 @@
+    $manager = \Drupal::service('plugin.manager.block');

should be $manager = $this->container->get('plugin.manager.block');

+++ b/core/modules/block/templates/block.tpl.phpundefined
@@ -5,13 +5,9 @@
- *    configured to hide the title ($block->label is empty in this case).
+ * - $plugin_id: The ID of the block implementation.
+ * - $configuration: An array of the block's configuration values.
  * - $content: Block content.
- * - $block->module: Module that generated the block.
- * - $block->delta: An ID for the block, unique within each module.
- * - $block->region: The block region embedding the current block.
  * - $attributes: An instance of Attributes class that can be manipulated as an
  *    array and printed as a string.

Missing the new $title variable.
Probably could do with example of whats inside the $configuration array... like the description of $attributes below… it should contain what is always going to be there eg. module, label...

+++ b/core/modules/menu/menu.moduleundefined
@@ -12,7 +12,7 @@
use Drupal\system\Plugin\Block\SystemMenuBlock;

This line can be removed now as SystemMenuBlock is no longer used in menu.module.

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.phpundefined
@@ -356,8 +356,6 @@ protected function drupalCreateContentType($settings = array()) {
    *   - machine_name: Random string.
    *   - region: 'sidebar_first'.
    *   - theme: The default theme.
-   * @param array $settings
-   *   (optional) An associative array of plugin-specific settings.
    *
    * @return \Drupal\block\Plugin\Core\Entity\Block
    *   The block entity.
@@ -365,15 +363,20 @@ protected function drupalCreateContentType($settings = array()) {

@@ -365,15 +363,20 @@ protected function drupalCreateContentType($settings = array()) {
    * @todo
    *   Add support for creating custom block instances.
    */
-  protected function drupalPlaceBlock($plugin_id, array $values = array(), array $settings = array()) {

Need to add - visibility: empty array. to the list of defaults used by drupalPlaceBlock() which I think means access on all pages for everyone.

+++ b/core/modules/system/system.moduleundefined
@@ -2718,20 +2718,18 @@ function system_user_timezone(&$form, &$form_state) {
+    case 'system_menu_block':
+      $variables['attributes_array']['role'] = 'navigation';
+      $variables['classes_array'][] = 'block-menu';

I think this should look like the code below... there is a patch somewhere to fix the other cases in system_preprocess_block() but we're touching this one so maybe we should fix it.

    case 'system_menu_block':
      $variables['attributes']['role'] = 'navigation';
      $variables['attributes']['classes'][] = 'block-menu';
alexpott’s picture

Status:Reviewed & tested by the community» Needs work

Setting back to need work because I'm sure that atleast the duplicate $definition = $this->getDefinition(); should be removed. :)

catch’s picture

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -547,7 +547,7 @@ function aggregator_filter_xss($value) {
function aggregator_preprocess_block(&$variables) {
-  if ($variables['block']->module == 'aggregator') {
+  if ($variables['configuration']['module'] == 'aggregator') {
     $variables['attributes']['role'] = 'complementary';
   }

Why is module in configuration here?

+++ b/core/profiles/standard/config/block.block.seven.login.ymlundefined
@@ -1,7 +1,13 @@
-plugin: user_login_block
+weight: '10'
status: '1'
+langcode: en
+region: content
+plugin: user_login_block
settings:
+  label: 'User login'
+  module: user
+  label_display: visible
   cache: '-1'

But settings here? I can see lots of people looking in $variables['settings'] or putting a configuration key instead settings in the YAML etc.

It's also weird that we specify both the plugin and the module in block config but that predates the patch so meh.

Couldn't find much else to complain about.

catch’s picture

Issue summary:View changes

Updated issue summary.

effulgentsia’s picture

Issue summary:View changes

Updated issue summary.

effulgentsia’s picture

For #197, I opened #1991438: Terminology confusion: 'settings' vs. $configuration and #1991442: Remove 'module' from block plugin configuration and added them to the followup issues in the summary.

This issue is still needs work on #195.

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new18.13 KB
new124.2 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

OK, this is completely untested, but should basically cover all of the feedback in #195. The changes were big enough in a couple places it seemed better to let testbot have first shot at it.

Eclipse

EclipseGc’s picture

StatusFileSize
new722 bytes
new124.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,505 pass(es).
[ View ]

I missed the leading slash on the Drupal class in the BlockPluginBag.

effulgentsia’s picture

Status:Needs review» Reviewed & tested by the community

Changes look good, and address all of #195.

alexpott’s picture

Title:Remove the tight coupling between Block Plugins and Block Entities» Change notice: Remove the tight coupling between Block Plugins and Block Entities
Priority:Major» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Avoid commit conflicts+Needs change record

Committed f630b51 and pushed to 8.x. Thanks!

We'll need to update the original blocks as plugins change notice... https://drupal.org/node/1880620

alexpott’s picture

Issue summary:View changes

Added follow up issue to summary

YesCT’s picture

https://drupal.org/contributor-tasks/draft-change-notification
is about drafting a change record from scratch, but might be helpful here with ideas on what info needs to be added to https://drupal.org/node/1880620 to update it.

After someone makes that update, set this to needs review and people will check it.

catch’s picture

tedbow’s picture

I think I found 2 issues related to changes made in this.

They are caused by overlay and node modules implementations of hook_block_access not checking the value of the $operation argument.

#2124611: Block operations links don't show in Block Layout page when using Overlay.

#2124599: node_block_access affects access to editing blocks Removes "operations" links for Block layout page.

If seems both these hooks should only be affect when $operation == 'view'. They were so they were affecting the "operations" links on the Block Layout page.

I provided patches in both issues that need review.

tedbow’s picture

Issue summary:View changes

clarifying according to alex in #202

xjm’s picture

Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release

The patch for this issue was committed on May 11, 2013. This means that the change record updates from this issue have been outstanding for more than eight months.

chx’s picture

Title:Change notice: Remove the tight coupling between Block Plugins and Block Entities» Remove the tight coupling between Block Plugins and Block Entities
Issue tags:-Needs change record, -Missing change record

I have rewritten the big change notice https://drupal.org/node/1927608/edit and included this in there.

chx’s picture

Status:Active» Fixed

Status:Fixed» Closed (fixed)

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