Convert all blocks into plugins

Yay. Thanks so much for posting this! A lot of great progress so far. I'm sure I'll have more feedback as I digest all of this more, but here's what initially jumped out at me.

+++ b/core/includes/bootstrap.inc
@@ -1,6 +1,10 @@
+use Drupal\Core\Utility\SchemaCache;

Is this just a merge artifact, or is this somehow related to blocks?

+++ b/core/includes/module.inc
@@ -22,8 +22,17 @@ function module_load_all($bootstrap = FALSE) {
+    // Retrieve the Drupal ClassLoader to add all the module namespaces.
+    $loader = drupal_classloader();
+
     foreach (module_list(TRUE, $bootstrap) as $module) {
       drupal_load('module', $module);
+
+      // Register the module's namespace to the module's lib path.
+      $path = dirname(drupal_get_filename('module', $module));
+      $loader->registerNamespaces(array(
+        "Drupal\\$module" => DRUPAL_ROOT . '/' . $path . '/lib',
+      ));

Can this be removed now that #1467126: PSR-0 namespace auto registration for modules landed?

+++ b/core/lib/Drupal/Core/Config/DrupalConfigVerifiedStorage.php
@@ -27,6 +27,13 @@ abstract class DrupalConfigVerifiedStorage implements DrupalConfigVerifiedStorag
+   * Implements DrupalConfigVerifiedStorageInterface::getName().
+   */
+  function getName() {
+    return $this->name;
+  }
+

Here and elsewhere in Drupal\Core\Config: is this addition needed? I don't see what in the rest of the patch uses this.

+++ b/core/modules/block/block.admin.inc
@@ -182,30 +177,13 @@ function block_admin_display_form($form, &$form_state, $blocks, $theme, $block_r
-      db_update('block')
-        ->fields(array(
-          'status' => $block['status'],
-          'weight' => $block['weight'],
-          'region' => $block['region'],
-        ))
...
+    $config = config($block['config_id']);
+    $config->set('weight', $block['weight']);
+    $config->set('region', $block['region']);
+    $config->save();

What happens to 'status' in the new architecture?

+++ b/core/modules/block/block.module
@@ -894,45 +780,9 @@ function block_block_list_alter(&$blocks) {
-    // Allow modules to modify the block before it is viewed, via either
-    // hook_block_view_alter() or hook_block_view_MODULE_DELTA_alter().
-    drupal_alter(array('block_view', "block_view_{$block->module}_{$block->delta}"), $array, $block);

Can we keep hook_block_view_alter(), and just change module/delta to id, rather than removing the hook entirely?

+++ b/core/modules/block/block.module
@@ -894,45 +780,9 @@ function block_block_list_alter(&$blocks) {
+    $element += $block->build();

Can we call this view()? See #658364: Does build/view/formatter terminology make sense?.

+++ b/core/modules/aggregator/config/plugin.definition.core.block.aggregatorcategory.xml
@@ -0,0 +1,13 @@
+<config>
+  <title>Aggregator category</title>
+  <module>aggregator</module>
+  <block_class>Drupal\aggregator\CategoryBlock</block_class>
+  <derivatives>TRUE</derivatives>
+  <settings>
+    <subject>Aggregator category</subject>
+    <region>sidebar_second</region>
+    <block_count>10</block_count>
+    <status>TRUE</status>
+  </settings>
+</config>

Also, I'm wondering if this is combining plugin definition and plugin instance settings into 1 config file, and if we really want that.

Thanks for the answers. I just have a couple follow-ups:

We'd probably need to pass the UUID for the block through so hook_block_view_$uuid_alter for the specific

The cool thing about drupal_alter() is we can do multiple specifics. For example,

drupal_alter(array('block_view', "block_view_{$base_plugin_id}", "block_view_{$derived_plugin_id}", "block_view_{$instance_uuid}"), $array, $block);

All in all I'm ok with it if we think it's necessary, but a lot of this stuff is one-off configurable instance sort of information, so I'm not sure why you'd want to change that through code when you have access to change it through UI already.

The one use-case we have in core is http://api.drupal.org/api/drupal/core!modules!menu!menu.module/function/menu_block_view_alter/8">menu_block_view_alter(), though there's others in contrib. This is menu.module enhancing system menu blocks, and is a good use-case for altering based on $base_plugin_id. True, this use-case could also be implemented by menu.module defining its own block plugin that inherits from system menu's plugin, so I'll try to dig up some use-cases where alter would clearly be better than inheritance.

This isn't mixing instance and definition configuration, this is providing default instance configuration within the plugin

Ah. I think I got tripped up by <region> but you address that in the issue summary. Yeah, this default instance settings makes sense at least until CMI solves it more generically, and maybe even after that.

I'll try to dig up some use-cases where alter would clearly be better than inheritance

One reason for menu.module to use alter of the system menu blocks instead of defining its own block plugin for the system menus: if it did the latter, and you wanted contextual links, you'd have to remove the system block instances from your layout, and add the menu block versions. Then, suppose you disable menu.module. Now you have to go and re-add the system blocks. Not a huge deal, but still shows the desirability of enhancing existing blocks via alter().

Clarifying title according to #1696302: [meta] Blocks/Layouts everywhere, bumping priority, and tagging for architectural review for @drupalchanges.

I know you're working on this patch for a long time. But to be honest I didn't knew it was this nice alrdy.
I'm just going to say *everything* I noticed.

1) All the blocks are gone after applying the patch. And update.php whitescreens.
2) The block library is *awesome*. The gui is smooth and clean and easy...

3) Added only an aggregator category no feeds attached. I'm also wondering why I see the same errors multiple times. Maybe the results are not cached?

Notice: Undefined property: Drupal\aggregator\Plugin\Derivative\CategoryBlock::$config in Drupal\aggregator\Plugin\Derivative\CategoryBlock->getDerivativeDefinitions() (line 30 of core\modules\aggregator\lib\Drupal\aggregator\Plugin\Derivative\CategoryBlock.php).
Notice: Undefined property: Drupal\aggregator\Plugin\Derivative\CategoryBlock::$config in Drupal\aggregator\Plugin\Derivative\CategoryBlock->getDerivativeDefinitions() (line 30 of core\modules\aggregator\lib\Drupal\aggregator\Plugin\Derivative\CategoryBlock.php).

Notice: Undefined index: module in block_library_block_form() (line 317 of core\modules\block\block.admin.inc).
Notice: Undefined index: module in block_library_block_form() (line 317 of core\modules\block\block.admin.inc).

(haven't reviewed anything yet, just changing status).

Here's an additional patch (interdiff with #18) that updates testBlock in BlockTest.php.

I switched from using the Navigation menu block to using the Powered by Drupal block in testBlock, because the appearance of the Navigation menu relies on having access to a menu item there, which seemed like unnecessary extra complexity.

testBlockRehash in BlockTest.php is also in the patch but is not complete (see TODO).

All other tests in BlockTest.php cannot be fixed yet as they relate to adding custom blocks and block visibility (not yet implemented).

Noticed a stray line in the last patch. Updated patch, better filename.

FYI: _block_get_renderable_region function needs attention as it still uses module, delta

New patch, now covers BlockCacheTest and BlockHtmlIdTest as well.

testBlockRehash and testBlockInInvalidRegion cite the concept of 'block rehashing' but I notice _block_rehash is gone, so maybe we don't need to test it anymore, or...

Still trying to decide if we should get conditions working on this patch or just get block visibility working and delete it later. Thoughts welcome.

My general inclination with large patches is to introduce as few new concepts as possible. Many concepts + many lines of code = patch sits in the queue with insufficient reviews for too long. So I'm +1 to punting conditions to a separate follow-up issue.

Here's a custom block module to provide a custom block plugin.

Let me know how this is looking and I'll move on to fixing all the block tests that need custom blocks.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
@@ -0,0 +1,104 @@
+ *    "body" = "",
+ *    "format" = "",
+ *  }
+ * )
+ */

No trailing commas in annotations :(

This makes sense to me.

New patch for tim.

On the patch in #31

namespace Drupal\block\Plugins\Type;
namespace Drupal\aggregator\Plugin\block\block;

I would think both would be Plugin (singular).

Merged in HEAD. (layout-blocks-1535868-sun, based on 8.x-blocks)

interdiff wasn't possible, bailed out with errors. However, I did not perform any other changes than merging in HEAD.

- #1786990: [Module]Bundle is registered too late in WebTestBase::setUp() by sun: Stop-gap fix for unavailable module bundles in web tests.

Eager to help on this patch. The issue is difficult to follow but it seems you're close to implementing a conversion example that all of the core blocks should follow. Are we at or past the point where you need an army of folks to go through core and convert all the blocks to the new way of implementing things?

Would it be possible to put just one or two conversions in this patch, add a bc layer, then leave the individual block conversions to follow-up issues. I'm concerned that there are #54 comments here, a 219kb patch, and only a couple of reviews since April.

An API changes-only patch for review purposes sounds good yeah.

#51: annoblocks.patch queued for re-testing.

Retesting because some meat-y things landed recently.

@EclipseGc regarding your point #5 (CMI) . Is the work needed to convert this to CMI apart of this issue or a separate one?

I'm happy to help with that. I'll see if I can find some time to work on tests.

What I'm most worried about here, from a moving this through the core process standpoint, is that this both changes the API of blocks and the block admin UI. I haven't looked at the patch yet to see how easy/hard it would be to split off all UI changes into a separate issue, but if that's at all possible, I would strongly encourage that. It would help getting the API part of this issue to green, and also not getting bogged down in a 200 comment discussion about UI (we'd still need to have that discussion, but could do so in parallel with using the new API elsewhere in core).

dropping a note to say i'm pulling this in (at least in part) to facilitate the base controllers work i'm doing.

Clearly this is not a issue about changing the UI, in that perspective it would make more sense to split it off. Since we cannot give good consideration to the UI at this point.

However given that it was clear from the start that this issue is only an intermediate step, and the proposed UI is close to agreed upon in the blocks & layout discussion - its not necessarily a bad first step. I am at a loss how to move any of this blocks & layout stuff forward.

Merged HEAD.

Patch in #69 didn't apply. Here is a reroll. Am I right in thinking that everything is in the patch from #69 or is some of it split out?

bot?

#430886: Make all blocks fieldable entities closed as duplicate of this issue.

Tagging for additional test coverage; I know it's way premature at this point, but if blocks become entities in the course of this issue, we'll want to add blocks to all the various all-entity-type testing (including any I manage to add for #1811684: XSS: Bartik's node.tpl.php prone to XSS (prints $title) which is why I thought to look for this this morning). :)

Woops. My reroll in #70 only included some of the original changes. Here is a new one that does not trash about half the code.

I have not been able to review the patch yet. But could you guys please make sure that a Save, Insert and Update will fire a hook that every module can hook into the process and grab the content inside for analysis e.g. linkchecker link extraction?

Given Bojhan's comment in #66, I retract mine in #63. The key UI change in this patch (not having disabled blocks immediately visible for dragging on the admin/structure/blocks page, but instead, having to click "Add block" and then select a block from the (new concept introduced by this patch) "Block library")) is logically part of the scope of this issue, because a blocks as plugins architecture introduces a distinction between a "block" and a "block instance", and it makes sense for the UI to reflect that architectural change. If we're willing to accept this interim UI change without bikeshedding it until follow up issues that are specifically focused on UI refinement, then I'm okay with us proceeding with this full patch in this issue.

Can we get screenshots, and an updated issue summary? This patch keeps growing and growing and growing and...

if blocks become entities in the course of this issue

As I understand it, block instances in general, are configuration, so maybe at some point, will become ConfigEntities, but not fieldable entities. #48 indicates a desire to make *custom* blocks into entities, and this I agree with in principle, but not as part of this issue. That's not yet done in #78, and I'm concerned about adding that to the scope of this issue. I'd recommend spinning that off into a separate issue that's postponed on this one, or whose patches are rolled against this one until this issue is committed.

The block config UI gave me all kinds of whack errors so I could not configure blocks at all.

I found two call by reference function calls and removed the &. That fixed it. I have ~E_DEPRECATED on in my php.ini and I think everything went wonky over that with those calls.

I have been going through the tests in the obvious "Blocks" category for now. A lot of them depend on the custom blocks module, so I have been skipping them for now because that still needs some work.

I have fixed "Blocks -> Administration theme test" so far. Next on the list is the comment block test.

I have not seen ":" in urls before so unless it in unintended in the admin UI I learned something new today. The updated test uses them and it works.

There were other tests that I looked at that failed on db-insertion because the "pages" field in the block table does not have a default value. I added a default of "" to that in this patch too.

Interdiff is against the patch in #78 and so is the actual patch. Let me know if it is easier with another workflow.

The patch still fails a slew of tests so no reason to set it to needs review unless someone feels like that is destroying a workflow. I just feel bad for the bot.

Ignore the part about the database in #86.

Mysql explicitly manual says: BLOB and TEXT columns cannot have DEFAULT values. I chose to read that too late :)

Here is a little more test progress.

I made CommentBlockTest work.

I *almost* made AggregatorRenderingTest work, but it throws an exception complaining about custom_blocks table not existing. I have no idea what causes that, but I suspect that it should not be fixed in the test. So I'll leave it as is for now.

The full patch is kept up with 8.x and the interdiff is an unscientific approach to letting people see what I am altering. It is probably not precise because I just diff my local commits. Sorry for the laziness.

For the aggregator tests I had to fix some small errors in the actual block plugins. If someone could take a look at those fixes in the interdiff and validate that I am not making things up it would be great.

I am running into a lot of problems with block plugins (the derivative ones) being loaded when running tests - even if the module is not enabled. I tried the ModuleDecorator from timplunketts patch in #1763974: Convert entity type info into plugins but the DerivativeInterface::getDerivativeDefinitions() gets run no matter what.

Some of the plugins run sql queries in getDerivativeDefinitions() so I get exceptions from the db.

I'm not really sure how to proceed. Does anyone have an idea how to be able to run tests without the definitions getting loaded?

If someone wants to test this, then run the "Block caching" test to get an exception from the aggregator module.

I like where this is going. Needs some performance testing, imo.

Still having trouble running tests at all.

I now copied the idea from ModuleDecorator to the already existing DerivativeDiscoveryDecorator

I tried adding the lines checking if the module is enabled to getDerivativeFetcher(). It does solve my problem but creates new ones down the line in DefaultFactorys getPluginClass() not finding the class of the plugin and throwing an exception.

  protected function getDerivativeFetcher($base_plugin_id, array $base_definition) {
    if (!isset($this->derivativeFetchers[$base_plugin_id])) {
      $this->derivativeFetchers[$base_plugin_id] = FALSE;
      if (isset($base_definition['derivative'])) {
        // Plugins don't have to belong to a module, but if it does - check if
        // it is enabled.
        if (!isset($base_definition['module']) || (isset($base_definition['module']) && module_exists($base_definition['module']))) {
          $class = $base_definition['derivative'];
          $this->derivativeFetchers[$base_plugin_id] = new $class($base_plugin_id);
        }
      }
    }
    return $this->derivativeFetchers[$base_plugin_id] ?: NULL;
  }

This is only reproducible in simpletest, so the loading of plugins must be different there.

The loading of plugins can't really be different at any point. The problem with simpletest seems to be that the annotation loader looks at the class loader to find registered namespaces. This is totally reasonable and the right way to handle it but in simpletest its to aggregate namespaces from other tests and the parent site stuff because we're running tests in userspace. This is just a shortcoming of the way simpletest works. We shouldn't special case code to work around simpletest so we'll have to work with some of the simpletest guys to work out how to solve this.

Here is some more test-progress. Patch is kept up with 8.x (post views commit). Interdiff is still un-scientific.

I got these tests passing:

./core/modules/block/lib/Drupal/block/Tests/BlockAdminThemeTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.php

I can still only create blocks in tests via the UI - I run into the loader problem otherwise. That is not a bad thing per se, but test could probably be sped up some by just injecting blocks. We can always come around to that.

Note that some tests throw exceptions that will have to be fixed in the testing layer. And I totally agree that it would be the place to fix it.

CNR for bot to see what tests are still failing.

Some more progress. I added some module_exists checks in derivatives see #1821658: Plugin derivatives calling disabled modules in tests - that made it much easier to move forward.

These tests pass now:

./core/modules/block/lib/Drupal/block/Tests/BlockAdminThemeTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockTest.php
./core/modules/block/lib/Drupal/block/Tests/NonDefaultBlockAdminTest.php
./core/modules/block/lib/Drupal/block/Tests/BlockCacheTest.php
./core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.php

About BlockInvalidRegionTest.php: There used to be a warning when a block was assigned to an invalid region. It is not there anymore, so the test doesn't really make sense. But should we still have a warning for that?

I am posting an interdiff for now - I got a merge conflict from 8.x just before bedtime. I'll see if I can clean that up tomorrow. Interdiff is against #86

Here is a patch that is merged with 8.x

There was a conflict in user.module, but it was pretty straightforward to resolve - it was changes to stuff that is being removed by this patch. If someone could verify that by taking a look at the patch it would be great.

More updated tests to come tomorrow.

Some more tests done:

./core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.php
./core/modules/node/lib/Drupal/node/Tests/NodeBlockTest.php
./core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.php

I got another conflict merging in 8.x that I couldn't resolve as easily as the one from #100. If someone could have a go at a merge with the patch in #100 and 8.x it would be great. The conflict in the tests (there is one file in tests that conflicts) should just be overwritten by the tests from this patch.

I can upload more patch progress once I can sync up with 8.x again.

I *think* this does the trick.

#594660: Rename default menu names prefixed all menu blocks with 'menu-'.

Some more progress. Patch will still fail miserably, but I am getting the number of failing tests down. Marking it for the bot so I can see what I need to work on.

Patch is kept up with 8.x. I had anther merge conflict but it was pretty easy to resolve. It was in bootstrap.inc and came from this issue: #1809206: KeyValueFactory hard-codes DatabaseStorage

I had to change BookNavigationBlock.php to avoid an error from common.inc: "@key" is an invalid render array key. I don't like the code in my change there so much, but I made it work. It might need some clarification with comments.

I have a list of stuff that is waiting for descisions or me figuring out how to do it. Here is what I have so far:

./core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.php

Eclipse is re-writing parts of the form in user login: #1826452: Do not alter forms after calling drupal_get_form()

./core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.php

I can't make test work without WebTestBase right now. This is a unit test and could just be changed, but I want to see if there is a way to mock blocks first.

./core/modules/block/lib/Drupal/block/Tests/BlockUserAccountSettingsTest.php

Personalized blocks are going away, so I guess this test should be deleted?

./core/modules/block/lib/Drupal/block/Tests/NewDefaultThemeBlocksTest.php

Default blocks are not done yet.

./core/modules/block/lib/Drupal/block/Tests/BlockInvalidRegionTest.php

There used to be a warning when a block was assigned to an invalid region. It is not there anymore, so the test doesn't really make sense. But should we still have a warning for that?

./core/modules/filter/lib/Drupal/filter/Tests/FilterHooksTest.php

Uses custom block which is not done yet. (but does not need to)

there's no way i'll be able to penetrate the forest of block reimplementations here. wish i could, but i just can't. so i'll focus on the core architecture, particularly with an eye towards the stuff this is blocking.

when i say core architecture, i mean the family of API-level functionality around the new plugin type that's being created. that's because this patch is tricky: while it's an improvement on its own, it's REALLY worthwhile in the context of the panels-in-core-ish, blocks-and-layouts significant overhaul we're doing. in fact, it's a prerequisite for all of that. it's my hope that if we can just get this patch to the point of not breaking everything for everyone else, then we can get it in quickly so that the other patches blocked on it (#1812720: Implement the new panels-ish controller [it's a good purple] and to a greater or lesser extent #1817044: Implement Display, a type of config for use by layouts, et. all and #1787942: Allow assigning layouts to pages) can roll forward. we're losing valuable time by having to take half-measures in these patches.

architecture review

BlockInterface::accessWrapper() and BlockInterface::access()

these methods represent a reasonable approach to block visibility, for the old paradigm: since the block's visibility settings are stored as part of its own internal configuration, it only makes sense that we attach methods directly to the block classes in order to perform those checks. moreover, the separation between BlockInterface::accessWrapper(), where general block visibility settings are checked (e.g., path or role-based visibility), vs. BlockInterface::access(), which allows each block plugin to also define its own access criteria (and both must pass in order for the block to be visible), is fairly solid - though i have concerns. more on that below.

however, at least BlockInterface::accessWrapper() is destined for the chopping block. when we switch over to a panelsy renderer, a) this data is no longer stored on the block itself, but on the Display, b) we lose the contextual assumptions that allow most of these generic contracts to make sense (since we're ON a display, not in giant globaldom) or at least c) it makes no sense to hardcode them, and we should instead handle that case with condition plugins.

BlockInterface::access() is not something we really have in Panels right now, but is probably a good addition. in particular, it is useful because of the thinking we need to do around making blocks addressable: because the access check is tightly coupled with the particular block plugin itself, we know that (at least) this check should be applied to the block's addressable route. it's not clear in my mind whether or not we also would want to include the optional, user-selected instance-specific logic accommodated by BlockInterface::accessWrapper(), but having the tightly coupled access() call gets us a long way.

then, there's the set of six methods for configuration form (configure[Wrapper], configureSubmit[Wrapper], configureValidate[Wrapper]). here i start to have a concern about the paired main + wrapper callback. pedantry first: since we're expecting object instances to call the non-wrapper methods from the wrapper methods, the non-wrappers could/should be protected. of course, if they were, then they couldn't be defined on the interface. this points to a basic problem with the interface: it's written with the base class in mind. if we're going to take this appraoch, we should ditch the wrapper methods and have abstract protected methods declared by the BlockBase for its children to implement.

the last method is BlockInterface::build(), which is responsible for returning block output as a renderable array. this works fine in the current paradigm, but is really the place where it's clearest that this code is intended to be transitional. without a method that returns a rendered string, blocks can't take their place as first-class controllers in the new routing system. and really, that method should return a symfony Response object, which should also be loaded up with out-of-band data (css, js, etc.) and pass it along that way. in effect, the Response is stepping in for renderable arrays. it'll be better-documented, but slightly less performant. hopefully negligibly so.

note - we could retain the build() method at least transitionally as the place where renderable arrays are produced, but add a render() method on BlockInterface that simply translates the renderable array into a Response.

note 2 - given that we are necessarily exploring blocks becoming less...i dunno, blocky, we might consider IN THE FUTURE reducing the scope of this interface so that it better accommodates some of the less traditional blocks - for example, the 'main content' block.

going forward

this patch basically has four parts. i've included a rough % indicating how much of each part is useful in the long term for the new blocks-and-layouts rendering model:

• the new block API/architecture itself (70%, but 95%+ if we refactor the BlockInterface and BlockBase per my suggestions)
• reimplementation of all core blocks into the new block architecture (95% or more)
• the new blocks admin UI (30%)
• the glue code that sticks all the blocks into the new places (0%)

this patch is stuck in a catch-22. other patches need it in order to fully implement the new rendering model, but this patch needs the new rendering model in order for its architectural approach to fully make sense. this is a big reason why it's dragging, apart from test conversion just being a slog. (kudos, @naxoc!)

what would be great is to set a "good enough" goalpost. nobody is motivated to burn time working on an interface that will be thrown out; we need it to be OK that the interface blows, but that the API is mostly there. that may even entail deleting some tests, then recreating them later in the new system.

@naxoc - could you please also include interdiffs?

i've added some improved docblocks to BlockInterface. not all the methods, but some. i actually think they help highlight some of my complaints in #108 :)

intentionally leaving on needs work as this was a docs-only change.

Rerolled for HEAD.

I added to the standard and testing profile config that we should have our usual default blocks displaying (search, help, login, powered by drupal, content) in bartik but not in seven.

This removes the default blocks from the testing profile (we just need them in standard profile), fixes a path in an upgrade test, and fixes the ID in user_new_block.

Fixes for Drupal\system\Tests\System\AccessDeniedTest to configure blocks properly and not rely on the block title of the User Login block.

Fixes for Drupal\search\Tests\SearchConfigSettingsFormTest to configure block properly.

Fixes the Drupal\block\Tests\BlockTemplateSuggestionsUnitTest.

this involved some changes to the way we generate theme hook suggestions for blocks. because it's possible for us to have multiple layers of derivatives (e.g., :::), we now generate incremental suggestions for EACH level, translating : into __.

This removes BlockUserAccountSettingsTest. Support for this in the code was removed long ago (nobody really uses this...come on). and it's unlikely to translate well into the new rendering paradigm, anyway.

I had to make a lot of changes in openid because we no longer have block_view_alter.

Definitely needs work on this one to clean it up.

Removing unnecessary settings method call.

last patch is 0 bytes

Patches in 119 and 120 are incomplete, so this is #118 plus 119 and 120 interdiffs plus an annotation change on BlockPluginUI to set menu=FALSE, because as TRUE, it was leading to many of the remaining test failures.

This gets rid of the change in 119. We should just add in the block_view_alter functionality instead of working around it being missing.

Eclipse and I found that the change in #122 to menu=FALSE broke the library page.

This patch remove that. We're going to try to fix those tests by enabling block module for them (via sdboyer)

this patch also fixes an aggregator test by removing an unnecessary cache clear and updates a translation test to the new block system.

This fixes the invalid region block tests by implementing handling of blocks in invalid regions in _block_rehash as it worked historically.

To fix BlockCache tests, I had to add a dependency on views to prevent a fatal error on cache clear. Will document in issue summary.

link filter to issue

Keeping list of modules we had to enable within tests to get passing tests.

This is what Sam tried to post in #117 to fix Drupal\block\Tests\BlockTemplateSuggestionsUnitTest

Fix for the Format Filter Hooks test.

Fix for Drupal\forum\Tests\ForumNodeAccessTest chasing an upstream change I believe re node access.

adding comment number

another test had to have views enabled

another module enabling in a test

Fixed Drupal\image\Tests\ImageFieldDefaultImagesTest by enabling block in the test. Noted in the issue description and in TODO.

Fixes the views wizard basic functionality test.

Fixing views wizard number of items test

Another module had to be enabled to pass tests

Fixed Drupal\image\Tests\ImageFieldDisplayTest by enabling block in the test. Noted in the issue description and in TODO.

Fixed overridden displays tests.

Added more dependencies on block and views modules to fix tests.

Fixed the menu block building.

Added another block module enabling to a test

Combo of 142 and 143 since we cross-posted.

More dependencies on block/views to track

More tests being 'fixed' by enabling block module in tests.

had to chase 8.x after changes went in for #1823348: Convert user_block variables to cmi. so, interdiff not really possible.

hopefully this will fix MenuTest's 49 fails. it's not behaving in testing on my local, so i really can't tell :(

Borrowing a trick we use in Views and the Entity API to work around #1780396: Namespaces of disabled modules are registered during test runs, in an attempt to fix StatisticsReportsTest

That fix needs to be specific to the block system.

Should fix
MenuNodeTest
BlockUpgradePathTest
UserAccountLinksTests

Fixed TrailTest and TranslationTest

Oh I rolled this last night but I must have forgotten to upload it.
Fixes UserBlockTests and part of MenuTest.

This patch:

Removes the custom derivative decorator
Adds a workaround for #1780396: Namespaces of disabled modules are registered during test runs to DerivativeDiscoveryDecorator that works for all plugins
Provides the block's subject from annotation as the default for the configure form
Cleans up some of the BlockPluginUI code
Removes the unnecessary addition of views.module to tests
Fixes the Statistics test
Removes a hack added to the UserLoginBlock

LanguageUpgradePathTest and FilledStandardUpgradePathTest fail because placement of blocks is not migrated during the upgrade

UserRoleUpgradePathTest fails because visibility doesn't do anything any more :)

ModulesDisabledUpgradePathTest fails because enabling custom_block tries to create the {block_custom} table while the old D7 block module still provides it.

Whoops! I guess that fix for default subjects was a bit too far reaching.

While I was debugging it, I also noticed a easy gain in moving the call to block_manager() out of the foreach loop.

Per #81, adding some screenshots. I'm not inlining them into the comment body, because they're large, but am about to see if I can inline them into the issue summary at a reasonable size.

More tests 'fixed' by enabling block

Updated issue summary.

I updated the issue summary. Please update further as needed.

The Tools menu should be available to anonymous users, but I'm not quite sure how to accomplish that with derivatives.

Also, removing all of the hacks listed in the issue summary, once it passes I'll update that.

Updated issue summary.

ModulesDisabledUpgradePathTest fails because enabling custom_block tries to create the {block_custom} table while the old D7 block module still provides it.

This means that block.module needs an update function that enables custom_block using update_module_enable(). That function does not install the schema and the then we have a valid state again. Users who don't want custom blocks are then free to disable/uninstall the module.

One small suggestion after reviewing the screen shots: the local action should include a verb. I found "Block Library" confusing at first glance. How about "Add block from library"?

Rerolled for core changes.