Issue #1535868 by EclipseGc, tim.plunkett, xjm, Jody Lynn, sdboyer, naxoc, tizzo, effulgentsia, dawehner, disasm, beejeebus: Convert all blocks into plugins.

Problem/Motivation

This is the foundational patch for #1696302: [meta] Blocks/Layouts everywhere.

Proposed resolution

The sandbox:
http://drupalcode.org/sandbox/eclipsegc/1441840.git/shortlog/refs/heads/...
This issue:

  1. Converts Drupal's Block architecture to be plugin-based.
  2. Enables blocks to be instantiated multiple times in the same theme (i.e., you can add a "Who's Online" block configured to show who's been active the last 5 minutes to your left sidebar, and a "Who's Online" block configured to show who's been active the last 60 minutes to your right sidebar). This will become more useful when we add more useful blocks to the system: i.e., a menu block that can replace both the 'primary links' and 'secondary links' currently hard-coded into page.tpl.php.
  3. Converts all core blocks to plugins (specifically, to annotated classes implementing the new BlockInterface that is part of this patch).
  4. Converts all core block configuration to CMI (no more {block} table).
  5. Adds a "Block Library" UI to the admin/structure/block workflow to incorporate the concept of configured block instances being separate from bare blocks. This UI is very rough and will be improved in follow up issues.

Remaining tasks

The patch for this issue was committed In January. Two change notifications have been filed. At least one additional change notification is still needed. From #406:

@xjm: tried to look into doing a change notice for "change in block template names" but the only new/changed tpl.php I found in the patch was "system-plugin-ui-form.tpl.php" which is a new admin template, not really relevant for blocks themselves. I don't think that is worth a change notice. What did I miss?

What I was alluding to is not changes in the names of templates themselves but in 1. render array keys and 2. variable name changes in theme preprocesses. In my reviews above, I commented on this for three places:

+++ b/core/modules/node/node.moduleundefined
@@ -1082,11 +1082,11 @@ function node_is_page(Node $node) {
-    switch ($variables['block']->delta) {
-      case 'syndicate':
+    switch ($variables['block']->id) {
+      case 'node_syndicate_block':
...
-      case 'recent':
+      case 'node_recent_block':

So this is at least the second time I've seen this, that it's switching from delta to ID. I know that the delta isn't going away from elsewhere in the patch, or at least it still appears to still be there for all intents and purposes. (Or intensive porpoises.) What's the deal? Presumably this will become clear once I read the new architecture. There's also some API change-y stuff going on here that we should make sure gets documented in a change notification. (Presumably this issue will have more than one, since this change is very themer-facing but themers don't care about the stuff we're doing to the base API.)

+++ b/core/modules/openid/openid.moduleundefined
@@ -130,20 +130,18 @@ function openid_user_logout($account) {
-function openid_block_view_user_login_alter(&$block) {
+function openid_block_view_user_login_block_alter(&$build, $block) {

@@ -152,10 +150,10 @@ function openid_block_view_user_login_alter(&$block) {
-  $block['content']['user_links']['#weight'] = 10;
+  $build['user_links']['#weight'] = 10;

Okay, so clearly an API change here that I hope I find to be documented when I get to block.api.php and the issue summary.

+++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
@@ -78,7 +78,7 @@ public function execute() {
-    $info['subject'] = filter_xss_admin($this->view->getTitle());
+    $info['#title'] = filter_xss_admin($this->view->getTitle());

This hints at another API change.

So, what I think is still needed is a change notice with an example of altering or theming a block to be rendered in D7, and how it changes in D8.

Additionally, the over 50 followups from this issue have not been resolved. Many of the followup issues for this issue were tagged with If SCOTCH fails, including one release blocker: #1875252: [META] Make the block plugin UI shippable. On Feb. 18, when Blocks and Layouts was feature-complete for whatever would be in Drupal 8, these issues were to be unpostponed, and t be the initiative's focus for the next phase of the release cycle. (See below.) This did not happen as agreed, and the issues are still outstanding.

See comment #294 for a review of the full patch. See #108 for an earlier review of the interface's architecture. For the core maintainers' feedback on this patch, see #307.

Followup issues

Also see the initiative roadmap at #1696302: [meta] Blocks/Layouts everywhere.

  1. Bugs and regressions introduced by this patch

  2. The architecture needs review.

    Specific points of concern follow. In each case, we should determine whether it is appropriate to explore the question in a followup, or whether it needs to be addressed more before this patch is committed.

  3. The new classes and interfaces introduced by the patch need detailed documentation.

    The patch includes numerous @todos where more documentation should be added. See #297 for a complete list. Followup issues:

  4. There are gaps in the test coverage

    See #294 and #337 for additional details.

  5. There is no upgrade path.

    #1871700: Provide an upgrade path to the new Block architecture from Drupal 7
    (see also #1887688: locale_update_8001() references unused block tables)

  6. The patch has not been fully benchmarked.

    Commit blocker. File followups as appropriate. See #338.

    The following issues will likely be important for making the new architecture more performant:

  7. The user interface will need review and testing (as a followup).

    The actual user interface will probably need user testing and refinement at some point. It probably does not make sense to do this yet, since one of the goals of the Blocks and Layouts initiative is to create a streamlined workflow for placing blocks in layouts.

    1. #1841584: Add and configure master displays
    2. #1871746: Add modal block browser
    3. #1840500: Add simple blank page creation capability
    4. Add context dependent blocks/pages
    Other usability issues
  8. Other cleanups and followups.

    Others TBD. I'll add in the non-critical tasks and followups later. All the things mentioned under "Followups" in #246 on, plus the fix needed to hook_schema(), config and plugin names/IDs/deltas, etc.

Other related issues

User interface changes

Instead of all blocks being listed on admin/structure/block, only configured instances are listed.

There's a new "Block Library" link to add additional block instances. (The text of this link may be changed in #1874584: "Block Library" link label is unclear.)

This link takes you to a browser that allows you to pick which block you want to add, with an additional link to add a new custom block if the Custom Block module is enabled. (See also: #1874982: CSS bugs in the Block Library UI, #1875780: "Configure block" button text in the Block Library is confusing, #1875058: Provide separate interface for editing custom blocks.)

When you edit a new instance, you can give the block a custom title, or use the default title. You also give this instance a specific machine name (though we might generate a default machine name automatically, see #1875260: Make the block title required and allow it to be hidden and #1875016: Automatically generate machine names for block plugins).

You can also adjust the configuration of existing block instances as you previously would, by clicking the "configure" operation from the block administration page.


API changes

TODO: Fill in this section. See #281.

Blood Oath

We, the undersigned, do hereby avow that we will work on If SCOTCH fails issues after February 18:

EclipseGc
sdboyer

Files: 
CommentFileSizeAuthor
#376 block_library_action.png18.66 KBxjm
#376 block_library.png34.16 KBxjm
#376 existing_instance.png22.98 KBxjm
#376 new_instance.png24.61 KBxjm
#375 block_plugins_80_runs.png42.23 KBmsonnabaum
#375 1.blockplugins.xhprof.gz63.48 KBmsonnabaum
#375 2.blockplugins.xhprof.gz61.82 KBmsonnabaum
#373 block-1535868-372.patch441.58 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,467 pass(es).
[ View ]
#373 interdiff-372.txt7.45 KBxjm
#372 block-1535868-372.patch442.16 KBbeejeebus
PASSED: [[SimpleTest]]: [MySQL] 50,482 pass(es).
[ View ]
#372 interdiff.txt6.34 KBbeejeebus
#370 block-1535868-370.patch444.97 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,077 pass(es), 19 fail(s), and 6 exception(s).
[ View ]
#370 interdiff-370.txt9.72 KBxjm
#367 block-1535868-367.patch440.01 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,484 pass(es).
[ View ]
#367 interdiff.txt921 bytesxjm
#365 block-1535868-364.patch440 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 50,050 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#365 interdiff-364.txt12.85 KBxjm
#361 block-1535868-361.patch442.53 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,501 pass(es).
[ View ]
#361 interdiff-361.txt3.59 KBxjm
#358 block-1535868-358.patch442.81 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,475 pass(es).
[ View ]
#358 interdiff-358.txt4.62 KBxjm
#358 add_block_before.png44.35 KBxjm
#358 block_add_after.png30.07 KBxjm
#356 block-1535868-356-fixed.patch443.33 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 50,448 pass(es).
[ View ]
#356 interdiff-356-fixed.patch30.35 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-356-fixed.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#356 before.png13.98 KBxjm
#356 after.png27.72 KBxjm
#343 block-1535868-343.patch428.39 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1535868-343.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#343 interdiff-343.txt10.57 KBxjm
#341 add_block_confusion.png9.25 KBxjm
#341 block_instance_creation_annoyances.png20.55 KBxjm
#341 no_titles.png12.81 KBxjm
#341 css_bugs.png28.04 KBxjm
#340 block-1535868-340.patch422.17 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,341 pass(es).
[ View ]
#340 interdiff-340.txt23.92 KBxjm
#335 block-1535868-335.patch421.12 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,363 pass(es).
[ View ]
#335 interdiff-335.txt50.97 KBxjm
#331 blocks-1535868-330.patch412.83 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 49,232 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#323 blocks-1535868-323.patch412.79 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,432 pass(es).
[ View ]
#323 interdiff.txt1.1 KBtim.plunkett
#320 blocks-1535868-320.patch412.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,170 pass(es), 1 fail(s), and 26 exception(s).
[ View ]
#318 blocks-1535868-318.patch412.91 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 35,192 pass(es), 1,243 fail(s), and 46 exception(s).
[ View ]
#317 blockinterface-do-not-test.patch35.42 KBEclipseGc
#316 block-1535868-316.patch419.72 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,315 pass(es).
[ View ]
#316 interdiff.txt17.55 KBxjm
#295 DiffBlocks.txt11.24 KBmbrett5062
#294 block-1535868-294.patch418.3 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,432 pass(es).
[ View ]
#294 block-interdiff-294.txt35.26 KBxjm
#291 block-1535868-291.patch410.97 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,401 pass(es).
[ View ]
#291 interdiff.txt664 bytestim.plunkett
#289 block-1535868-290.patch411 KBxjm
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es).
[ View ]
#289 interdiff.txt776 bytesxjm
#285 block-1535868-285.patch411 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#285 interdiff.txt812 bytesxjm
#283 block-1535868-283.patch411 KBxjm
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php.
[ View ]
#283 interdiff.txt17.21 KBxjm
#282 interdiff.txt7.16 KBtim.plunkett
#282 blocks-1535868-282.patch409.99 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,396 pass(es).
[ View ]
#276 block-1535868-275.patch404.16 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,388 pass(es), 5 fail(s), and 25 exception(s).
[ View ]
#274 merge-conflicts-du-jour.txt13.71 KBxjm
#273 block-1535868-273-merge-conflicts.txt1.06 KBxjm
#262 blocks-1535868-262.patch404.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 49,101 pass(es).
[ View ]
#262 interdiff.txt2.1 KBtim.plunkett
#260 blocks-1535868-260.patch402.51 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 49,100 pass(es), 0 fail(s), and 200 exception(s).
[ View ]
#260 interdiff.txt634 bytestim.plunkett
#252 scotch-1535868-252.patch402.51 KBxjm
FAILED: [[SimpleTest]]: [MySQL] 49,123 pass(es), 15 fail(s), and 0 exception(s).
[ View ]
#252 interdiff.txt18.18 KBxjm
#252 merge-conflicts.txt19.6 KBxjm
#252 merge-diff.txt3.55 KBxjm
#242 blocks-1535868-242.patch404.37 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,956 pass(es).
[ View ]
#239 blocks-1535868-239.patch404.58 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,629 pass(es), 81 fail(s), and 7 exception(s).
[ View ]
#239 interdiff.txt3.77 KBtim.plunkett
#238 blocks-1535868-238.patch403.46 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,999 pass(es).
[ View ]
#236 blocks-1535868-236.patch402.25 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,444 pass(es), 189 fail(s), and 21 exception(s).
[ View ]
#236 interdiff.txt15.39 KBtim.plunkett
#234 blocks-1535868-234.patch401.72 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,779 pass(es), 765 fail(s), and 63 exception(s).
[ View ]
#234 interdiff.txt15.83 KBtim.plunkett
#231 blocks-plugins.patch401.63 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-plugins.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#228 blocks-1535868-228.patch397.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,916 pass(es).
[ View ]
#227 blocks-1535868-226.patch397.65 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#226 block-1535868-226.patch397.86 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1535868-226.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#226 interdiff.txt707 bytesnaxoc
#224 block-1535868-224.patch397.86 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1535868-224.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#222 interdiff.txt13.06 KBtim.plunkett
#221 blocks-1535868-220.patch396.46 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 48,913 pass(es).
[ View ]
#219 blocks-1535868-219.patch400.92 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#217 blocks-1535868-217.patch396.38 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-217.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#217 interdiff.txt1.38 KBsdboyer
#215 blocks-1535868-215.patch395.73 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,475 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#213 interdiff.txt3.3 KBtim.plunkett
#213 blocks-1535868-213.patch395.75 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#211 blocks-1535868-211.patch394.09 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,370 pass(es), 24 fail(s), and 0 exception(s).
[ View ]
#211 interdiff.txt14.48 KBtim.plunkett
#209 blocks-1535868-209.patch389.87 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 48,084 pass(es), 24 fail(s), and 0 exception(s).
[ View ]
#209 interdiff.txt67.33 KBtim.plunkett
#207 blocks-1535868-207.patch383.57 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 47,972 pass(es).
[ View ]
#207 interdiff.txt315 bytestim.plunkett
#206 blocks-1535868-206.patch383.56 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 47,968 pass(es).
[ View ]
#206 interdiff.txt1.26 KBtim.plunkett
#197 blocks-1535868-197.patch384.68 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 47,926 pass(es).
[ View ]
#197 interdiff.txt12.08 KBtim.plunkett
#190 blocks-1535868-190.patch386.15 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#188 blocks-1535868-188.patch384.14 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,913 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#187 interdiff.txt904 bytesdawehner
#185 blocks-1535868-185.patch366.52 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,813 pass(es), 54 fail(s), and 14 exception(s).
[ View ]
#183 blocks-1535868-183-interdiff.txt2.35 KBtizzo
#181 blocks-1535868-180.patch356.69 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,440 pass(es), 254 fail(s), and 35 exception(s).
[ View ]
#181 interdiff.txt817 bytestim.plunkett
#179 blocks-1535868-179.patch355.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,432 pass(es), 258 fail(s), and 37 exception(s).
[ View ]
#177 blocks-1535868-177.patch356.37 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,377 pass(es), 258 fail(s), and 37 exception(s).
[ View ]
#172 blocks-1535868-172.patch356.12 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,232 pass(es), 258 fail(s), and 38 exception(s).
[ View ]
#172 interdiff.txt4.8 KBtim.plunkett
#170 blocks-1.png89.89 KBeffulgentsia
#170 blocks-2.png62.17 KBeffulgentsia
#170 blocks-3.png126.03 KBeffulgentsia
#170 blocks-4.png152.07 KBeffulgentsia
#168 blocks-1535868-168.patch360 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,226 pass(es), 261 fail(s), and 38 exception(s).
[ View ]
#168 interdiff.txt1.46 KBtim.plunkett
#165 blocks-1535868-165.patch359.75 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 45,685 pass(es), 1,899 fail(s), and 149 exception(s).
[ View ]
#165 interdiff.txt11.01 KBtim.plunkett
#163 blocks-1535868-163.patch360.36 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,227 pass(es), 261 fail(s), and 38 exception(s).
[ View ]
#163 interdiff.txt2.38 KBtim.plunkett
#161 blocks-1535868-161.patch359.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,211 pass(es), 278 fail(s), and 41 exception(s).
[ View ]
#161 interdiff.txt2.03 KBtim.plunkett
#159 interdiff.txt4.62 KBtim.plunkett
#159 blocks-1535868-159.patch358.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,203 pass(es), 312 fail(s), and 36 exception(s).
[ View ]
#157 blocks-1535868-156.patch354.28 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,174 pass(es), 342 fail(s), and 39 exception(s).
[ View ]
#157 interdiff.txt3.39 KBtim.plunkett
#155 blocks-1535868-155.patch354.11 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 47,153 pass(es), 349 fail(s), and 47 exception(s).
[ View ]
#155 interdiff.txt3.17 KBtim.plunkett
#151 blocks-1535868-151.patch352.29 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 47,154 pass(es), 345 fail(s), and 41 exception(s).
[ View ]
#151 interdiff.txt1.17 KBsdboyer
#148 blocks-1535868-148.patch351.12 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 47,141 pass(es), 344 fail(s), and 41 exception(s).
[ View ]
#146 blocks-1535868-146.patch349.53 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-146.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#146 interdiff.txt2.01 KBJody Lynn
#144 blocks-1535868-144.patch347.52 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,821 pass(es), 391 fail(s), and 44 exception(s).
[ View ]
#143 blocks-1535868-142.patch342.07 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,837 pass(es), 397 fail(s), and 41 exception(s).
[ View ]
#143 interdiff.txt2.62 KBJody Lynn
#142 blocks-1535868-142.patch346.71 KBtizzo
FAILED: [[SimpleTest]]: [MySQL] 46,810 pass(es), 398 fail(s), and 41 exception(s).
[ View ]
#142 interdiff.txt5.81 KBtizzo
#138 blocks-1535868-137.patch341.26 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,813 pass(es), 415 fail(s), and 41 exception(s).
[ View ]
#138 interdiff.txt678 bytesJody Lynn
#137 blocks-1535868-137.patch340.6 KBtizzo
FAILED: [[SimpleTest]]: [MySQL] 46,797 pass(es), 432 fail(s), and 47 exception(s).
[ View ]
#137 interdiff.txt1.56 KBtizzo
#136 interdiff.txt1.26 KBtizzo
#136 blocks-1535868-136.patch339.04 KBtizzo
FAILED: [[SimpleTest]]: [MySQL] 46,787 pass(es), 439 fail(s), and 46 exception(s).
[ View ]
#135 blocks-1535868-135.patch337.71 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,796 pass(es), 443 fail(s), and 46 exception(s).
[ View ]
#135 interdiff.txt701 bytesJody Lynn
#133 blocks-1535868-133.patch336.81 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,791 pass(es), 450 fail(s), and 46 exception(s).
[ View ]
#133 interdiff.txt2.56 KBJody Lynn
#131 blocks-1535868-131.patch336.09 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#131 interdiff.txt3.02 KBJody Lynn
#129 blocks-1535868-129.patch333.07 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#129 interdiff.txt5.68 KBJody Lynn
#128 blocks-1535868-128.patch329.66 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,694 pass(es), 471 fail(s), and 50 exception(s).
[ View ]
#128 interdiff.txt4.86 KBJody Lynn
#127 blocks-1535868-127.patch325.27 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#127 interdiff.txt2.32 KBJody Lynn
#124 blocks-1535868-124.patch323.76 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,466 pass(es), 374 fail(s), and 45 exception(s).
[ View ]
#124 interdiff.txt4.63 KBJody Lynn
#122 blocks-1535868-121.patch328.95 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 46,822 pass(es), 201 fail(s), and 19 exception(s).
[ View ]
#120 interdiff.txt651 bytestizzo
#120 blocks-120-1535868.patch0 bytestizzo
PASSED: [[SimpleTest]]: [MySQL] 46,874 pass(es).
[ View ]
#119 blocks-119-1535868.patch114.28 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,884 pass(es), 235 fail(s), and 29 exception(s).
[ View ]
#119 interdiff.txt4.78 KBJody Lynn
#118 blocks-1535868-118.patch323.91 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#118 interdiff.txt1.35 KBsdboyer
#117 interdiff.txt9.19 KBsdboyer
#117 blocks-1535868-117.patch322.56 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] 45,657 pass(es), 463 fail(s), and 51 exception(s).
[ View ]
#116 views-blocks-do-not-test.patch9.19 KBEclipseGc
#115 blocks-115-1535868.patch313.37 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 45,981 pass(es), 466 fail(s), and 58 exception(s).
[ View ]
#115 interdiff.txt628 bytesJody Lynn
#114 blocks-114-1535868.patch323.48 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,109 pass(es), 482 fail(s), and 58 exception(s).
[ View ]
#114 interdiff.txt1.92 KBJody Lynn
#113 blocks-113-1535868.patch321.37 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,131 pass(es), 488 fail(s), and 58 exception(s).
[ View ]
#113 interdiff.txt6.17 KBJody Lynn
#111 blocks-111-1535868.patch327.46 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,051 pass(es), 561 fail(s), and 61 exception(s).
[ View ]
#110 blocks-110-1535868.patch313.35 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] 46,065 pass(es), 572 fail(s), and 62 exception(s).
[ View ]
#109 blocks-109-1535868.patch324.27 KBsdboyer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-109-1535868.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#109 interdiff.txt2.69 KBsdboyer
#106 blocks-1535868-106.patch323.46 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 45,461 pass(es), 577 fail(s), and 61 exception(s).
[ View ]
#104 blocks-1535868-104.patch266.48 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 45,423 pass(es), 715 fail(s), and 62 exception(s).
[ View ]
#100 blocks-as-plugins-100.patch276.92 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 45,275 pass(es), 723 fail(s), and 65 exception(s).
[ View ]
#99 interdiff.txt72.76 KBnaxoc
#96 blocks-as-plugins-96.patch238.08 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-96.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#96 interdiff.txt25.73 KBnaxoc
#89 blocks-as-plugins-89.patch226.1 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#89 interdiff.txt12.14 KBnaxoc
#86 blocks-as-plugins-86.patch220.98 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#86 interdiff.txt2.46 KBnaxoc
#83 blocks-as-plugins-83.patch219.6 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 41,530 pass(es), 743 fail(s), and 71 exception(s).
[ View ]
#83 interdiff.txt917 bytesnaxoc
#78 blocks-as-plugins-78.patch219.6 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 41,481 pass(es), 755 fail(s), and 85 exception(s).
[ View ]
#74 blocks-as-plugins-74.patch219.61 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] 41,484 pass(es), 751 fail(s), and 85 exception(s).
[ View ]
#70 blocks-as-plugins-70.patch128.11 KBnaxoc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]
#69 blocks-as-plugins-69.patch219.54 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#67 blocks-as-plugins-67.patch219.55 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] 41,065 pass(es), 710 fail(s), and 86 exception(s).
[ View ]
#62 annoblocksapi-do-not-test.patch130.04 KBEclipseGc
#62 aggregator-do-not-test.patch12.64 KBEclipseGc
#62 block_custom-do-not-test.patch10.71 KBEclipseGc
#62 book-do-not-test.patch8.28 KBEclipseGc
#62 comment-do-not-test.patch4.14 KBEclipseGc
#62 forum-do-not-test.patch6.61 KBEclipseGc
#62 language-do-not-test.patch4.64 KBEclipseGc
#62 node-do-not-test.patch12.19 KBEclipseGc
#62 overlay-do-not-test.patch2.34 KBEclipseGc
#62 poll-do-not-test.patch3.21 KBEclipseGc
#62 search-do-not-test.patch2.1 KBEclipseGc
#62 shortcut-do-not-test.patch1.91 KBEclipseGc
#62 statistics-do-not-test.patch8.2 KBEclipseGc
#62 user-do-not-test.patch12.69 KBEclipseGc
#51 annoblocks.patch219.7 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 41,237 pass(es), 716 fail(s), and 86 exception(s).
[ View ]
#48 annoblocks.patch219.66 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#46 annoblocks.patch204.92 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 41,140 pass(es), 708 fail(s), and 71 exception(s).
[ View ]
#44 annoblocks.patch191.79 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] 40,687 pass(es), 538 fail(s), and 51 exception(s).
[ View ]
#42 layout.blocks.42.patch178.67 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,497 pass(es), 520 fail(s), and 45 exception(s).
[ View ]
#42 interdiff.txt696 bytessun
#40 layout.blocks.40.patch177.99 KBsun
FAILED: [[SimpleTest]]: [MySQL] 40,393 pass(es), 622 fail(s), and 543 exception(s).
[ View ]
#38 custom_block.patch4.63 KBJody Lynn
PASSED: [[SimpleTest]]: [MySQL] 41,402 pass(es).
[ View ]
#36 custom_block.patch4.63 KBJody Lynn
PASSED: [[SimpleTest]]: [MySQL] 41,391 pass(es).
[ View ]
#31 annoblocks.patch177.6 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 annoblocks.patch177.6 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 1535868-27-interdiff.txt13 KBJody Lynn
#25 1535868-25-interdiff.txt8.31 KBJody Lynn
#24 scotch-interdiff.patch8.37 KBJody Lynn
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#18 annoblocks.patch152.83 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#14 annoblocks.patch152.96 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#13 annoblocks.patch144.16 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 annoblocks.patch143.33 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 d8-blocks-architecture3.patch148.5 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
d8-blocks-architecture.patch144.31 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch d8-blocks-architecture.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

EclipseGc’s picture

I fully expect tests to fail here, there's lots of stuff that doesn't have updated tests because of how much it has changed.

EclipseGc’s picture

StatusFileSize
new148.5 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

Updated patch

effulgentsia’s picture

Status:Active» Needs review

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

effulgentsia’s picture

Status:Needs review» Active
+++ 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.

EclipseGc’s picture

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

Is a merge artifact. I got it from somewhere, and just assumed it was still needed. It is not for anything I need.

+++ 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',
+      ));

Yes, this can probably be removed.

+++ 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;
+  }
+

This was being used before plugins were updated to not need DrupalConfig objects, so no, at this point blocks doesn't need it, but I can totally see the utility within CMI, and think it SHOULD exist. That being said it certainly doesn't belong in this patch.

+++ 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();

Status, even now, is kind of obtuse. Ultimately, I don't really see a use for it since we'll have visibility settings through condition plugins, and placement through layouts and variations of layouts. I'm not sure I see a need, so unless someone has a really good example use case of why we still need it, I think we can forgo it.

+++ 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);

We'd probably need to pass the UUID for the block through so hook_block_view_$uuid_alter for the specific... I'm not sure it's a debate I care to have, though we might run it past merlin. 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. Again I think use case here, cause what we'd be altering is actually the configuration for the block.

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

I am not sold on any specific naming convention, so sure, build(), view(), render(), whatever()... we might bike shed little things like this a bit, but I don't honestly care, if view the consensus, I'm on board.

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

This isn't mixing instance and definition configuration, this is providing default instance configuration within the plugin, i.e. we need a default block_count for aggregator category in the config form. Normally variable_get() would have provided this mechanism for us, CMI does not do so, and providing such defaults is REALLY ugly. This is very similar to what ctools does for presetting instance configuration of its plugins as well, so I think neclimdul and merlin would probably ++ the spirit of what this is doing.

Hopefully that answers your initial questions. Let me know if you have others.

Eclipse

effulgentsia’s picture

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

effulgentsia’s picture

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

EclipseGc’s picture

Actually, as it stands currently, menu blocks and system menu blocks are actually exposing totally different menus (i.e. system menus and user generated menus). But I get where you're going with this... also while derivatives aren't working right now, check out the menu block class. :-D I think you'll like it if you've not looked real hard yet.

As I said, I'm not opposed to doing this, though after a weekend of working on twig and theme layer stuff, I'm wondering about the use cases that we typically see for this function, but that is sort of a separate concern perhaps.

sun’s picture

Title:D8 Scotch Initiative Architecture Review» Convert all blocks into plugins
Priority:Normal» Major
Issue tags:+Needs architectural review, +API change, +Blocks-Layouts

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

sun’s picture

Issue tags:+Plugin system
EclipseGc’s picture

StatusFileSize
new143.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Wanted to post an update of this. The block have all been converted to the new plugin system.

A few highlights and noteworthy points:

  1. I removed custom blocks. They will need to be added back in, but this will likely be done through a separate module, even if the block is fundamentally unchanged (i.e. we will likely back this block with an entity at some later date)
  2. There is a settings array in the annotation, I really really like this, others have expressed a distaste for it, I'm willing to change it but I didn't really have the time to square that away before this patch.
  3. With the previous point being made PLEASE don't get upset about the @Evaluate annotation class. Moving this as has been requested with regard to the plugin settings should remove the need for this ultimately.
  4. It would be wise to apply the patch in #1727392: Minor logic fix for Derivates Decorator first.

Hopefully that gives some good stuff to work from here. The patch will remove all block hooks EVERYWHERE. The only thing that is left is hook_preprocess_block, and that sort of makes sense to leave alone in my opinion. At least for the time being. I have some ideas about how this might could be made redundant as well, but more on that later.

There's no caching around the annotation discovery. This is great for development purposes, it means creating new blocks happens effortlessly, also the CacheDecorator is getting some love over here: #1722882: Plugin CacheDecorator caches globally, ignores request context, and does not specify tags for cache items. Obviously once that is squared away, a CacheDecorator would be used for blocks.

I have tested to see if this installs properly and it does. No blocks will be enabled by default, but due to how drupal currently handels the main content block, that's not a problem. You can easily start adding content into regions. All blocks have default regions assigned to them, this would obviously go away in the final product, but it was convenient for the time being. You can, of course, move them. A lot of untested stuff in here, should totally fail tests, but the patch should at least be interesting at this point.

Eclipse

PS: For those of you who've looked at the older patches, I removed all the conditions module work I was doing in favor of keeping this as straight forward as possible. There's already a ton of stuff in here, no reason to pollute it with non-essentials.

EclipseGc’s picture

Also, my existing use of UUIDs should likely be moved to a standard "machine_name" model. Doing so would kill #334759: Add machine names for custom blocks.

EclipseGc’s picture

StatusFileSize
new144.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This is a little updated patch that utilizes the new Symfony Bundles to register the BlockManager class in the Dependency Injection Container.

EclipseGc’s picture

StatusFileSize
new152.96 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This patch includes some pretty significant UI changes. I hope to have default blocks in the install profile later today. We will see.

Eclipse

aspilicious’s picture

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

EclipseGc’s picture

Yeah, I'm not even the slightest amount of interested in an upgrade path right now, because this is an intermediary UI designed to give us a good foundation to continue working, but no our final, so I think any upgrade path would be a waste of time right now. Also, various aspects of the system are still in flux.

Glad you're liking the library, it has some tweaks yet that need to be made and I'm sure someone else can hit it with the css stick better than I did in a single pass.

I'd really encourage you to apply the patch against a fresh code base and THEN install, not the other way around. If you do so, please let me know if you still experience problems with the aggregator block(s). I've tried to test everything, but the code has changed quite a bit, and I don't promise that it's perfect ;-)

Eclipse

EclipseGc’s picture

This UI should be a lot better, there's now a UI per theme (much like there was before) however each block instance is per theme, instead of allowing a single instance to support multiple regions in multiple themes. I debated this for a long long time, but functionally, it should be fairly similar to how this would work for layouts, even if architecturally it's different.

region defaults were removed from all blocks. I consider this a pretty big win. The next major steps for this patch are to remove the settings array from the annotations entirely in favor of a method on the class, as well as getting some default configuration in place for the initial install.

Default Configuration:

This is actually a really interesting topic... initially I thought I was going to put this configuration into the install profile, and indeed some of it could, and likely should end up there, but by the same token I wonder if a theme should carry default configuration (so that things like help and main content end up in the right regions by default). I'd love some discussion on this topic, I think this patch is coming along nicely.

Eclipse

EclipseGc’s picture

StatusFileSize
new152.83 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

oops, missed the patch

catch’s picture

Status:Active» Needs review

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

EclipseGc’s picture

Oh, this should totally fail testing for a number of reasons.

1.) I've not updated tests to match the new functionality yet
2.) Last I knew the Symfony Bundles don't work outside of the index.php approach

Still, sort of interested to see what happens :-)

Eclipse

Jody Lynn’s picture

StatusFileSize
new8.37 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Jody Lynn’s picture

StatusFileSize
new8.31 KB

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

Jody Lynn’s picture

Status:Needs review» Needs work

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

Jody Lynn’s picture

StatusFileSize
new13 KB

New patch, now covers BlockCacheTest and BlockHtmlIdTest as well.

Jody Lynn’s picture

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

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new177.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I haven't addressed any of Jody's questions or concerns, but I have included her tests! This is rerolled against head, and includes an abstraction of the UI that was used for blocks so that other systems (conditions for example) could make use of it. 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.

Eclipse

EclipseGc’s picture

StatusFileSize
new177.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

this I missed the tests.

EclipseGc’s picture

or not... whatever...

effulgentsia’s picture

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.

EclipseGc’s picture

Status:Needs review» Needs work

yeah, the only problem is that we'll have to get a system that is currently broken working only to promptly remove it. Still perhaps you're correct.

Jody Lynn’s picture

StatusFileSize
new4.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,391 pass(es).
[ View ]

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.

tim.plunkett’s picture

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

Jody Lynn’s picture

StatusFileSize
new4.63 KB
PASSED: [[SimpleTest]]: [MySQL] 41,402 pass(es).
[ View ]

New patch for tim.

tim.plunkett’s picture

On the patch in #31

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

I would think both would be Plugin (singular).

sun’s picture

Status:Needs work» Needs review
StatusFileSize
new177.99 KB
FAILED: [[SimpleTest]]: [MySQL] 40,393 pass(es), 622 fail(s), and 543 exception(s).
[ View ]

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.

sun’s picture

StatusFileSize
new696 bytes
new178.67 KB
FAILED: [[SimpleTest]]: [MySQL] 40,497 pass(es), 520 fail(s), and 45 exception(s).
[ View ]

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

EclipseGc’s picture

StatusFileSize
new191.79 KB
FAILED: [[SimpleTest]]: [MySQL] 40,687 pass(es), 538 fail(s), and 51 exception(s).
[ View ]

OK, added a first pass at custom blocks, I don't expect any new tests to start magically passing, but at least we'll have the start of something we can being converting the tests to.

Highlights:

Custom blocks can be created from a local action on the Block Library page. New instances of pre-existing blocks can be spawned from the block library itself. The content of the block cannot be updated from there, a separate administration has been constructed for that, though the actual administration of that has not been done yet, it is purely placeholder to communicate my intentions.

Given how this went, I'm heavily leaning towards converting this to an entity now. Sun, Indytechcook and Jodyh all seem on board for that approach, and I think it fixes a number of WTFs at the moment.

WTFs:

Status is a thing on blocks... it doesn't make a lot of sense at the moment, but for custom entities as blocks it could make a lot of sense.
All custom blocks are currently reusable, and that's unlikely to be true in the long term, rather specific blocks would likely want to be marked as being reusable.
Custom block edit form is going to end up being kind of weird since it needs to serve as display (to some extent) within the block configuration, as well as serving as a "creation" mechanism there, while edits happen outside of blocks, because you're not changing the instance, you're changing the content the instance references and... yeah, it's just sort of annoying and weird right now. I think entities would sort of stitch it all together nicely.

Hopefully this all made sense. Try out the patch, and let me know.

Eclipse

EclipseGc’s picture

StatusFileSize
new204.92 KB
FAILED: [[SimpleTest]]: [MySQL] 41,140 pass(es), 708 fail(s), and 71 exception(s).
[ View ]

I've put a lot of work into getting most of the visibility rules working. language rules still need to be updated to work, but overall, they should be working here. This patch required quite a bit of my time today, many Bothans died to bring you this patch.

The tests really need love at this point, I'll be working on language stuff here next, and then we need to finish the custom blocks patch and then clean this thing up. Please install and play, as soon as language visibility is working, I'll be cleaning up the patch to make sure all the functions are sensibly named, and remove the default settings from the annotations.

Eclipse

EclipseGc’s picture

Status:Needs review» Needs work
StatusFileSize
new219.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch annoblocks_7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've not rerolled this against head, but it applies as of: c259b8ec521d75c5018cb9c4b2626413e9c0b2dc I'll reroll this weekend likely.

Thing of note:

All visibility rules should work at this point.
All the hooks that alter block visibility rules have been updated.
Various comment clean up to reflect new architecture.
Updated function names that no longer made sense. (mostly having to do with custom blocks in the main block module and admin)
Removed a ton of code that was no longer executed in the main block.module.
Updated the derivatives variable to default to array(). This removes some of the errors seen in calls to getDefinitions for the block manager class.
Restructured the base class to include configuration/validate/submit/access wrappers to prevent the need for block plugins to ever call parent::method() of any sort.
Cleaned up a handful of things that were apparent errors in various block plugins.
Moved settings in the annotation definitions to a settings method.
Removed the Evaluation annotation class.
Moved the ConfigMapper to Drupal\Core\Plugin\Mapper
Moved the BlockManager class out of Plugins\Type to Plugin\Type (per typical standard).

This is a significant set of changes from the previous patch. I think custom_blocks is totally borked at this point which is fine, cause it's going to need some serious love anyway. I updated every other block in the system. Barring finding any other outstanding issues with this, what I think is left is:

Custom blocks as entities.
Passing tests.

Hopefully this is a significant step forward. I'll be working on Custom blocks as soon as this is rerolled against head. This requires the AlterDecorator, which wasn't committed as of the log hash I mentioned. I'll sort these problems out soon.

Eclipse

EclipseGc’s picture

Status:Needs work» Needs review
StatusFileSize
new219.7 KB
FAILED: [[SimpleTest]]: [MySQL] 41,237 pass(es), 716 fail(s), and 86 exception(s).
[ View ]

Ok, got a patch that should apply to core now. As a little bit of information that I left out of the last update:

1.) hook_block_list_alter has been removed in favor of hook_block_access which is invoked on a block by block basis.
2.) hook_block_view is not implemented in this patch yet. I should fix that.
3.) hook_block_alter was added which actually allows for the complete swapping of block classes (in addition to anything else controlled by the definition).
4.) There are a handful of changes in the BlockInterface and BlockBase that probably need to move up to the PluginBase.
5.) I've not yet removed the unnecessary tables from the block.install, but most of them will be removed in favor of CMI at this point.

This isn't exhaustive, but it's pretty close. Hopefully this patch is getting closer. Custom blocks are still broken. I'll hopefully fix that soon.

Eclipse

cosmicdreams’s picture

Status:Needs review» Needs work

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?

catch’s picture

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.

EclipseGc’s picture

@cosmicdreams

All blocks are converted, what we really need help on here it tests.

@catch

Some amount of conversion will be needed in any event (bare minimum we'll need to remove deltas and add a unique plugin id to the hook_block_* calls), but your request might be doable. I'll also have to port in all the CMI change over the existing variable calls. I think what you're asking for can probably be done, but I'd like to hold off on doing it until only that and tests are left to do.

As a compromise would it be possible to provide the sort of patch you've asked for for review purposes only? I know reviewing a patch of this size is difficult, but in this way we could prevent what will likely be a lot of extra work to create a very temporary BC layer? I could provide separate diffs for each module's blocks, and this might make reviewing in general a whole lot easier. Let me know if that sounds workable. I'm happy to break the patch down, but I worry about the amount of work that would be required to build the BC layer.

Eclipse

catch’s picture

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

cosmicdreams’s picture

Status:Needs work» Needs review

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

Retesting because some meat-y things landed recently.

cosmicdreams’s picture

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

EclipseGc’s picture

No, all of the CMI stuff has been done, I was just saying if we tried to build a BC layer for this, that'd be difficult from a variable_get/cmi stand point because we'd have to figure out how to inject config arrays into the BC layer and all of a sudden that gets messy.

@catch,

I'll see if I can break the patch appart into block plugins and api changes this afternoon.

EclipseGc’s picture

OK, I've broken this into multiple patches. Main patch first:

I've left the system conversion and the menu conversion. Help.module also has a little code in this since it's block is in system, but it's preprocess is in help. The block tests conversion is also in here. Custom blocks are in a separate patch. All other module's code have been moved to separate patches.

Eclipse

effulgentsia’s picture

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

EclipseGc’s picture

I don't see how we'd separate those. Block UI is completely informed by the API, and an updated api won't working the the existing UI. :-S

sdboyer’s picture

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

Bojhan’s picture

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.

effulgentsia’s picture

Status:Needs work» Needs review
StatusFileSize
new219.55 KB
FAILED: [[SimpleTest]]: [MySQL] 41,065 pass(es), 710 fail(s), and 86 exception(s).
[ View ]

Merged HEAD.

EclipseGc’s picture

Status:Needs review» Needs work
StatusFileSize
new219.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-69.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

fixed the whitespace issues in 67, cause that's just how I roll.

naxoc’s picture

StatusFileSize
new128.11 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

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?

podarok’s picture

Status:Needs work» Needs review

bot?

xjm’s picture

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

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

naxoc’s picture

StatusFileSize
new219.61 KB
FAILED: [[SimpleTest]]: [MySQL] 41,484 pass(es), 751 fail(s), and 85 exception(s).
[ View ]

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.

hass’s picture

Status:Needs work» Needs review

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?

EclipseGc’s picture

StatusFileSize
new219.6 KB
FAILED: [[SimpleTest]]: [MySQL] 41,481 pass(es), 755 fail(s), and 85 exception(s).
[ View ]

Getting working against HEAD again.

effulgentsia’s picture

Status:Needs review» Needs work

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.

webchick’s picture

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

effulgentsia’s picture

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.

naxoc’s picture

StatusFileSize
new917 bytes
new219.6 KB
FAILED: [[SimpleTest]]: [MySQL] 41,530 pass(es), 743 fail(s), and 71 exception(s).
[ View ]

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.

EclipseGc’s picture

@naxoc, good catch! Thanks.

@webchick I'll try to get screenshots up here shortly.

@effulgentsia fieldable custom blocks still makes sense here, mostly because unfieldable custom blocks looks totally bizarre in the current patch. I'm open to discussion on that point, but this is what I'm actively working on right now.

Eclipse

naxoc’s picture

StatusFileSize
new2.46 KB
new220.98 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-86.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

naxoc’s picture

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

EclipseGc’s picture

the vast majority of tables are actually irrelevant at this point. I'll remove them in the next patch.

Eclipse

naxoc’s picture

StatusFileSize
new12.14 KB
new226.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-89.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

EclipseGc’s picture

The changes to the aggregator blocks look super sane to me, my apologies for letting those slip through earlier.

naxoc’s picture

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.

Dries’s picture

Issue tags:+Favorite-of-Dries

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

EclipseGc’s picture

@Dries,

Ok, I've left the CacheDecorator off of the plugin discovery thus far for ease of development, but I'll try to get that squared away fairly soon in the hopes of making performance testing possible and reliable.

Eclipse

naxoc’s picture

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.

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

neclimdul’s picture

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.

naxoc’s picture

StatusFileSize
new25.73 KB
new238.08 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-as-plugins-96.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

effulgentsia’s picture

Status:Needs work» Needs review

CNR for bot to see what tests are still failing.

naxoc’s picture

Status:Needs review» Needs work
StatusFileSize
new72.76 KB

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

naxoc’s picture

StatusFileSize
new276.92 KB
FAILED: [[SimpleTest]]: [MySQL] 45,275 pass(es), 723 fail(s), and 65 exception(s).
[ View ]

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.

naxoc’s picture

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.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new266.48 KB
FAILED: [[SimpleTest]]: [MySQL] 45,423 pass(es), 715 fail(s), and 62 exception(s).
[ View ]

I *think* this does the trick.

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

naxoc’s picture

StatusFileSize
new323.46 KB
FAILED: [[SimpleTest]]: [MySQL] 45,461 pass(es), 577 fail(s), and 61 exception(s).
[ View ]

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)
sdboyer’s picture

Status:Needs review» Needs work

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.

sdboyer’s picture

StatusFileSize
new2.69 KB
new324.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-109-1535868.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Jody Lynn’s picture

StatusFileSize
new313.35 KB
FAILED: [[SimpleTest]]: [MySQL] 46,065 pass(es), 572 fail(s), and 62 exception(s).
[ View ]

Rerolled for HEAD.

Jody Lynn’s picture

StatusFileSize
new327.46 KB
FAILED: [[SimpleTest]]: [MySQL] 46,051 pass(es), 561 fail(s), and 61 exception(s).
[ View ]

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.

Jody Lynn’s picture

Status:Needs work» Needs review
Jody Lynn’s picture

StatusFileSize
new6.17 KB
new321.37 KB
FAILED: [[SimpleTest]]: [MySQL] 46,131 pass(es), 488 fail(s), and 58 exception(s).
[ View ]

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.

Jody Lynn’s picture

StatusFileSize
new1.92 KB
new323.48 KB
FAILED: [[SimpleTest]]: [MySQL] 46,109 pass(es), 482 fail(s), and 58 exception(s).
[ View ]

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

Jody Lynn’s picture

StatusFileSize
new628 bytes
new313.37 KB
FAILED: [[SimpleTest]]: [MySQL] 45,981 pass(es), 466 fail(s), and 58 exception(s).
[ View ]

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

EclipseGc’s picture

StatusFileSize
new9.19 KB

Converted the views blocks. This is just an interdiff for simplicity sake for the moment.

Eclipse

sdboyer’s picture

StatusFileSize
new322.56 KB
FAILED: [[SimpleTest]]: [MySQL] 45,657 pass(es), 463 fail(s), and 51 exception(s).
[ View ]
new9.19 KB

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

sdboyer’s picture

StatusFileSize
new1.35 KB
new323.91 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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.

Jody Lynn’s picture

StatusFileSize
new4.78 KB
new114.28 KB
FAILED: [[SimpleTest]]: [MySQL] 46,884 pass(es), 235 fail(s), and 29 exception(s).
[ View ]

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.

tizzo’s picture

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 46,874 pass(es).
[ View ]
new651 bytes

Removing unnecessary settings method call.

neclimdul’s picture

last patch is 0 bytes

effulgentsia’s picture

StatusFileSize
new328.95 KB
FAILED: [[SimpleTest]]: [MySQL] 46,822 pass(es), 201 fail(s), and 19 exception(s).
[ View ]

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.

Jody Lynn’s picture

Status:Needs review» Needs work
StatusFileSize
new4.63 KB
new323.76 KB
FAILED: [[SimpleTest]]: [MySQL] 46,466 pass(es), 374 fail(s), and 45 exception(s).
[ View ]

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.

Jody Lynn’s picture

StatusFileSize
new2.32 KB
new325.27 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-127.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Jody Lynn’s picture

Status:Needs work» Needs review
StatusFileSize
new4.86 KB
new329.66 KB
FAILED: [[SimpleTest]]: [MySQL] 46,694 pass(es), 471 fail(s), and 50 exception(s).
[ View ]

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.

Jody Lynn’s picture

Issue summary:View changes

link filter to issue

Jody Lynn’s picture

Issue summary:View changes

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

Jody Lynn’s picture

StatusFileSize
new5.68 KB
new333.07 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

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

Jody Lynn’s picture

StatusFileSize
new3.02 KB
new336.09 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Fix for the Format Filter Hooks test.

Jody Lynn’s picture

Status:Needs review» Needs work
StatusFileSize
new2.56 KB
new336.81 KB
FAILED: [[SimpleTest]]: [MySQL] 46,791 pass(es), 450 fail(s), and 46 exception(s).
[ View ]

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

Jody Lynn’s picture

Issue summary:View changes

adding comment number

Jody Lynn’s picture

Issue summary:View changes

another test had to have views enabled

Jody Lynn’s picture

Issue summary:View changes

another module enabling in a test

Jody Lynn’s picture

Status:Needs work» Needs review
StatusFileSize
new701 bytes
new337.71 KB
FAILED: [[SimpleTest]]: [MySQL] 46,796 pass(es), 443 fail(s), and 46 exception(s).
[ View ]

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

tizzo’s picture

StatusFileSize
new339.04 KB
FAILED: [[SimpleTest]]: [MySQL] 46,787 pass(es), 439 fail(s), and 46 exception(s).
[ View ]
new1.26 KB

Fixes the views wizard basic functionality test.

tizzo’s picture

StatusFileSize
new1.56 KB
new340.6 KB
FAILED: [[SimpleTest]]: [MySQL] 46,797 pass(es), 432 fail(s), and 47 exception(s).
[ View ]

Fixing views wizard number of items test

tizzo’s picture

Issue summary:View changes

Another module had to be enabled to pass tests

Jody Lynn’s picture

StatusFileSize
new678 bytes
new341.26 KB
FAILED: [[SimpleTest]]: [MySQL] 46,813 pass(es), 415 fail(s), and 41 exception(s).
[ View ]

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

tizzo’s picture

StatusFileSize
new5.81 KB
new346.71 KB
FAILED: [[SimpleTest]]: [MySQL] 46,810 pass(es), 398 fail(s), and 41 exception(s).
[ View ]

Fixed overridden displays tests.

Jody Lynn’s picture

StatusFileSize
new2.62 KB
new342.07 KB
FAILED: [[SimpleTest]]: [MySQL] 46,837 pass(es), 397 fail(s), and 41 exception(s).
[ View ]

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

Fixed the menu block building.

Jody Lynn’s picture

Issue summary:View changes

Added another block module enabling to a test

Jody Lynn’s picture

StatusFileSize
new347.52 KB
FAILED: [[SimpleTest]]: [MySQL] 46,821 pass(es), 391 fail(s), and 44 exception(s).
[ View ]

Combo of 142 and 143 since we cross-posted.

Jody Lynn’s picture

Issue summary:View changes

More dependencies on block/views to track

Jody Lynn’s picture

StatusFileSize
new2.01 KB
new349.53 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-146.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Anonymous’s picture

Issue summary:View changes

Update comment numbers

sdboyer’s picture

Status:Needs review» Needs work
StatusFileSize
new351.12 KB
FAILED: [[SimpleTest]]: [MySQL] 47,141 pass(es), 344 fail(s), and 41 exception(s).
[ View ]

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

sdboyer’s picture

Status:Needs work» Needs review
StatusFileSize
new1.17 KB
new352.29 KB
FAILED: [[SimpleTest]]: [MySQL] 47,154 pass(es), 345 fail(s), and 41 exception(s).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new3.17 KB
new354.11 KB
FAILED: [[SimpleTest]]: [MySQL] 47,153 pass(es), 349 fail(s), and 47 exception(s).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new3.39 KB
new354.28 KB
FAILED: [[SimpleTest]]: [MySQL] 47,174 pass(es), 342 fail(s), and 39 exception(s).
[ View ]

That fix needs to be specific to the block system.

tim.plunkett’s picture

StatusFileSize
new358.05 KB
FAILED: [[SimpleTest]]: [MySQL] 47,203 pass(es), 312 fail(s), and 36 exception(s).
[ View ]
new4.62 KB

Should fix
MenuNodeTest
BlockUpgradePathTest
UserAccountLinksTests

tim.plunkett’s picture

StatusFileSize
new2.03 KB
new359.23 KB
FAILED: [[SimpleTest]]: [MySQL] 47,211 pass(es), 278 fail(s), and 41 exception(s).
[ View ]

Fixed TrailTest and TranslationTest

tim.plunkett’s picture

StatusFileSize
new2.38 KB
new360.36 KB
FAILED: [[SimpleTest]]: [MySQL] 47,227 pass(es), 261 fail(s), and 38 exception(s).
[ View ]

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

tim.plunkett’s picture

StatusFileSize
new11.01 KB
new359.75 KB
FAILED: [[SimpleTest]]: [MySQL] 45,685 pass(es), 1,899 fail(s), and 149 exception(s).
[ View ]

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

tim.plunkett’s picture

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.

tim.plunkett’s picture

Status:Needs review» Needs work
StatusFileSize
new1.46 KB
new360 KB
FAILED: [[SimpleTest]]: [MySQL] 47,226 pass(es), 261 fail(s), and 38 exception(s).
[ View ]

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.

effulgentsia’s picture

Issue tags:-Needs screenshots
StatusFileSize
new152.07 KB
new126.03 KB
new62.17 KB
new89.89 KB

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.

effulgentsia’s picture

Issue summary:View changes

More tests 'fixed' by enabling block

effulgentsia’s picture

Issue summary:View changes

Updated issue summary.

effulgentsia’s picture

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

tim.plunkett’s picture

Status:Needs work» Needs review
Issue tags:+D8 upgrade path
StatusFileSize
new4.8 KB
new356.12 KB
FAILED: [[SimpleTest]]: [MySQL] 47,232 pass(es), 258 fail(s), and 38 exception(s).
[ View ]

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.

Anonymous’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

Berdir’s picture

Status:Needs review» Needs work

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.

EclipseGc’s picture

also, we'll be removing custom_block module from this patch entirely and filing a critical follow up patch to re-add it. This includes all tests that actually need custom block (as opposed to tests that just use it).

Eclipse

rickvug’s picture

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

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new356.37 KB
FAILED: [[SimpleTest]]: [MySQL] 47,377 pass(es), 258 fail(s), and 37 exception(s).
[ View ]

Rerolled for core changes.

tim.plunkett’s picture

StatusFileSize
new355.89 KB
FAILED: [[SimpleTest]]: [MySQL] 47,432 pass(es), 258 fail(s), and 37 exception(s).
[ View ]
tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

StatusFileSize
new817 bytes
new356.69 KB
FAILED: [[SimpleTest]]: [MySQL] 47,440 pass(es), 254 fail(s), and 35 exception(s).
[ View ]

This will hopefully fix MainContentFallbackTest and EnableDisableTest.

tizzo’s picture

Status:Needs review» Needs work
StatusFileSize
new2.35 KB

Fixed BreakCrumb's TrailTest with a commit in the sandbox. Interdiff attached.

yoroy’s picture

Issue tags:+Usability

Tagging because this changes/introduces UI elements. Exciting stuff! :-)

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new366.52 KB
FAILED: [[SimpleTest]]: [MySQL] 47,813 pass(es), 54 fail(s), and 14 exception(s).
[ View ]

Okay here's a patch with a bunch of changes from the sandbox.

dawehner’s picture

Status:Needs review» Needs work
StatusFileSize
new904 bytes

The idea to fix the views block tests is to the subject of the views block to "", so it uses the subject generated by the rendered block by views. As config['subject'] is used in the admin listing i think setting a different default in the UI seems to easy way to do it.

Didn't runned the tests locally but i guess this should work.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new384.14 KB
FAILED: [[SimpleTest]]: [MySQL] 47,913 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

New patch from the branch. Is it possible this might be green...?

tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

StatusFileSize
new386.15 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Random fail, but I have a new patch anyway.

effulgentsia’s picture

Priority:Major» Critical

OMG! Green! How in the world has this issue not been marked critical before? Doing so now.

effulgentsia’s picture

I asked for a retest, because locally, I'm getting an install failure possibly due to #1831350-100: Break the circular dependency between bootstrap container and kernel.

Crell’s picture

I didn't make it all the way through the patch, but here's some thoughts so far:

+++ b/core/includes/bootstrap.inc
@@ -2480,6 +2480,9 @@ function drupal_container(Container $new_container = NULL, $rebuild = FALSE) {
+
+    // Register the Plugin UI.
+    $container->register('plugin.manager.system.plugin_ui', 'Drupal\system\Plugin\Type\PluginUIManager');

Why is this in drupal_container, when we're trying to eliminate the bootstrap container? There may be a reason, but it should be documented inline.

+++ b/core/lib/Drupal/Core/Plugin/Mapper/ConfigMapper.php
@@ -0,0 +1,43 @@
+  public function getInstance(array $options) {
+    $config = config($options['config']);
+    if ($config) {
+      $plugin_id = $config->get('id');
+      $settings = $config->get();
+      $settings['config_id'] = $options['config'];
+      return $this->manager->createInstance($plugin_id, $settings);
+    }
+    // @todo throw an exception.
+  }

Can the config system not be injected here, rather than calling config()?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/FeedBlock.php
@@ -0,0 +1,85 @@
+    // Plugin ids look something like this: aggregator_feed_block:1.
+    $id = substr($this->getPluginId(), 22);

Geesh. Really, there's no other way than to substr it? Treating a plugin ID as non-opaque makes me squirm. If it's impossible to do otherwise, the reason needs to be documented because I am not the only person who will squirm at that.

+++ b/core/modules/block/block.module
@@ -722,176 +461,54 @@ function block_list($region) {
/**
+ * Returns the block plugin manager from the dependency injection container.
+ */
+function block_manager() {
+  return drupal_container()->get('plugin.manager.block');
+}

I'd much prefer we don't add utility wrappers for things that never had them. We need to be weaning people off of these and encouraging use of the DIC. Step one to that is making it clear that people are using the DIC, so that they can then make the shorter jump to injecting from the DIC.

+++ b/core/modules/block/block.module
@@ -722,176 +461,54 @@ function block_list($region) {
-function block_load($module, $delta) {
-  if (isset($delta)) {
-    $block = db_query('SELECT * FROM {block} WHERE module = :module AND delta = :delta', array(':module' => $module, ':delta' => $delta))->fetchObject();
+function block_load($plugin_id, array $conf = array()) {
+  try {
+    $block = block_manager()->getInstance(array('config' => $plugin_id));
   }
-
-  // If the block does not exist in the database yet return a stub block
-  // object.
-  if (empty($block)) {
-    $block = new stdClass();
-    $block->module = $module;
-    $block->delta = $delta;
+  catch (Drupal\Component\Plugin\Exception\PluginException $e) {
+    $block = block_manager()->createInstance($plugin_id, $conf);
   }
-
   return $block;
}

I must be reading this wrong. It looks to me like we're going to be throwing a lot of exceptions in normal operation here. Vis, trying to fetch an already instantiated object, and if an exception is thrown, then creating the object.

If my read is correct, that's totally not cool and not an acceptable use of exceptions. If I'm wrong, please explain why I'm wrong and what is actually going on here, preferably with additional comments.

+++ b/core/modules/block/block.module
@@ -904,50 +521,42 @@ function block_block_list_alter(&$blocks) {
+function block_build($block) {
+  // Allow modules to modify the block before it is viewed, via either
+  // hook_block_view_alter() or hook_block_view_ID_alter().
+  $id = $block->getPluginID();
+  $build = $block->build();
+  drupal_alter(array('block_view', "block_view_{$id}"), $build, $block);
+  return $build;
+}

Er, why is this utility function needed? What's wrong with telling people to just damned well call methods on the object?

If it's for the alter functionality, that should be baked into the plugin somehow, IMO, rather than dipping back out to a random function.

+++ b/core/modules/block/custom_block/custom_block.admin.inc
@@ -0,0 +1,50 @@
+  $results = db_select('block_custom', 'cb')
+    ->fields('cb')
+    ->execute();

There's no reason for this to be a db_select(). db_query() is faster.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.php
@@ -0,0 +1,55 @@
+    $results = db_select('block_custom', 'b')
+      ->fields('b')
+      ->execute();

Ibid.

And also...

zOMG +20 to everyone who's been working on this!

effulgentsia’s picture

Status:Needs review» Needs work
+++ b/core/modules/block/block.module
@@ -1023,23 +646,29 @@ function template_preprocess_block(&$variables) {
+  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
+  foreach ($block_configs as $config_id) {
+    $config = config($config_id);
+    if ($config->get('id') == 'menu_menu_block:' . $menu['menu_name']) {
+      $config->delete();
+    }
+  }

1. Based on the pattern for all our other config entities, the config prefix should be block.block rather than plugin.core.block. I'd even prefer just block, but that might be risky if block.module ever wants to have any config for anything other than the blocks themselves.
2. I think it would be ideal to make block configurations into ConfigEntity objects as part of this initial patch. But, if that will require more than a couple days to accomplish, we can make it a critical follow up instead.

15 days to next Drupal core point release.

tim.plunkett’s picture

StatusFileSize
new12.08 KB
new384.68 KB
PASSED: [[SimpleTest]]: [MySQL] 47,926 pass(es).
[ View ]

@effulgentsia, your first point has a follow-up, and I'd really like to leave that over there: #1839904: Rename plugin.core.block CMI prefix to block.block
Your second point I'd also like to discuss in a follow-up.

@Crell, these are your points I didn't address in this patch:

"Can the config system not be injected here, rather than calling config"
I'm not sure. Not attempting that now.

"trying to fetch an already instantiated object, and if an exception is thrown, then creating the object"
Yes, that's how it works. If you don't like that usage, how would you suggest it change?

EclipseGc’s picture

Status:Needs work» Needs review

Can the config system not be injected here, rather than calling config?

Not unless we want to rearchitect everything in the patch :-). This is a mapper, it's intended to have code specific to the use case, and the use case is "here's a cmi key, get me a plugin instance from that if possible". I have avoided config object injection at every point and suggested to others to do the same, it simplifies mocking significantly, and calling ->get() methods on it just return arrays anyway, so this seems really sane and straight forward to me, and this allows for simple array mocking when needing to satisfy a plugin's configuration.

trying to fetch an already instantiated object, and if an exception is thrown, then creating the object

Yes we ask, "Is there a block by this CMI key? if so return it, if not, then expect this to be a plugin id and configuration combination. The same exception is thrown in both cases, if it is NOT a plugin_id/config combo or relevant CMI key. This allows us to get block instances through the same function whether they're mocked or not, and that's ridiculously handy. It also means that things like layouts could store one off block configurations with relative ease. If you have suggestions on this that's appreciated, but it is very very very intentional.

Eclipse

EclipseGc’s picture

Issue summary:View changes

Updated issue summary.

Crell’s picture

I'm confused now by Tim and Kris's replies. It looks on the surface, and from Tim's comment, like getInstance() is just querying for an already-instantiated object in a cache array somewhere; if that fails, an exception is thrown, and then the object is created instead and added to the array. (I have not verified that's what's actually going on, just that's what it looks like from my not-entirely-deep review.)

If that's correct, then that means a page with 15 blocks on it will throw 15 exceptions in "normal" behavior. That's completely unacceptable; exception throwing is a rather expensive operation, and should by design be reserved for "exceptional", vis. very unusual, situations. Rather, we should do what we do in a few dozen other places in core where we have a static cache and use an isset() check inside a createOrReuseDependingOnWhetherItsBeenInstantiatedAlready() operation.

Based on Kris's comment, however, it sounds like it's for spawning a saved block vs. a created-on-the-fly block. That's a very different situation, but at the same time not an "exceptional" one. In that case, "load a saved block" and "spawn a new block out of whole cloth, with this info" are two entirely different operations and belong in entirely different methods.

If neither of those is what's going on, then I'm very confused. :-)

sdboyer’s picture

we discussed kicking most things to follow-ups today, as this thing is blocker to a handful of other criticals. but, until we've actually put those up...

injecting config, rather than calling it internally within the ConfigMapper, is pretty important. i've been complaining about this (indirectly) for some time, because the current approach basically ensures that we're gonna be issuing one query to the config cache for each block that's loaded (since those are inherently single-loading operations). that's just dumb, and we *know* that we need to be more efficient about that.

EclipseGc’s picture

@Crell

<?php
function block_load($plugin_id, array $conf = array()) {
 
$manager = drupal_container()->get('plugin.manager.block');
  try {
   
$block = $manager->getInstance(array('config' => $plugin_id));
   }
  catch (
Drupal\Component\Plugin\Exception\PluginException $e) {
   
$block = $manager->createInstance($plugin_id, $conf);
   }
   return
$block;
 }
?>

$manager->getInstance() calls the aforementioned mapper, passes a CMI key to it, loads that (if it's legit, which most of the time it will be) and then returns a call to createInstance() with plugin_id and conf parsed from the CMI object. If that fails and an exception is thrown, then we assume it's a plugin_id/conf combo, and manually call createInstance() ourself. (Empty arrays are actually valid configuration for things like the powered by drupal block)

Both of these methods call through to createInstance() there are just various levels of logical wrapping around the final execution of that call. block_load() is the central handler for loading blocks regardless of method and is intended to be smart enough to handle all the supported use cases. If it is not, then any logic that needs a block has to first determine (for itself) whether or not it's dealing with CMI keys, plugin_id/conf or some mixture there of (God help it), and in fact, in order to determine that for itself, it will likely have to rebuild EXACTLY this function to determine that. Plugin_id/conf combos are definitely the degenerate state, but I REALLY thing we want to support them, especially given how that methodology can support testing. Separating this logic has non-trivial consequences, and while I'm not opposed to doing it, we should know what it will mean to do so before moving forward on that crusade.

Happy to detail more in IRC or skype or whatever. Let me know.

Eclipse

Crell’s picture

From the look of the code, at bare minimum the exception should be replaced with a far more efficient if-check:

<?php
function block_load($plugin_id, array $conf = array()) {
 
$manager = drupal_container()->get('plugin.manager.block');
  if (
$conf) {
   
$block = $manager->createInstance($plugin_id, $conf);
  }
  else {
   
$block = $manager->getInstance(array('config' => $plugin_id));
  }
  return
$block;
}
?>

That at least eliminates the unnecessary exception. However, looking at that code I think it becomes obvious why that's a code smell. The D7 registry code that did that was actually one of my slide examples at DrupalCon London for bad code smells. :-)

Calling code needs to know if it's loading a saved plugin by name or an anonymous plugin by conf array anyway, because it needs to know if it's passing in a conf array. If it knows that already, then it's really no burden on it to move that if statement up a level (where such a conditional would have to already exist), at which point this function doesn't have much purpose anymore (aside from potentially being a menu load object for the old menu system).

aspilicious’s picture

+++ b/core/modules/poll/lib/Drupal/poll/Plugin/block/block/PollRecentBlock.phpundefined
@@ -0,0 +1,67 @@
\ No newline at end of file

This patch lacks documentation, some documentation is not consistent and than you have these "no newline at end of file" messages. I know this is an important patch so I'm willing to accept this if there is someone who is going to fix this in the end... (in the best case it would be nice if someone quickly could fix most of the issues in this patch)

aspilicious’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/FeedBlock.phpundefined
@@ -0,0 +1,49 @@
+  public function getDerivativeDefinition($derivative_id, array $base_plugin_definition) {
+    if (!empty($this->derivatives) && !empty($this->derivatives[$derivative_id])) {
+      return $this->derivatives[$derivative_id];
+    }
+    $result = db_query('SELECT cid, title FROM {aggregator_category} ORDER BY title WHERE cid = :cid', array(':cid' => $derivative_id))->fetchObject();
+    $this->derivatives[$derivative_id] = $base_plugin_definition;
+    $this->derivatives[$derivative_id]['delta'] = $result->cid;
+    $this->derivatives[$derivative_id]['subject'] = t('@title feed latest items', array('@title' => $result->title));
+    return $this->derivatives[$derivative_id];
+  }

Isn't this a bug? Shouldn't this be fetching something from the aggregatot_feed table as we are in the FeedBlock class. Seems a copy paste error.

hass’s picture

How about "Latest @title feed items" ?

tim.plunkett’s picture

StatusFileSize
new1.26 KB
new383.56 KB
PASSED: [[SimpleTest]]: [MySQL] 47,968 pass(es).
[ View ]

Rerolled because #1827424: Parse annotations recursively went in.

Also fixed #204, it must have been copy/paste.

#205, this is straight from aggregator with no change, please file an issue against aggregator module.

tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

tim.plunkett’s picture

StatusFileSize
new315 bytes
new383.57 KB
PASSED: [[SimpleTest]]: [MySQL] 47,972 pass(es).
[ View ]

Rerolled with a fix for standard.profile.

sdboyer’s picture

created a followup for the issues raised in #198-#206: #1842932: Refactor the block plugin type's use of config.

next followup to create is the one regarding the structure of the interface, which i complained about in #108.

tim.plunkett’s picture

StatusFileSize
new67.33 KB
new389.87 KB
FAILED: [[SimpleTest]]: [MySQL] 48,084 pass(es), 24 fail(s), and 0 exception(s).
[ View ]

PluginUIBase and PluginUIInterface are completely undocumented, and most of the method parameters make no sense to me.

$plugin and $plugin_id are used almost interchangeably, and I don't think the plugins own ID should be passed to the methods, it's almost never used and getPluginId() should be enough.

In addition, almost every actual block plugin needs a class summary docblock.

I've cleaned up a lot of code, and uncommented out some tests, so we'll see if it still passes.

tim.plunkett’s picture

StatusFileSize
new14.48 KB
new394.09 KB
FAILED: [[SimpleTest]]: [MySQL] 48,370 pass(es), 24 fail(s), and 0 exception(s).
[ View ]

Well, it also turned out that custom blocks were broken, and the tests were all removed or not running (the filename didn't match the class name).

I didn't yet figure out how I broke the aggregator tests.

tim.plunkett’s picture

StatusFileSize
new395.75 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new3.3 KB

Default blocks being loaded were conflicting with the aggregator test.

Also, I tried rerolling around #891670: Add a Footer menu to the built-in system menus, we'll see if that worked.

tim.plunkett’s picture

StatusFileSize
new395.73 KB
FAILED: [[SimpleTest]]: [MySQL] 48,475 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Well, it turns out that 350 uses of setUp() have the wrong visibility (it should be protected).
Not fixing them all here :)

sdboyer’s picture

StatusFileSize
new1.38 KB
new396.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-1535868-217.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

fixed that one, small new error with the standard profile test...hoping that makes the difference.

sdboyer’s picture

StatusFileSize
new400.92 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

bleh, rerolled to chase HEAD. hopefully got it all right...

tim.plunkett’s picture

StatusFileSize
new396.46 KB
PASSED: [[SimpleTest]]: [MySQL] 48,913 pass(es).
[ View ]

Here's another attempt at a reroll.

tim.plunkett’s picture

StatusFileSize
new13.06 KB

Okay, I added docblocks for the various block plugins.

The only remaining task is to cleanup and document BlockPluginUI.

Only attaching an interdiff, since they're doc-only fixes. The changes are in the branch.

sdboyer’s picture

Posted another followup, based on the issues raised in #108: #1850286: Refactor BlockInterface wrapper methods

tim.plunkett’s picture

StatusFileSize
new397.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1535868-224.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
naxoc’s picture

StatusFileSize
new707 bytes
new397.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1535868-226.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

The PluginUIManager was overriding a method as protected, that the parent PluginManagerBase has a public. Made the installer fail. New patch with public method.

tim.plunkett’s picture

StatusFileSize
new397.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Both this patch and #1793520: Add access control mechanism for new router system added SystemBundle, that got in first.

tim.plunkett’s picture

StatusFileSize
new397.65 KB
PASSED: [[SimpleTest]]: [MySQL] 48,916 pass(es).
[ View ]

We crossposted, I committed/pushed your fix and rebased.

mbrett5062’s picture

Tried to test this latest patch from #228 but failed to apply against latest 8.x.

Here is the git message for ref.

error: patch failed: core/modules/block/block.admin.inc:267
error: core/modules/block/block.admin.inc: patch does not apply
error: patch failed: core/modules/block/block.module:403
error: core/modules/block/block.module: patch does not apply
error: patch failed: core/modules/node/node.module:2056
error: core/modules/node/node.module: patch does not apply

Hope that is of some help.

EclipseGc’s picture

yay chasing head. Will rectify this once I'm done with my changes to the patch.

EclipseGc’s picture

StatusFileSize
new401.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch blocks-plugins.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

OK, didn't have a chance to fix the whole chasing head thing, thought I'd post my patch anyway.

Eclipse

(This should fail to apply)

tim.plunkett’s picture

Assigned:EclipseGc» tim.plunkett
Status:Needs review» Needs work

I'll post the reroll tonight, and include an interdiff of eclipse's changes.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new15.83 KB
new401.72 KB
FAILED: [[SimpleTest]]: [MySQL] 47,779 pass(es), 765 fail(s), and 63 exception(s).
[ View ]

Okay, I think I managed to reroll everything. Also, here is the interdiff that should have (ahem) gone with #231.

I'm going to review these changes and clean up their coding standards while this thing runs.

tim.plunkett’s picture

StatusFileSize
new15.39 KB
new402.25 KB
FAILED: [[SimpleTest]]: [MySQL] 48,444 pass(es), 189 fail(s), and 21 exception(s).
[ View ]

PluginUIBase makes a lot of assumptions that are currently specific to the Block UI. I've moved those into BlockPluginUI.

Also, I reverted the block_load() changes from the last patch, that caused most of the failures.

tim.plunkett’s picture

StatusFileSize
new403.46 KB
PASSED: [[SimpleTest]]: [MySQL] 48,999 pass(es).
[ View ]

Fixed that failure, I think we should be in back in business.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
StatusFileSize
new3.77 KB
new404.58 KB
FAILED: [[SimpleTest]]: [MySQL] 48,629 pass(es), 81 fail(s), and 7 exception(s).
[ View ]

Okay, I finally sat down and looked at block_load(), and Crell was right, it was the wrong place for exception handling. It should, and now is, handled by the mapper itself.

Also, per @EclipseGc's request, I added a CacheDecorator to BlockManager.

From my perspective this is ready.

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett
Status:Needs review» Needs work

Ah, the CacheDecorator pointed out a couple places where blocks are created that the definitions should be cleared. Working on that now.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned
Status:Needs work» Needs review
StatusFileSize
new404.37 KB
PASSED: [[SimpleTest]]: [MySQL] 48,956 pass(es).
[ View ]

After digging through that a bit, I've opened #1853190: Cache block plugin definitions.
Currently hook_block_info() is called without caching, the current patch is no different in that way.

Assuming that all of the above fails were strictly related to caching, this should pass.

EclipseGc’s picture

further tagging.

aspilicious’s picture

Quickly scanned the patch.

+++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.phpundefined
@@ -65,6 +65,11 @@ public function getDefinitions() {
     foreach ($base_plugin_definitions as $base_plugin_id => $plugin_definition) {
+      // @todo Remove this check once http://drupal.org/node/1780396 is resolved.

+++ b/core/modules/system/system.plugin.ui.cssundefined
@@ -0,0 +1,29 @@
\ No newline at end of file

Newline needed + some docblocks have a summary that is way longer than 80 chars. But other than that this looks prety good already.

Why are those lines in system.install commented?

xjm’s picture

Assigned:Unassigned» xjm

Going to review this monster. I'll inevitably find some docs issues that will distract me from the code, so preemptively assigning to myself to clean them up. ;)

xjm’s picture

Okay so far I've reviewed only up from the bottom of the patch through the User module. I have questions that are potentially answered elsewhere in the patch or issue, which I'll read after. More after dinner. I'll also clean up some of the things I mention once I get through the patch, so don't bother rerolling just yet. :)

  1. +++ b/core/modules/user/lib/Drupal/user/Plugin/block/block/UserOnlineBlock.phpundefined
    @@ -0,0 +1,105 @@
    +  public function configure($form, &$form_state) {

    I dig this.

  2. +++ b/core/modules/user/lib/Drupal/user/Plugin/block/block/UserOnlineBlock.phpundefined
    @@ -0,0 +1,105 @@
    +   * @todo Move this block to statistics.module and remove dependency on
    +   *   user.access.

    user.access? Is this supposed to be user_access()? Let's move this @todo to the class docblock.

  3. +++ b/core/modules/user/lib/Drupal/user/Tests/UserAccountLinksTests.phpundefined
    @@ -19,7 +19,7 @@ class UserAccountLinksTests extends WebTestBase {
    -  public static $modules = array('menu');
    +  public static $modules = array('menu', 'block');

    Buh?

  4. +++ b/core/modules/user/user.installundefined
    @@ -650,22 +650,9 @@ function user_update_8009(&$sandbox) {
    -function user_update_8010() {
    ...
    -function user_update_8011() {
    +function user_update_8010() {

    @@ -755,7 +742,7 @@ function user_update_8011() {
    -function user_update_8012(&$sandbox) {
    +function user_update_8011(&$sandbox) {

    @@ -823,7 +810,7 @@ function user_update_8012(&$sandbox) {
    -function user_update_8013() {
    +function user_update_8012() {

    I guess we can do this since we aren't rolling unstables yet and one has to nuke and reinstall D8 regularly anyway.

  5. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
    @@ -0,0 +1,85 @@
    + * Contains \Drupal\views\Plugin\block\block\ViewsBlock.

    lol

  6. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
    @@ -0,0 +1,85 @@
    +    list($plugin, $delta) = explode(':', $this->getPluginId());
    +    list($name, $this->displayID) = explode('-', $delta, 2);

    This totally needs an inline comment, or to be factored into a method with a meaningful name.

  7. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
    @@ -0,0 +1,85 @@
    +    // Set the default subject to '' so the views internal title is used.
    +    $form['settings']['title']['#default_value'] = '';
    +    $form['settings']['title']['#access'] = FALSE;
    +    return $form;

    Interesting. I guess we probably always did this.

  8. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsBlock.phpundefined
    @@ -0,0 +1,85 @@
    +  public function build() {
    +    $output = $this->view->executeDisplay($this->displayID);

    So, this is way cool and clean, but it does represent a change in the way block displays get rendered, no? Has there been any profiling around that?

  9. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsExposedFilterBlock.phpundefined
    @@ -0,0 +1,38 @@
    +use Drupal\Core\Annotation\Plugin;
    +use Drupal\Core\Annotation\Translation;

    Hm, do we always use these in annotated plugins and I just never noticed before? How unobservant of me.

  10. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsExposedFilterBlock.phpundefined
    @@ -0,0 +1,38 @@
    +    $this->view->destroy();

    I am enjoying the concept of build() calling destroy(). Is this for memory reasons? Note to self: look this up.

  11. +++ b/core/profiles/standard/config/plugin.core.block.bartik.powered.ymlundefined
    @@ -0,0 +1,18 @@
    +subject: ''

    Yeah, seeing this over and over makes me think of #1591806: Change block "subject" so that it's called a (admin_)label like everything else on the theme layer. It's like someone grabbed a thesaurus because they were tired of seeing "title". :)

  12. +++ b/core/profiles/standard/config/plugin.core.block.bartik.tools.ymlundefined
    @@ -0,0 +1,18 @@
    +id: 'system_menu_block:menu-tools'

    So the IDs are block_type:instance-name? Why the switch from underscores to hyphens? Wil this be clear to me when I get to the top of the patch and/or read the issue? :)

  13. +++ b/core/profiles/standard/config/plugin.core.block.bartik.tools.ymlundefined
    --- /dev/null
    +++ b/core/profiles/standard/config/plugin.core.block.seven.content.ymlundefined

    +++ b/core/profiles/standard/config/plugin.core.block.seven.content.ymlundefined
    @@ -0,0 +1,18 @@
    +region: content

    --- /dev/null
    +++ b/core/profiles/standard/config/plugin.core.block.seven.help.ymlundefined

    +++ b/core/profiles/standard/config/plugin.core.block.seven.help.ymlundefined
    @@ -0,0 +1,18 @@
    +region: help

    Wait, why does this have a region key in the config if there's also a region in the object name?

  14. +++ b/core/profiles/standard/config/plugin.core.block.seven.login.ymlundefined
    @@ -0,0 +1,19 @@
    +whois_new_count: '5'

    Fancy custom configuration key for this block. Nifty.

  15. +++ b/core/profiles/standard/config/plugin.core.block.seven.login.ymlundefined
    --- /dev/null
    +++ b/core/profiles/standard/config/plugin.core.block.seven.navigation.ymlundefined

    +++ b/core/profiles/standard/config/plugin.core.block.seven.navigation.ymlundefined
    @@ -0,0 +1,18 @@
    +id: 'system_menu_block:menu-main'
    ...
    +module: system

    +++ b/core/profiles/standard/config/plugin.core.block.seven.powered.ymlundefined
    --- /dev/null
    +++ b/core/profiles/standard/config/plugin.core.block.seven.search.ymlundefined

    +++ b/core/profiles/standard/config/plugin.core.block.seven.search.ymlundefined
    @@ -0,0 +1,18 @@
    +id: search_form_block
    ...
    +module: search

    Looks like quite the naming pattern, plugin.core.module.theme.region. If we cap module and theme names at 64 each, per #1852454: Restrict module and theme name lengths to 50 characters, that leaves us at a maximum region ID length of 112 characters. (Of course, the API module is always going to be block here, but it's a good reason to not additionally add the owner module to the name in #1776830-13: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API. (We mostly decided not to do that anyway.) See #1186034: Length of configuration object name can be too small.

    Also, the fact that blocks are storing the module they belong to here as a key is interesting in light of #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API. We were considering metadata or a manifest file. ...Except, actually, the owner of this configuration is not search, it's standard. Ahhhh! Are there any default blocks provided by modules in this patch?

  16. +++ b/core/profiles/standard/standard.installundefined
    @@ -76,114 +76,114 @@ function standard_install() {
    +  //$admin_theme = 'seven';
    +  //$blocks = array(
    +    //array(

    Why is all this stuff commented out instead of deleted?

Followups

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsExposedFilterBlock.phpundefined
    @@ -0,0 +1,38 @@
    +    $type = 'exp';

    Out of scope for this issue, but it took me like 20 minutes for my subconscious to clarify that this means "exposed filter". At least I think it does.

  2. +++ b/core/modules/views/lib/Drupal/views/Tests/UI/OverrideDisplaysTest.phpundefined
    @@ -102,17 +99,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
    -    // Put the block into the first sidebar region, and make sure it will not
    -    // display on the view's page display (since we will be searching for the
    -    // presence/absence of the view's title in both the page and the block).

    @@ -142,7 +138,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
    -    $this->assertNoText($view['block[title]']);

    @@ -162,7 +157,6 @@ function testWizardMixedDefaultOverriddenDisplays() {
    -    $this->assertNoText($new_block_title);

    Hm, why are we removing this part of the coverage? (I haven't read the test; it just jumped out at me.) Perhaps instead of assertText()/assertNoText() we should add xpath assertions to this test? (That can totally be a followup issue.)

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/UI/OverrideDisplaysTest.phpundefined
    @@ -44,31 +44,31 @@ function testOverrideDisplays() {
    -    // Change the title for the page display only, and make sure that is the
    -    // only one that is changed.
    +    // Change the title for the page display only, and make sure that the original title
    +    // still appears on the page.

    @@ -77,10 +77,7 @@ function testOverrideDisplays() {
    -    $this->assertNoText($original_title);
    -    $this->drupalGet('');
    ...
    -    $this->assertNoText($new_title);

    Similar here. We're losing a bit of test coverage here, or so it seems. (Also, that comment is way over 80 characters.) :)

  4. +++ b/core/modules/user/lib/Drupal/user/Tests/UserBlocksTests.phpundefined
    @@ -21,32 +21,41 @@ class UserBlocksTests extends WebTestBase {
    -    // Enable user login block.
    -    db_merge('block')
    -      ->key(array(
    -        'module' => 'user',
    -        'delta' => 'login',
    -        'theme' => variable_get('theme_default', 'stark'),
    -      ))
    -      ->fields(array(
    -        'status' => 1,
    -        'weight' => 0,
    -        'region' => 'sidebar_first',
    -        'pages' => '',
    -        'cache' => -1,
    -      ))
    -      ->execute();
    +    $this->adminUser = $this->drupalCreateUser(array('administer blocks'));
    +    $this->drupalLogin($this->adminUser);
    +
    +    $block_id = 'user_login_block';
    +    $default_theme = variable_get('theme_default', 'stark');
    +
    +    $block = array(
    +      'title' => $this->randomName(8),
    +      'machine_name' => $this->randomName(8),
    +      'region' => 'sidebar_first',
    +    );
    +
    +    // Enable the user login block.
    +    $this->drupalPost('admin/structure/block/manage/' . $block_id . '/' . $default_theme, $block, t('Save block'));
    +    $this->assertText(t('The block configuration has been saved.'), 'User login block enabled');
    +    $this->plugin_id = 'plugin.core.block.' . $default_theme . '.' . $block['machine_name'];
    +    $this->drupalLogout($this->adminUser);

    Hm. Is there a way we could do this programmatically instead? Could be a followup issue.

Code style crap

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/block/block/ViewsExposedFilterBlock.phpundefined
    @@ -0,0 +1,38 @@
    +    // Before returning the block output, convert it to a renderable
    +    // array with contextual links.

    Comment wrapping too early, I believe.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsBlock.phpundefined
    @@ -0,0 +1,70 @@
    +      // disabled views get nothing.

    +++ b/core/modules/views/lib/Drupal/views/Plugin/Derivative/ViewsExposedFilterBlock.phpundefined
    @@ -0,0 +1,63 @@
    +      // disabled views get nothing.

    Capitalize me. And actually, maybe the comment could be a bit more clear than "get nothing"? :)

  3. +++ b/core/modules/user/lib/Drupal/user/Plugin/block/block/UserNewBlock.phpundefined
    @@ -0,0 +1,80 @@
    +    // Retrieve a list of new users who have subsequently accessed the site successfully.

    Over 80 characters.

xjm’s picture

Few more notes, up through the top of System module. I'll finish reviewing and do my cleanups tomorrow probably.

  1. +++ b/core/modules/system/system.moduleundefined
    @@ -1071,10 +1075,103 @@ function system_menu() {
    +  foreach (drupal_container()->get('plugin.manager.system.plugin_ui')->getDefinitions() as $plugin_id => $plugin) {

    plugin_ui? I am experiencing trepidation.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Derivative/SystemMenuBlock.phpundefined
    @@ -0,0 +1,50 @@
    +      // The block deltas need to be prefixed with 'menu-', since the 'main'
    +      // menu would otherwise clash with the 'main' page content block.

    Okay, this answers my earlier question sorta. Why a hyphen instead of an underscore, though? Is it so we can explode menu-my_special_menu around the hyphen? I guess that makes sense, but it's kinda ugly, especially when the config object name for these objects already has two other kinds of separators in it.

  3. +++ b/core/modules/system/lib/Drupal/system/Plugin/PluginUIInterface.phpundefined
    @@ -0,0 +1,48 @@
    +/**
    + * Interface definition for Plugin UI plugins.
    + */
    +interface PluginUIInterface {

    +++ b/core/modules/system/lib/Drupal/system/Plugin/Type/PluginUIManager.phpundefined
    @@ -0,0 +1,42 @@
    +/**
    + * Manages discovery and instantiation of Plugin UI plugins.
    + */
    +class PluginUIManager extends PluginManagerBase {

    Do I want to know what Plugin UI plugins are? Is this elsewhere in the patch? Is this API that could be added before the patch? :)

    Also, I really hope there's a whole lot more documentation about Plugin UI plugins somewhere further up in the patch, and these classes should have an @see to where it's documented.

    Finally, class summaries should start with a third-person verb.

  4. +++ b/core/modules/system/lib/Drupal/system/Plugin/PluginUIBase.phpundefined
    @@ -0,0 +1,105 @@
    + * An abstract base class for use in creating sane default user interfaces for
    + * plugins of a particular type.

    Okay, well, this is slightly better than the complete lack of documentation on the interface and manager, however, a lot more documentation would still be a good idea. What is it? What does it mean? Is this a thing like the list controller?

    It seems like the interface would probably be the best place for the bulk of the documentation?

    This could also use some @see. Finally, the summary should be one line of 80 characters or fewer, with subsequent paragraphs of text after a blank line.

  5. +++ b/core/modules/system/lib/Drupal/system/Plugin/Type/PluginUIManager.phpundefined
    @@ -0,0 +1,42 @@
    +    $this->discovery = new CacheDecorator(new AlterDecorator(new DerivativeDiscoveryDecorator(new AnnotatedClassDiscovery('system', 'plugin_ui')), 'plugin_ui'), 'plugin_ui');

    Very decorative.

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemMainBlock.phpundefined
    @@ -0,0 +1,34 @@
    +  public function build() {
    +    return array(
    +      drupal_set_page_content()
    +    );

    Nice.

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemMenuBlock.phpundefined
    @@ -0,0 +1,45 @@
    +    list($plugin, $derivative) = explode(':', $this->getPluginId());
    ...
    +    list($plugin, $derivative) = explode(':', $this->getPluginId());

    More uncommented explodings, though this one is not as icky as the last one. :)

  8. +++ b/core/modules/system/lib/Drupal/system/Tests/System/AccessDeniedTest.phpundefined
    @@ -55,15 +55,16 @@ function testAccessDenied() {
    -    $this->assertText(t('User login'), 'Blocks are shown on the custom 403 page');
    +    $this->assertText(t('Username'), 'Blocks are shown on the custom 403 page');

    @@ -77,12 +78,15 @@ function testAccessDenied() {
    -    $this->assertText(t('User login'), 'Blocks are shown on the default 403 page');
    +    $this->assertText(t('Username'), 'Blocks are shown on the default 403 page');

    Hm, the title is changing from "User login" to "Username," or the title is not shown for some reason? Note to self: see what the verbose results on this page are.

  9. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/FilledStandardUpgradePathTest.phpundefined
    @@ -45,7 +45,8 @@ public function testFilledStandardUpgrade() {
    +    // @todo Blocks are not being upgraded.
    +    //   $this->assertText(t('Tools'));

    +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.phpundefined
    @@ -58,7 +58,8 @@ public function testLanguageUpgrade() {
    +    // @todo Blocks are not being upgraded.
    +    //   $this->assertTrue($this->xpath('//div[@id="block-language-language-interface"]'), 'The language switcher block is being correctly showed.');

    +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/UserRoleUpgradePathTest.phpundefined
    @@ -58,8 +58,11 @@ public function testRoleUpgrade() {
    +    // @todo Blocks are not being upgraded.
    +    //   $this->assertFieldChecked('edit-visibility-role-roles-5', "Who's online block visibility setting is correctly set for the long role name.");

    Um. This sounds like a pretty big deal for an inline comment and an @todo. That sounds like a blocker.

  10. +++ b/core/modules/system/system.moduleundefined
    @@ -1071,10 +1075,103 @@ function system_menu() {
    +      $items[$plugin['path'] . '/' . $plugin_id . '/%'] = array(
    +        'title' => $plugin['title'],
    +        'page callback' => 'drupal_get_form',
    +        'page arguments' => array($plugin['id'], $plugin_id, (count(explode('/', $plugin['path'])) + 1)),
    +        'access callback' => 'system_plugin_ui_access',
    +        'access arguments' => array($plugin_id, (count(explode('/', $plugin['path'])) + 1)),
    +        'type' => MENU_CALLBACK,
    +      );

    Hmmmmm. Is the % an argument placeholder? Aren't routes supposed to be in yaml or some thing? Suddenly D7's hook_menu() looks very attractive.

  11. +++ b/core/modules/system/system.moduleundefined
    @@ -2542,100 +2639,37 @@ function system_user_timezone(&$form, &$form_state) {
    +        $variables['attributes_array']['role'] = 'navigation';

    Confused. The role is navigation?

    Oh. This is that HTML5 accessibility stuff, right?

Followups

  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/BlockUpgradePathTest.phpundefined
    @@ -35,25 +35,23 @@ public function setUp() {
         $this->assertText('Block title cannot be longer than 255 characters', 'Block with title longer than 255 characters created unsuccessfully.');

    Totally out of scope (it's just context lines in an updated test) but "created unsuccessfully" doesn't make any sense.

  2. +++ b/core/modules/system/lib/Drupal/system/Plugin/block/block/SystemMenuBlock.phpundefined
    @@ -0,0 +1,45 @@
    +    // @todo The 'Tools' menu should be available to anonymous users.

    Is there an issue for this yet?

Code style crap

  1. +++ b/core/modules/statistics/statistics.moduleundefined
    @@ -480,3 +407,13 @@ function statistics_library_info() {
    +/**
    + * Implements hook_block_alter().
    + */
    +function statistics_block_alter(&$definitions) {
    +  $statistics_count_content_views = config('statistics.settings')->get('count_content_views');
    +  if (empty($statistics_count_content_views)) {
    +    unset($definitions['statistics_popular_block']);
    +  }

    Not that this code is hard to understand or anything, but it's slightly annoying when the docblock doesn't say what the hook implementation is doing and there isn't an inline comment either. (I'm cognizant of the fact that this lack-o-doc might not have been introduced here.)

  2. +++ b/core/modules/system/system.plugin.ui.cssundefined
    @@ -0,0 +1,29 @@
    \ No newline at end of file

    Missing newline.

  3. +++ b/core/modules/system/system.moduleundefined
    @@ -1071,10 +1075,103 @@ function system_menu() {
    + * A simple wrapper function for proxying to the plugin class' access method.

    Verbs the foo bar.

  4. +++ b/core/modules/system/system.moduleundefined
    @@ -1071,10 +1075,103 @@ function system_menu() {
    + * Page callback: Autocomplete any plugin system tied to a plugin UI plugin.

    Add s.

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/TrailTest.phpundefined
    @@ -30,34 +30,28 @@ public static function getInfo() {
    +    $this->drupalPost("admin/structure/block/manage/system_menu_block:menu-tools/{$default_theme}", $block, t('Save block'));
    +

         // This test puts menu links in the Administration menu and then tests for

    Extra blank line. :)

  6. +++ b/core/modules/system/lib/Drupal/system/Plugin/PluginUIInterface.phpundefined
    @@ -0,0 +1,48 @@
    +   * Create a form array.

    Add s.

  7. +++ b/core/modules/system/lib/Drupal/system/Plugin/PluginUIBase.phpundefined
    @@ -0,0 +1,105 @@
    +   * Allows a given plugin to exclude various defintions form the user
    +   * interface via whatever criteria make sense for that plugin.
    ...
    +   * Generic access check for use with plugins of this type.
    ...
    +   * A customized row method for displaying plugins for configuration within
    +   * the user interface.

    Summaries should be one line of 80 characters or fewer.

xjm’s picture

There aren't enough tags on this issue.

xjm’s picture

Up through the Node module. Still not even half done...

  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/block/block/SyndicateBlock.phpundefined
    @@ -0,0 +1,51 @@
    + * Provides a 'Syndicate' block.
    + *
    + * @Plugin(
    + *   id = "node_syndicate_block",
    + *   subject = @Translation("Syndicate"),
    + *   module = "node"
    + * )
    ...
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build();
    +   */
    +  public function build() {
    +    return array(
    +      '#theme' => 'feed_icon',
    +      '#url' => 'rss.xml',
    +    );

    Huh. I never knew this was a block, and I didn't understand what it was at first. I think that the block plugin docblocks could, in general, do a bit more to mention what the blocks are and what they're for. "A 'Syndicate' block" isn't entirely clear out of context, and I find myself wondering why we're letting the Syndicate have a block in Drupal. Trust no one.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockTest.phpundefined
    @@ -30,20 +30,30 @@ public static function getInfo() {
    +    // Enable the syndicate block.
    +    $this->drupalPost('admin/structure/block/manage/' . $block_id . '/' . $default_theme, $block, t('Save block'));
    +    $this->assertText(t('The block configuration has been saved.'), 'Node syndicate block enabled.');

    -    // Set the block to a region to confirm block is available.
    -    $edit = array();
    -    $edit['blocks[node_syndicate][region]'] = 'footer';
    -    $this->drupalPost('admin/structure/block', $edit, t('Save blocks'));
    -    $this->assertText(t('The block settings have been updated.'), 'Block successfully move to footer region.');
    +    // Confirm that the block's xpath is available.
    +    $xpath = $this->buildXPathQuery('//div[@id=:id]/*', array(':id' => 'block-' . strtr(strtolower($block['machine_name']), '-', '_')));
    +    $this->assertFieldByXPath($xpath, NULL, 'Syndicate block found.');

    Yay, xpath! Robust coverage++ But why was this test fixed differently than the other ones that are just asserting that the blocks get rendered?

  3. +++ b/core/modules/node/node.moduleundefined
    @@ -1082,11 +1082,11 @@ function node_is_page(Node $node) {
    -    switch ($variables['block']->delta) {
    -      case 'syndicate':
    +    switch ($variables['block']->id) {
    +      case 'node_syndicate_block':
    ...
    -      case 'recent':
    +      case 'node_recent_block':

    So this is at least the second time I've seen this, that it's switching from delta to ID. I know that the delta isn't going away from elsewhere in the patch, or at least it still appears to still be there for all intents and purposes. (Or intensive porpoises.) What's the deal? Presumably this will become clear once I read the new architecture. There's also some API change-y stuff going on here that we should make sure gets documented in a change notification. (Presumably this issue will have more than one, since this change is very themer-facing but themers don't care about the stuff we're doing to the base API.)

  4. +++ b/core/modules/node/node.moduleundefined
    @@ -2045,7 +1973,7 @@ function theme_node_recent_content($variables) {
    function node_form_block_add_block_form_alter(&$form, &$form_state) {
    -  node_form_block_admin_configure_alter($form, $form_state);
    +  //node_form_block_admin_configure_alter($form, $form_state);
    }

    Why is this commented out? When you comment stuff out, at least comment why you're commenting it out. :)

  5. +++ b/core/modules/node/node.moduleundefined
    @@ -2124,62 +2008,51 @@ function node_form_block_custom_block_delete_submit($form, &$form_state) {
    +  // Remove the block visibility settings for all node types.
    +  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');

    So, @tim.plunkett pointed me to #1839904: Rename plugin.core.block CMI prefix to block.block and I think that really makes more sense. plugin.core.block is a bit superfluous.

  6. +++ b/core/modules/openid/openid.moduleundefined
    @@ -130,20 +130,18 @@ function openid_user_logout($account) {
      * Implements hook_block_view_MODULE_DELTA_alter().
      *
      * Adds the OpenID login form to the user login block.
    - *
    - * @see user_login_block()

    Why removing this @see? Shouldn't we just update it to reference the new plugin instead?

  7. +++ b/core/modules/openid/openid.moduleundefined
    @@ -130,20 +130,18 @@ function openid_user_logout($account) {
    -function openid_block_view_user_login_alter(&$block) {
    +function openid_block_view_user_login_block_alter(&$build, $block) {

    @@ -152,10 +150,10 @@ function openid_block_view_user_login_alter(&$block) {
    -  $block['content']['user_links']['#weight'] = 10;
    +  $build['user_links']['#weight'] = 10;

    Okay, so clearly an API change here that I hope I find to be documented when I get to block.api.php and the issue summary.

  8. +++ b/core/modules/overlay/overlay.moduleundefined
    @@ -467,9 +467,9 @@ function theme_overlay_disable_message($variables) {
    /**
    - * Implements hook_block_list_alter().
    + * Implements hook_block_access().
      */
    -function overlay_block_list_alter(&$blocks) {
    +function overlay_block_access($block) {
       // If we are limiting rendering to a subset of page regions, hide all blocks
       // which appear in regions not on that list. Note that overlay_page_alter()
       // does a more comprehensive job of preventing unwanted regions from being

    @@ -866,7 +865,7 @@ function overlay_get_regions_to_render() {
    - * @see overlay_block_list_alter()
    + * @see overlay_block_access()

    Procedural function? Wrapper? Presumably the why of this will become clear as I read up.

    Edit, oh, it's a hook implementation. I'll probably have to actually read the patched module to grok this. I guess the gist is, overlay doesn't want all the regions on the page, so we enforce that by conditionally denying access to the blocks from other modules we don't want?

    Isn't this really more of a hook_block_access_alter(), conceptually? Since it's changing access defined by other modules? Or I guess maybe an alter hook doesn't make sense if there's not a different hook that it's altering because the access is in a per-plugin method instead. O brave new API, that has such methods in't.

  9. +++ b/core/modules/poll/lib/Drupal/poll/Tests/PollBlockTest.phpundefined
    @@ -12,6 +12,8 @@
    +  protected $adminUser;

    @@ -31,23 +33,26 @@ function setUp() {
         // Create and login user
    -    $admin_user = $this->drupalCreateUser(array('administer blocks'));
    -    $this->drupalLogin($admin_user);
    +    $adminUser = $this->drupalCreateUser(array('administer blocks'));
    +    $this->drupalLogin($adminUser);

    So, we're declaring the admin user as a member variable (missing docblock btw), but we're not actually storing the camelCased admin user as the class member here. I think I saw this elsewhere in the patch as well.

  10. +++ b/core/modules/search/lib/Drupal/search/Tests/SearchBlockTest.phpundefined
    @@ -27,31 +29,28 @@ public static function getInfo() {
    -    // Set the block to a region to confirm block is available.
    -    $edit = array();
    -    $edit['blocks[search_form][region]'] = 'footer';
    -    $this->drupalPost('admin/structure/block', $edit, t('Save blocks'));
    -    $this->assertText(t('The block settings have been updated.'), 'Block successfully move to footer region.');

    So, coverage being removed here as well, but this totally makes sense; it's not Search's job to test that updating block regions works. I see this repeated in other module's block tests too. Fewer useless assertions++

  11. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/block/block/ShortcutsBlock.phpundefined
    @@ -0,0 +1,34 @@
    +class ShortcutsBlock extends BlockBase {
    +
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build().
    +   */
    +  public function build() {
    +    return array(
    +      shortcut_renderable_links(shortcut_current_displayed_set()),
    +    );
    +  }
    +

    +++ b/core/modules/shortcut/shortcut.moduleundefined
    @@ -184,29 +184,6 @@ function shortcut_theme() {
    -  // Shortcut blocks can't be cached because each menu item can have a custom
    -  // access callback. menu.inc manages its own caching.
    -  $blocks['shortcuts']['cache'] = DRUPAL_NO_CACHE;

    Hm. If the old block was not cacheable, shouldn't this have some cache settings override as well?

Followups

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.phpundefined
    @@ -122,32 +126,36 @@ function testRecentNodeBlock() {
    +    // Enable the "Powered by Drupal" block and test the visibility by node
    +    // type functionality.
    +    $block_name = 'system_powered_by_block';
    +    $block = array(
    +      'machine_name' => $this->randomName(8),
    +      'region' => 'sidebar_first',
    +      'title' => $this->randomName(8),
    +      'visibility[node_type][types][article]' => TRUE,
    +    );
    +    // Set the block to be shown only on node/xx if node is an article.
    +    $this->drupalPost('admin/structure/block/manage/' . $block_name . '/' . $default_theme, $block, t('Save block'));
    +    $this->assertText('The block configuration has been saved.', 'Block was saved');

    What is this doing in testRecentNodeBlock()? It seems completely unrelated. Is it to compare block visibility to a different kind of block than the kinds being tested to make sure Node isn't altering away the wrong blocks? If so there should be a comment saying so. If not it should be moved elsewhere to a method specifically testing per-node-type functionality or removed if such coverage already exists. (The whole test sprawls a bit, but this one especially stood out.)

    I recognize this is just a stand-in for a custom block that used to be there (since custom blocks aren't implemented yet), but we can open a followup.

  2. +++ b/core/modules/node/node.moduleundefined
    @@ -2124,62 +2008,51 @@ function node_form_block_custom_block_delete_submit($form, &$form_state) {
    +    if (arg(0) == 'node' && arg(1) == 'add' && arg(2)) {
    +      $node_add_arg = strtr(arg(2), '-', '_');
    +    }

    Not introduced here (probably dates to Drupal 4.7 or something), but yeacchhhhh. Gahh. Eww. Followup?

  3. +++ b/core/modules/openid/lib/Drupal/openid/Tests/OpenIDTestBase.phpundefined
    @@ -24,21 +24,17 @@
    +    $this->admin_user = $this->drupalCreateUser(array('administer blocks'));
    +    $this->drupalLogin($this->admin_user);
    +
         // Enable user login block.
    -    db_merge('block')
    -      ->key(array(
    -        'module' => 'user',
    -        'delta' => 'login',
    -        'theme' => variable_get('theme_default', 'stark'),
    -      ))
    -      ->fields(array(
    -        'status' => 1,
    -        'weight' => 0,
    -        'region' => 'sidebar_first',
    -        'pages' => '',
    -        'cache' => -1,
    -      ))
    -      ->execute();
    +    $edit = array(
    +      'machine_name' => 'user_login',
    +      'region' => 'sidebar_first',
    +    );
    +    $this->drupalPost('admin/structure/block/manage/user_login_block/stark', $edit, t('Save block'));
    +
    +    $this->drupalLogout();

    Same note as in a previous comment re: a followup to do this programmatically if possible.

  4. +++ b/core/modules/poll/lib/Drupal/poll/Plugin/block/block/PollRecentBlock.phpundefined
    @@ -0,0 +1,81 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::access().
    +   */
    +  public function access() {
    +    if (user_access('access content')) {
    +      // Retrieve the latest poll.
    +      $select = db_select('node', 'n');
    +      $select->join('poll', 'p', 'p.nid = n.nid');
    +      $select->fields('n', array('nid'))
    +        ->condition('n.status', 1)
    +        ->condition('p.active', 1)
    +        ->orderBy('n.created', 'DESC')
    +        ->range(0, 1)
    +        ->addTag('node_access');
    +
    +      $record = $select->execute()->fetchObject();
    +      if ($record) {
    +        $this->record = $record->nid;
    +        return TRUE;
    +      }
    +    }
    +    return FALSE;
    +  }
    +
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build().
    +   */
    +  public function build() {
    +    $poll = node_load($this->record);
    +    if ($poll->nid) {
    +      $poll = poll_block_latest_poll_view($poll);
    +      return array(
    +        $poll->content
    +      );
    +    }
    +    return array();
    +  }

    So, here, PollRecentBlock::build() has a dependency on PollRecentBlock::access(), because the access method also sets what the most recent poll is. This makes sense because it has to be the most recent poll that the user can access--a newer one might be restricted--but it's a little weird for access() to have any responsibilities other than checking access. I'd open a followup to move the "what is the most recent accessible poll" logic into another method and set $this->record... in the constructor maybe? Or are there circumstances where we want to use a PollRecentBlock object without retrieving the record? Is there a different method on the base class that fits?

    +++ b/core/modules/node/lib/Drupal/node/Plugin/block/block/RecentBlock.phpundefined
    @@ -0,0 +1,78 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::access().
    +   */
    +  public function access() {
    +    return user_access('access content');
    +  }
    ...
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build();
    +   */
    +  public function build() {
    +    if ($nodes = node_get_recent($this->configuration['block_count'])) {
    +      return array(
    +        '#theme' => 'node_recent_block',
    +        '#nodes' => $nodes,
    +      );
    +    }
    +    else {
    +      return array(
    +        '#children' => t('No content available.'),
    +      );
    +    }
    +  }
    +
    +}
    diff --git a/core/modules/node/lib/Drupal/node/Plugin/block/block/SyndicateBlock.php b/core/modules/node/lib/Drupal/node/Plugin/block/block/SyndicateBlock.php

    Here, the recent block uses the Node module's API to filter the node list to those to which the user has access, rather than the stuff that poll is doing later on (see below). Polls are nodes; why is poll running a special query rather than using the Node module API?

    Looking into this can be a followup.

Code style crap

  1. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockFunctionalTest.phpundefined
    @@ -12,6 +12,9 @@
    +  protected $adminUser;
    +  protected $webUser;

    @@ -31,42 +34,44 @@ function setUp() {
    -    $this->admin_user = $this->drupalCreateUser(array('administer content types', 'administer nodes', 'administer blocks'));
    -    $this->web_user = $this->drupalCreateUser(array('access content', 'create article content'));
    +    $this->adminUser = $this->drupalCreateUser(array('administer content types', 'administer nodes', 'administer blocks'));
    +    $this->webUser = $this->drupalCreateUser(array('access content', 'create article content'));

    One step closer to standards, but these need docblocks still.

  2. +++ b/core/modules/node/lib/Drupal/node/Tests/NodeBlockTest.phpundefined
    @@ -30,20 +30,30 @@ public static function getInfo() {
    -    // Create and login user
    +    // Create and login user.

    If we're correcting this out-of-scope comment, "log in" should also be two words since it's a verb here. :)

Aside, I apologize for making so many long, unconsolidated posts, but this is just too much to go through in one sitting. When I'm done, I promise to reroll the patch with what fixes I can, actually read the issue summary, and post a TLDR of concerns I have once I have the full context of the patch. :) So please don't bother replying to any of this yet.

EclipseGc’s picture

you have nothing to apologize for. I apologize for the size of the patch :-(

xjm’s picture

Alright, so I've now reviewed everything in the patch except the Block module. Kind of like nibbling 250K of crust off a patch sandwich before eating it.

  1. +++ b/core/lib/Drupal/Core/Plugin/Mapper/ConfigMapper.phpundefined
    @@ -0,0 +1,56 @@
    +/**
    + * Retrieves plugin instances from the configuration system.
    + */
    +class ConfigMapper implements MapperInterface {

    This seems like another nice prerequisite thing that could go in its own patch and should have its own test coverage, like the plugin UI stuff.

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/FeedBlock.phpundefined
    @@ -0,0 +1,51 @@
    +/**
    + * Provides block plugin definitions for aggregator feeds.
    + */
    +class FeedBlock implements DerivativeInterface {

    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/FeedBlock.phpundefined
    @@ -0,0 +1,85 @@
    +/**
    + * Provides an 'Aggregator feed' block.
    + *
    + * @Plugin(
    + *   id = "aggregator_feed_block",
    + *   subject = @Translation("Aggregator feed"),
    + *   module = "aggregator",
    + *   derivative = "Drupal\aggregator\Plugin\Derivative\FeedBlock"
    + * )
    + */
    +class FeedBlock extends BlockBase {

    Okay so I still don't quite grok derivatives. I'll try to remedy that before I review the rest of the patch.

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/CategoryBlock.phpundefined
    @@ -0,0 +1,86 @@
    +      $result = db_query_range('SELECT i.* FROM {aggregator_category_item} ci LEFT JOIN {aggregator_item} i ON ci.iid = i.iid WHERE ci.cid = :cid ORDER BY i.timestamp DESC, i.iid DESC', 0, $this->configuration['block_count'], array(':cid' => $category->cid));

    iiiiiiiiii-d!

  4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/FeedBlock.phpundefined
    @@ -0,0 +1,85 @@
    +    // Plugin IDs look something like this: aggregator_feed_block:1.
    +    list(, $id) = explode(':', $this->getPluginId());

    Yay, a freaking comment describing this line! Finally! However, since we repeat this line of code 2000 times and the other 1999 don't have this, add a method to the plugin API or something. I've said this a few times by now I think. :)

  5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/block/block/FeedBlock.phpundefined
    @@ -0,0 +1,85 @@
    +    if ($feed = db_query('SELECT fid, title, block FROM {aggregator_feed} WHERE block <> 0 AND fid = :fid', array(':fid' => $id))->fetchObject()) {
    +      $result = db_query_range("SELECT * FROM {aggregator_item} WHERE fid = :fid ORDER BY timestamp DESC, iid DESC", 0, $this->configuration['block_count'], array(':fid' => $id));
    +      $read_more = theme('more_link', array('url' => 'aggregator/sources/' . $feed->fid, 'title' => t("View this feed's recent news.")));

    So, just an observation now that I've read every module's block integration. There seems to be a lot of variation on where the business logic resides for what get built in build(). In some cases, as here, queries happen directly in the method. In others, the method calls the module's procedural API to get the data. In one case (Poll), the logic is in the access method. Most memorably of all, in Comment, the logic is in a theme function.

    It's definitely well beyond the reach of this already-gargantuan patch to sort that out, but we should probably establish some best practices around this, and then later in the release cycle we can clean up core so it serves as a good model for contrib.

    On the other hand, maybe part of the answer is "convert as many as possible to Views block displays". :) VDC has mostly focused on page listings up to this point, but there are a LOT of blocks that are ordered, filtered content lists, plain and simple.

  6. +++ b/core/modules/book/lib/Drupal/book/Plugin/block/block/BookNavigationBlock.phpundefined
    @@ -0,0 +1,120 @@
    +    elseif ($current_bid) {
    +      // Only display this block when the user is browsing a book.
    +      $select = db_select('node', 'n')
    +        ->fields('n', array('title'))
    +        ->condition('n.nid', $node->book['bid'])
    +        ->addTag('node_access');
    +      $title = $select->execute()->fetchField();
    +      // Only show the block if the user has view access for the top-level node.
    +      if ($title) {
    +        $tree = menu_tree_all_data($node->book['menu_name'], $node->book);
    +        // There should only be one element at the top level.
    +        $data = array_shift($tree);
    +        $below = menu_tree_output($data['below']);
    +        if (!empty($below)) {
    +          return array(
    +            '#title' => theme('book_title_link', array('link' => $data['link'])),
    +            $below,
    +          );
    +        }
    +      }
    +    }
    +    return array();

    Yet another variation of implementing node access for a node-related block. This class doesn't override the access callback at all. Shouldn't it at least check "access content"? At least, the others do.

  7. +++ b/core/modules/menu/lib/Drupal/menu/Plugin/block/block/MenuBlock.phpundefined
    @@ -0,0 +1,34 @@
    +    list($plugin, $menu) = explode(':', $this->getPluginId());

    There it is again.

  8. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
    @@ -94,9 +89,10 @@ function testRecentCommentBlock() {
    -    $this->drupalPost('admin/structure/block/manage/comment/recent/configure', $block, t('Save block'));
    +
    +    $this->drupalPost("admin/structure/block/manage/plugin.core.block.$current_theme.$machine_name/$current_theme/configure", $block, t('Save block'));

    Yeah okay, we are exposing plugin.core.block in URLs too? More motivation to change the prefix. The word "plugin" shouldn't show up for the user anywhere.

  9. +++ b/core/modules/forum/forum.moduleundefined
    @@ -641,81 +641,7 @@ function forum_form_node_form_alter(&$form, &$form_state, $form_id) {
    -/**
    - * Render API callback: Lists nodes based on the element's #query property.
    - *
    - * This function can be used as a #pre_render callback.
    + * A #pre_render callback. Lists nodes based on the element's #query property.
      *
      * @see forum_block_view()

    Why are we undoing some poor novice's doc cleanup here? This is now changing the docblock away from the correct standard to an incorrect format. Reference: http://drupal.org/node/1354#render

    I'm not putting this under "code style crap" because either someone went out of their way to make an out-of-scope change from something right to something wrong, or screwed up a merge. :P

  10. +++ b/core/modules/forum/lib/Drupal/forum/Plugin/block/block/ForumBlockBase.phpundefined
    @@ -0,0 +1,57 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::access().
    +   */
    +  public function access() {
    +    return user_access('access content');

    +++ b/core/modules/forum/lib/Drupal/forum/Plugin/block/block/NewBlock.phpundefined
    @@ -0,0 +1,40 @@
    +class NewBlock extends ForumBlockBase {
    +
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build().
    +   */
    +  public function build() {
    +    $query = db_select('forum_index', 'f')
    +      ->fields('f')
    +      ->addTag('node_access')
    +      ->addMetaData('base_table', 'forum_index')
    +      ->orderBy('f.created', 'DESC')
    +      ->range(0, $this->configuration['block_count']);
    +
    +    return array(
    +      drupal_render_cache_by_query($query, 'forum_block_view'),
    +    );
    +  }
    +

    So forum does this, instead of what poll does. Note both the fact that the access callback is just user_access(), and the clever render/query caching.

  11. +++ b/core/modules/menu/lib/Drupal/menu/Plugin/Derivative/MenuBlock.phpundefined
    @@ -0,0 +1,29 @@
    +/**
    + * Provides block plugin definitions for custom menus.
    + */
    +class MenuBlock extends SystemMenuBlock {
    +
    +  /**
    +   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinitions().
    +   */
    +  public function getDerivativeDefinitions(array $base_plugin_definition) {
    +    foreach (menu_get_menus(FALSE) as $menu => $name) {
    +      $this->derivatives[$menu] = $base_plugin_definition;
    +      $this->derivatives[$menu]['subject'] = t('Menu: @menu', array('@menu' => $name));
    +      $this->derivatives[$menu]['cache'] = DRUPAL_NO_CACHE;
    +    }
    +    return $this->derivatives;
    +  }

    +++ b/core/modules/menu/lib/Drupal/menu/Plugin/block/block/MenuBlock.phpundefined
    @@ -0,0 +1,34 @@
    +/**
    + * Provides a generic Menu block.
    + *
    + * @Plugin(
    + *   id = "menu_menu_block",
    + *   subject = @Translation("Menu"),
    + *   module = "menu",
    + *   derivative = "Drupal\menu\Plugin\Derivative\MenuBlock"
    + * )
    + */
    +class MenuBlock extends SystemMenuBlock {

    So module-provided menus use the main class, and user-created custom menus are derivatives?

Followups

  1. +++ b/core/lib/Drupal/Component/Plugin/Discovery/DerivativeDiscoveryDecorator.phpundefined
    @@ -65,6 +65,11 @@ public function getDefinitions() {
    +      // @todo Remove this check once http://drupal.org/node/1780396 is resolved.
    +      if (isset($plugin_definition['module']) && !module_exists($plugin_definition['module'])) {
    +        continue;
    +      }

    Is there a point in the summary of that issue referencing this? If not, let's add that.

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/ImportOpmlTest.phpundefined
    @@ -16,6 +24,13 @@ public static function getInfo() {
    +    $web_user = $this->drupalCreateUser(array('administer news feeds', 'access news feeds', 'create article content', 'administer blocks'));
    +    $this->drupalLogin($web_user);

    @@ -30,6 +45,17 @@ function openImportForm() {

    +    // Enable the help block.
    +    $block_id = 'system_help_block';
    +    $default_theme = variable_get('theme_default', 'stark');
    +    $block = array(
    +      'title' => $this->randomName(8),
    +      'machine_name' => $this->randomName(8),
    +      'region' => 'help',
    +    );
    +    $this->drupalPost('admin/structure/block/manage/' . $block_id . '/' . $default_theme, $block, t('Save block'));
    +    $this->assertText(t('The block configuration has been saved.'), '"Help" block enabled');
    ...
         $this->assertText('A single OPML document may contain a collection of many feeds.', 'Found OPML help text.');

    What on earth is this doing here?

    Edit: Oh. I see in the context lines that it's asserting the presence of the help text later. How did that ever work, then?

    Also, this is another case where it would be better to enable the block programmatically rather than through the UI.

    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.phpundefined
    @@ -19,7 +19,7 @@
    -  public static $modules = array('node', 'block', 'aggregator', 'aggregator_test');
    +  public static $modules = array('node', 'aggregator', 'aggregator_test');

    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/ImportOpmlTest.phpundefined
    @@ -8,6 +8,14 @@
    +
    +  /**
    +   * Modules to enable.
    +   *
    +   * @var array
    +   */
    +  public static $modules = array('block');

    Okay, that explains that part at least. +1.

  3. +++ b/core/modules/comment/comment.moduleundefined
    @@ -186,7 +186,7 @@ function comment_field_extra_fields() {
    function comment_theme() {
       return array(
         'comment_block' => array(
    -      'variables' => array(),
    +      'variables' => array('number' => NULL),

    @@ -593,9 +548,9 @@ function comment_new_page_count($num_comments, $new_replies, Node $node) {
    -function theme_comment_block() {
    +function theme_comment_block($variables) {
    ...
    -  $number = variable_get('comment_block_count', 10);
    +  $number = $variables['number'];

    +++ b/core/modules/comment/lib/Drupal/comment/Plugin/block/block/RecentBlock.phpundefined
    @@ -0,0 +1,71 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::build();
    +   */
    +  public function build() {
    +    return array(
    +      '#theme' => 'comment_block',
    +      '#number' => $this->configuration['block_count'],
    +    );

    Out of scope, but it seems way totally wrong to me for theme_comment_block() to be not only theming but calling the API function that queries the database to retrieve the comments. The block should get its own data, or the comment module should give it its data, or something, but it absolutely shouldn't be in a theme function... Followup, and/or check whether #731724: Convert comment settings into a field to make them work with CMI and non-node entities is taking care of un-wtfing this.

  4. +++ b/core/modules/filter/lib/Drupal/filter/Tests/FilterHooksTest.phpundefined
    @@ -19,7 +19,7 @@ class FilterHooksTest extends WebTestBase {
    -  public static $modules = array('block', 'filter_test');
    +  public static $modules = array('node', 'filter_test');

    @@ -42,6 +40,14 @@ function setUp() {
       function testFilterHooks() {
    +    // Create content type, with underscores.
    +    $type_name = 'test_' . strtolower($this->randomName());
    +    $type = $this->drupalCreateContentType(array('name' => $type_name, 'type' => $type_name));
    +    $node_permission = "create $type_name content";
    +
    +    $admin_user = $this->drupalCreateUser(array('administer filters', 'administer nodes', $node_permission));
    +    $this->drupalLogin($admin_user);

    @@ -61,19 +67,16 @@ function testFilterHooks() {
    -    // Add a new custom block.
    -    $custom_block = array();
    -    $custom_block['info'] = $this->randomName(8);
    -    $custom_block['title'] = $this->randomName(8);
    -    $custom_block['body[value]'] = $this->randomName(32);
         // Use the format created.
    -    $custom_block['body[format]'] = $format_id;
    -    $this->drupalPost('admin/structure/block/add', $custom_block, t('Save block'));
    -    $this->assertText(t('The block has been created.'), 'New block successfully created.');
    -
    -    // Verify the new block is in the database.
    -    $bid = db_query("SELECT bid FROM {block_custom} WHERE info = :info", array(':info' => $custom_block['info']))->fetchField();
    -    $this->assertNotNull($bid, 'New block found in database');
    +    $language_not_specified = LANGUAGE_NOT_SPECIFIED;
    +    $title = $this->randomName(8);
    +    $edit = array(
    +      "title" => $title,
    +      "body[$language_not_specified][0][value]" => $this->randomName(32),
    +      "body[$language_not_specified][0][format]" => $format_id,
    +    );
    +    $this->drupalPost("node/add/{$type->type}", $edit, t('Save'));
    +    $this->assertText(t('@type @title has been created.', array('@type' => $type_name, '@title' => $title)), 'New node successfully created.');

    This is changing what this test does, I gather because there are no custom blocks and the actual thing with the filtered text is peripheral to the test... but perhaps we should use a test entity instead, and does it matter that this was in fact testing text filters on non- entities? And what's the purpose of this in testFilterHooks() anyway? To ensure that the text format can be disabled even when it's used on a node? There should be a comment to that effect (I realize it's the same in the existing test). Can be a followup.

Code style crap

  1. +++ b/core/modules/forum/lib/Drupal/forum/Tests/ForumBlockTest.phpundefined
    @@ -87,16 +95,12 @@ function testNewForumTopicsBlock() {
    +  /**
    +   * Test the "Active forum topics" block.
    +   */
    +  public function testActiveForumTopicsBlock() {

    Tests.

xjm’s picture

StatusFileSize
new3.55 KB
new19.6 KB
new18.18 KB
new402.51 KB
FAILED: [[SimpleTest]]: [MySQL] 49,123 pass(es), 15 fail(s), and 0 exception(s).
[ View ]

I cleaned up all the code style stuff from the above, and added some @todo for critical documentation.

I also had to rebase. Bunch of merge conflicts, some from #1843420: Change block test variables - block_test_cache and block_test_content to state system, some from #1849004: One Service Container To Rule Them All, and some in block.admin.inc that I didn't track down but it seemed obvious we wanted whatever was in this branch.

CONFLICT (content): Merge conflict in core/modules/block/tests/block_test.module
Removing core/modules/block/lib/Drupal/block/Tests/BlockUserAccountSettingsTest.php
Auto-merging core/modules/block/lib/Drupal/block/Tests/BlockTest.php
CONFLICT (content): Merge conflict in core/modules/block/lib/Drupal/block/Tests/BlockTest.php
Auto-merging core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
CONFLICT (content): Merge conflict in core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php
Auto-merging core/modules/block/lib/Drupal/block/Tests/BlockCacheTest.php
Auto-merging core/modules/block/block.admin.inc
CONFLICT (content): Merge conflict in core/modules/block/block.admin.inc
Auto-merging core/includes/bootstrap.inc
CONFLICT (content): Merge conflict in core/includes/bootstrap.inc

I didn't push my rebase yet in case I screwed it up.

Edit: merge-diff.txt is nonsense; ignore it.

xjm’s picture

The way I resolved the conflicts is: in bootstrap.inc, use 8.x; in core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.php, combine both to switch the variable to state(), and in everything else, use the sandbox.

tim.plunkett’s picture

@xjm can you push your rebased branch up as a new branch? Thanks!

xjm’s picture

Status:Needs review» Needs work

@tim.plunkett I was waiting for tests to pass and they didn't, so I'm afeared I broke something. :) Edit: looks like BlockCacheTest in particular.

Edit: It's in 1535868-blocks-rebase.

Edit: Oh, I misread; BlockCacheTest did not have a conflict. So maybe it's not my fault.

xjm’s picture

Assigned:xjm» Unassigned

Unassigning for now in case someone who's already working on this knows right off how to fix the test failures. If you do update the patch, please for the love of Drupal include an interdiff. :)

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new634 bytes
new402.51 KB
FAILED: [[SimpleTest]]: [MySQL] 49,100 pass(es), 0 fail(s), and 200 exception(s).
[ View ]

I think this should fix the tests (conflict from #1843420: Change block test variables - block_test_cache and block_test_content to state system). Once it passes, I'll push up all the changes to the branch.

tim.plunkett’s picture

StatusFileSize
new2.1 KB
new404.61 KB
PASSED: [[SimpleTest]]: [MySQL] 49,101 pass(es).
[ View ]

It seems those lines in the views tests weren't needed. This passed locally.

xjm’s picture

Assigned:Unassigned» xjm

Okay, round 2 starts today.

tim.plunkett’s picture

The 1535868-blocks branch has been updated and rebased!
It reflects the patch in #262 and current HEAD as of this comment.

xjm’s picture

I went over all the changes to the block module's test coverage really closely to make sure we're not regressing the coverage. Mostly just style questions, though there's some gaps in the test coverage.

  1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -45,156 +43,69 @@ public function testLanguageBlockVisibility() {
    -    // Create a new block.
    -    $info_name = $this->randomString(10);
    -    $body = '';
    -    for ($i = 0; $i <= 100; $i++) {
    -      $body .= chr(rand(97, 122));
    -    }

    This is weird enough that I blamed it to make sure we weren't removing some unexplained coverage for some edge-case bug. It dates to #135464-55: Add language options to block visibility where @Artusamak said: "Rewritting the body generation." Apparently it was to correct a test failure in comment #52 on that issue?, but I examined the diffs and it seems superfluous to the fix. So, long story short, deleting it is good. :)

  2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -45,156 +43,69 @@ public function testLanguageBlockVisibility() {
    +    // Check that we have the language in config fter saving the setting.
    +    $config = config('plugin.core.block.' . $default_theme . '.language_block_test');
    +    $setting = $config->get('visibility.language.langcodes.fr');
    +    $this->assertFalse('fr' === $setting, 'Language was not set in the block config.');

    This is doing the opposite of what the comment says. Furthermore, the assertion is also likely to give a false pass with a malformed result or changed result format; can we assert that $setting is empty instead? (The previous coverage did not have this flaw, which is why I'm not considering this a followup.) Also typo: "fter".

    +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -45,156 +43,69 @@ public function testLanguageBlockVisibility() {
    +    // Check that we have the language in config fter saving the setting.

    Same typo again, "fter".

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
    @@ -7,13 +7,20 @@
    -class BlockTemplateSuggestionsUnitTest extends UnitTestBase {
    +class BlockTemplateSuggestionsUnitTest extends WebTestBase {

    @@ -26,31 +33,23 @@ public static function getInfo() {
    +    $block = drupal_container()->get('plugin.manager.block')->createInstance('system_menu_block:menu-admin', $data);

    Sad panda. Is it possible to use DrupalUnitTestBase instead? How did this work before with a unit test that we have to switch it now?

    Edit: is it this container check?

  4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
    @@ -26,31 +33,23 @@ public static function getInfo() {
    +    // Define a block with a derivative to be preprocessed, which includes both
    +    // an underscore (not transformed) and a hyphen (transformed to underscore),
    +    // and generates possibilities for each level of derivative.
    ...
    +    $variables['content_attributes']['class'][] = 'test-class';
    +    template_preprocess_block($variables);
    +    $this->assertEqual($variables['theme_hook_suggestions'], array('block__footer', 'block__system', 'block__system_menu_block', 'block__system_menu_block__menu_admin'));
    +    $this->assertEqual($variables['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');

    Oh boy. I guess this hints at an answer to my question a couple days ago? Also I don't know what this means. The previous comment, "Hyphens should be replaced with underscores," was pretty clear. How is the expected behavior changing, and what functionality are we testing? I think the main problem is the word "derivative".

    Also:

    +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
    @@ -26,31 +33,23 @@ public static function getInfo() {
    -    // Define block delta with underscore to be preprocessed
    -    $block1 = new stdClass();
    -    $block1->module = 'block';
    -    $block1->delta = 'underscore_test';
    -    $block1->region = 'footer';
    -    $variables1 = array();
    -    $variables1['elements']['#block'] = $block1;
    -    $variables1['elements']['#children'] = '';
    -    template_preprocess_block($variables1);
    -    $this->assertEqual($variables1['theme_hook_suggestions'], array('block__footer', 'block__block', 'block__block__underscore_test'), 'Found expected block suggestions for delta with underscore');
    -
    -    // Define block delta with hyphens to be preprocessed. Hyphens should be
    -    // replaced with underscores.
    -    $block2 = new stdClass();
    -    $block2->module = 'block';
    -    $block2->delta = 'hyphen-test';
    -    $block2->region = 'footer';
    -    $variables2 = array();
    -    $variables2['elements']['#block'] = $block2;
    -    $variables2['elements']['#children'] = '';
    ...
    -    $variables2['content_attributes']['class'][] = 'test-class';
    -    template_preprocess_block($variables2);
    -    $this->assertEqual($variables2['theme_hook_suggestions'], array('block__footer', 'block__block', 'block__block__hyphen_test'), 'Hyphens (-) in block delta were replaced by underscore (_)');
    -    // Test that the default class and added class are available.
    -    $this->assertEqual($variables2['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');
    +    $variables['content_attributes']['class'][] = 'test-class';
    +    template_preprocess_block($variables);
    +    $this->assertEqual($variables['theme_hook_suggestions'], array('block__footer', 'block__system', 'block__system_menu_block', 'block__system_menu_block__menu_admin'));
    +    $this->assertEqual($variables['content_attributes']['class'], array('test-class', 'content'), 'Default .content class added to block content_attributes_array');

    I have read the assertions three times and I don't understand how we're covering all the cases that were covered before.

  5. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -63,19 +63,30 @@ function setUp() {
    +    $this->assertLink(t('Add Custom Block'));

    Is the actual case of the link? It should be sentence-cased...

    Also, wait what, I thought the custom block functionality wasn't reimplemented yet based on other parts of the patch that remove references to it everywhere.

  6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -267,76 +261,27 @@ function testBlockVisibilityListedEmpty() {
       /**
    -   * Test user customization of block visibility.
    -   */
    -  function testBlockVisibilityPerUser() {

    +++ /dev/nullundefined
    @@ -1,45 +0,0 @@
    -  /**
    -   * Tests that the personalized block is shown.
    -   */
    -  function testAccountSettingsPage() {
    -    $this->drupalGet('admin/config/people/accounts/fields');
    -    $this->assertText(t('Personalize blocks'), 'Personalized block is present.');
    -  }

    Was this feature removed, or has the test coverage for it just gone elsewhere?

  7. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -45,156 +43,69 @@ public function testLanguageBlockVisibility() {
    -  /**
    -   * Tests if the visibility settings are removed if the block is deleted.
    -   */
    -  public function testLanguageBlockVisibilityBlockDelete() {

    Another place that an entire test is getting removed. Presumably related to the lack of custom blocks, but we'll want to make sure this coverage gets added there. We should file a followup to make sure that happens.

  8. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -267,76 +261,27 @@ function testBlockVisibilityListedEmpty() {
    +    $block['theme'] = variable_get('theme_default', 'stark');

    Let's keep an eye on #1829224: Convert the 'theme_default' variable to CMI for this (appears elsewhere in the patch as well). Actually we probably don't need to, because the tests will break if that goes in before this, and then we just do a string replace. Never mind. :)

  9. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.phpundefined
    @@ -0,0 +1,46 @@
    +  /**
    +   * Test block visibility.
    +   */
    +  function testBlockVisibility() {
    +  }
    +}

    Um, this seems to be empty?

    Also, "tests", and there should be a newline between class curlies and the members. But mostly it would be good to actually have test coverage for, like. Basic functionality.

Followups

  1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -345,25 +290,17 @@ function testBlock() {
    +    $xpath = $this->buildXPathQuery('//div[@id=:id]/*', array(':id' => 'block-' . strtr(strtolower($block['machine_name']), '-', '_')));

    Yay xpath! Could we get an inline comment here illustrating the expected pattern? (I realize this was an existing assertion but it's gotten less readable.)

  2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -345,25 +290,17 @@ function testBlock() {
    +    $this->drupalGet('node');

    Are we using this to simply check that the block appears on all pages? If so we should use the test page to decouple this from node (see #1811804: Provide a test page testing module).

  3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -267,76 +261,27 @@ function testBlockVisibilityListedEmpty() {
    +    // Select the 'Powered by Drupal' block to be configured and moved.
    ...
    +    $block['id'] = 'system_powered_by_block';

    Ideally we'd use a test block than an actual block in all these cases where we're not actually testing this block itself, but this is fine for now. Followup?

Code style crap

  1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
    @@ -26,31 +33,23 @@ public static function getInfo() {
    +    // $block3 = block_load('system_powered_by_block', $data3);

    Why is this here?

  2. +++ b/core/modules/block/templates/block.tpl.phpundefined
    @@ -41,7 +41,11 @@
    +<?php if (isset($block_html_id)) { ?>
    <div id="<?php print $block_html_id; ?>" <?php print $attributes; ?>>
    +<?php } else { ?>
    +<div <?php print $attributes; ?>>
    +<?php } ?>

    This violates our coding standards; don't we usually use the <?php if (foo): ?> syntax in templates?

  3. +++ b/core/modules/block/tests/lib/Drupal/block_test/Plugin/block/block/TestHtmlIdBlock.phpundefined
    @@ -0,0 +1,24 @@
    + *   subject = @Translation("Test block html id"),

    This surely wasn't introduced here, but "HTML" and "ID" should be capitalized. Not that the title of a testing module block matters really. :)

  4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.phpundefined
    @@ -0,0 +1,46 @@
    +  }
    +  function setUp() {

    Missing newline.

  5. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockUiTest.phpundefined
    @@ -0,0 +1,46 @@
    + * Definition of Drupal\block\Tests\BlockTest.
    ...
    +class BlockUiTest extends WebTestBase {

    Inaccurate docblock for the file, and missing docblock for the class.

    Also, blah blah contains preceding backslash etc., and I think it's supposed to be BlockUITest per our coding standards (not Ui).

  6. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -267,76 +261,27 @@ function testBlockVisibilityListedEmpty() {
    +    $this->assertEqual($config['subject'], $block['title'], t('Stored block title found.'));

    @@ -345,25 +290,17 @@ function testBlock() {
    +    $this->assertNoFieldByXPath($xpath, FALSE, t('Block found in no regions.'));

    @@ -406,19 +343,31 @@ function testBlockRehash() {
    +    $this->assertEqual($config['cache'], DRUPAL_CACHE_PER_ROLE, t('Test block cache mode defaults to DRUPAL_CACHE_PER_ROLE.'));
    ...
    +    $this->assertEqual($config['cache'], DRUPAL_NO_CACHE, t("Test block's database entry updated to DRUPAL_NO_CACHE."));

    The assertion message texts should not be translated. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

  7. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTemplateSuggestionsUnitTest.phpundefined
    @@ -26,31 +33,23 @@ public static function getInfo() {
    +
    +    $data = array(
    +      'region' => 'footer',
    +    );
    +

    Why the extra whitespace?

  8. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockHiddenRegionTest.phpundefined
    @@ -14,6 +14,7 @@
    +  protected $admin_user;

    +++ b/core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.phpundefined
    @@ -14,6 +14,8 @@
    +  protected $adminUser;

    +++ b/core/modules/block/lib/Drupal/block/Tests/BlockLanguageTest.phpundefined
    @@ -14,6 +14,7 @@
    +  protected $adminUser;

    Needs docblock, and space between it and the next docblock.

  9. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockHtmlIdTest.phpundefined
    @@ -50,6 +55,6 @@ function setUp() {
    +    $this->assertRaw('id="block-test-id-block"', 'HTML id for test block is valid.');

    Not introduced here, but ID should be capitalized.

Inane observations

  • +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -267,76 +261,27 @@ function testBlockVisibilityListedEmpty() {
    +    $this->removeDefaultBlocks();

    Handy.

  • +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -228,31 +232,21 @@ function testBlockVisibility() {
    +    $this->assertText('The block configuration has been saved.', 'Block was saved');

    The overridden assertion message is kinda superfluous here (and elsewhere), but I don't care quite enough to even suggest a followup for it. :)

  • +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -92,68 +103,70 @@ function testCustomBlock() {
    +    $this->drupalGet(NULL);

    NULL and not an empty string? Kinda weird.

  • +++ b/core/modules/block/lib/Drupal/block/Tests/BlockTest.phpundefined
    @@ -345,25 +290,17 @@ function testBlock() {
    -    // For convenience of developers, put the Administration menu block back.

    lolwhat

Next is the actual block plugin stuff, finally. Fun and more fun!

xjm’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/Plugin/Derivative/BlockPluginUI.phpundefined
    @@ -0,0 +1,44 @@
    +/**
    + * Provides block plugin definitions for each theme.
    + */
    +class BlockPluginUI implements DerivativeInterface {

    Okay, this is kind of weird. Is this class completely misnamed? I'd expect something called BlockPluginUI to be building a user interface, not merely providing the per-theme derivatives.

    Aside though, this particular example helps explain to me what derivatives are for, and might be good to add to the handbook docs alongside the menu block example.

  2. +++ b/core/modules/block/lib/Drupal/block/Plugin/Derivative/BlockPluginUI.phpundefined
    @@ -0,0 +1,44 @@
    +  /**
    +   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinition().
    +   */
    +  public function getDerivativeDefinition($derivative_id, array $base_plugin_definition) {
    +    if (!empty($this->derivatives) && !empty($this->derivatives[$derivative_id])) {
    +      return $this->derivatives[$derivative_id];
    +    }
    +    $this->getDerivativeDefinitions($base_plugin_definition);
    +    return $this->derivatives[$derivative_id];
    +  }
    +
    +  /**
    +   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinitions().
    +   */
    +  public function getDerivativeDefinitions(array $base_plugin_definition) {
    +    foreach (list_themes() as $key => $theme) {
    +      $this->derivatives[$key] = $base_plugin_definition;
    +    }
    +    return $this->derivatives;

    As I stated in a previous review, one inline comment or additional line in the docblock would be nice in cases like this, even though the code is quite simple. It helps learnability to tell someone in words what the bit of code does before they read it.

  3. +++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
    @@ -0,0 +1,30 @@
    +/**
    + * Manages discovery and instantiation of block plugins.
    + */
    +class BlockManager extends PluginManagerBase {

    What a tiny little important class. I find this both cool and scary. :)

    Even after reading the handbook docs, though, it still takes a bit to get a clear picture of how this fits together with the other classes that form the Block plugin's architecture (and anyway people should be able to find out some of what they need without going to d.o), so I'd recommend more documentation and @see on this class and its friends to help developers understand how they interrelate. Now, I may be a "self-taught hack" with no CS degree, but I think little documentation her would go a long way to helping people understand the new block system and the plugin system generally without having to do the journey of discovery I did in #1675048: DX: D8 Plugin writing. ;)

  4. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    + *   facets = {
    + *     "module" = @Translation("Modules")
    + *   },

    Hm, I thought @Translation did not work on child keys?

  5. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    +    $plugin_definition = $this->getDefinition();
    +    $manager = new $plugin_definition['manager']();
    +    $plugins = $this->excludeDefinitions($manager->getDefinitions());

    This seems like another sort of thing that merits a simple inline comment.

  6. +++ b/core/modules/block/lib/Drupal/block/Plugin/views/display/Block.phpundefined
    @@ -78,7 +78,7 @@ public function execute() {
    -    $info['subject'] = filter_xss_admin($this->view->getTitle());
    +    $info['#title'] = filter_xss_admin($this->view->getTitle());

    This hints at another API change.

Followups

  1. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    +    $form['right']['block'] = array(
    +      '#type' => 'textfield',
    +      '#title' => t('Search'),
    +      '#autocomplete_path' => 'system/autocomplete/' . $this->getPluginId(),
    +    );
    +    $form['right']['submit'] = array(
    +      '#type' => 'submit',
    +      '#value' => t('Next'),
    +    );
    ...
    +      $form['left']['plugin_library'] = array(
    +        '#theme' => 'table',
    +        '#header' => $this->tableHeader(),
    +        '#rows' => $rows,

    Minor, but don't we prefer not using "right" and "left" in machine names, since they are typically reversed for RTL languages? Could be a followup, especially if we're planning on iterating on this interface.

  2. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    +      foreach ($facets as $group => $values) {
    +        $form['right'][$group] = array(
    +          '#theme' => 'links',
    +          '#heading' => array(
    +            'text' => $plugin_definition['facets'][$group],
    +            'level' => 'h3',
    +          ),
    +          '#links' => $values,
    +        );
    ...
    +  protected function facetLink($facet, $display_plugin_id, array $display_plugin_definition) {
    ...
    +  protected function facetCompare($facet, $display_plugin_definition) {
    ...
    +  protected function allPluginsUrl($display_plugin_id, $display_plugin_definition) {

    So I asked @tim.plunkett why all this facet stuff is in Block rather than the system Plugin UI stuff I already reviewed. He said that he moved it into block because a lot of it is block-specific, and that it's probably better to abstract when we have a second usecase that'd dictate how it should be abstracted, which makes sense to me.

    I'm not entirely sold on the whole facet UI pattern, either generally or as it appears here. I think it's a pretty complicated interface pattern for core (...er, please pretend for a second I'm not involved with Views). ;) I personally even have a little trouble understanding the interface when I look at it.

    Bojhan came up with a design to solve a similar UI task in Views that I think is a reasonable first step:
    #1832862: Users feel overwhelmed by handler listings In that mockup (and the current Views UI for that matter), there's similarly a search for a particular use of a plugin, a filter by a module facet, and a large result set. If we go that direction for Views, we might want to consider something like that here, so that people recognize the same interaction, or otherwise standardize the two somehow. (There's some third usecase that's similar that we discussed at BADCamp, something layout-related I think.)

    All that said, I do NOT want to block this issue on a UI discussion. I am strongly in favor of putting in a rough, functional UI and then iterating on it.

    The one thing that does trouble me for the current patch is the lack of explicit test coverage. A couple parts of the UI are tested implicitly in almost every module's block tests, but the rest is untested, which makes me nervous, especially given that there's a pretty complicated UI brewing here.

Code style crap

  1. +++ b/core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.phpundefined
    @@ -0,0 +1,30 @@
    +  public function __construct() {
    +    $this->discovery = new AlterDecorator(new DerivativeDiscoveryDecorator(new AnnotatedClassDiscovery('block', 'block')), 'block');
    +    $this->factory = new DefaultFactory($this);
    +    $this->mapper = new ConfigMapper($this);
    +  }

    Block, block, block, Drupal chicken. Tim showed me this example in Views where we do not use the (admittedly more numerous) decorators all inline:
    http://drupalcode.org/project/drupal.git/blob?f=core/modules/views/lib/D...

  2. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    + * This plugin provides modifications to the system provided base class. Its
    + * purpose is to provide an overrideable user interface for block selection,
    + * configuration and placement.
    ...
    +   * Provides individually formatted links for the faceting that happens within
    +   * the user interface. Since this is a faceting style procedure, each plugin
    +   * may be parsed multiple times in order to extract all facets and their
    +   * appropriate labels.
    ...
    +   * Compares a given plugin definition with the selected facet to determine if
    +   * the plugin should be displayed in the user interface.

    Yay documentation! These should have a one-line summary above any extended description paragraphs, though.

  3. +++ b/core/modules/block/lib/Drupal/block/Plugin/system/plugin_ui/BlockPluginUI.phpundefined
    @@ -0,0 +1,205 @@
    + *   id = "block_plugin_ui",
    + *   title = @Translation("Block Library"),
    + *   title_attribute = "subject",
    + *   menu = TRUE,
    + *   module = "block",
    + *   all_plugins = @Translation("All Blocks"),
    + *   link_title = @Translation("Configure block"),
    + *   config_path = "admin/structure/block/manage",
    + *   path = "admin/structure/block/list",
    + *   suffix = "add",
    + *   type = MENU_LOCAL_ACTION,
    + *   manager = "Drupal\block\Plugin\Type\BlockManager",
    + *   derivative = "Drupal\block\Plugin\Derivative\BlockPluginUI",

    We should alphabetize these or something after the standard ones like ID and module. I'm having trouble scanning the list to identify a particular key.

Inane observations

  • +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +interface BlockInterface {

    I made it to the interface!! Finally! I feel like Mario when the princess is finally in this castle, or maybe Frodo at the base of Mount Doom.

    Time for a glass of wine; more later.

xjm’s picture

After talking to @EclipseGc awhile about previous comments, I realized it would be more helpful if I offered examples of the sorts of minor documentation additions that would make the code easier to follow, rather than pointing out over and over again that they were missing, because apparently it wasn't as obvious to Kris as it is to me what was missing. The last couple comments about missing documentation here were before that conversation, because I review code from the bottom up. ;)

  1. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +class CustomBlock extends BlockBase {

    ...Oh. It does still exist? Mmm, why is it being removed everywhere then?

    That said, I do see how it could make a lot of sense to have this in its own module, à la Views UI or Field UI. Not sure about the directory structure. Field UI is on the same level as Field. Views UI is inside Views, but we have a postponed issue somewhere-or-other to move it up to the parent level as well. Thoughts?

    Haven't reviewed the custom block code yet; going to paste my review before my browser crashes or something.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::settingsWrapper().
    +   */
    +  public function settingsWrapper() {
    +    $settings = $this->settings();
    +    $settings += array(
    +      'status' => TRUE,
    +      'cache' => DRUPAL_NO_CACHE,
    +    );
    +    return $settings;
    +  }
    ...
    +
    +  /**
    +   * Implements \Drupal\block\BlockInterface::getConfig().
    +   */
    +  public function getConfig() {
    +    if (empty($this->configuration)) {
    +      $this->configuration = $this->settingsWrapper();

    There are no getConfig() or settingsWrapper() methods on the interface. I just read it like three times. :)

  3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  public function setConfig($key, $value) {
    +    $this->configuration[$key] = $value;
    +  }

    Missing a docblock.

  4. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::configureWrapper().
    +   */
    +  public function configureWrapper($form, &$form_state) {

    Maybe add a docblock paragraph, "Creates a generic configuration form for all block types. Individual block plugins can add elements to this form by implementing BlockInterface::configure()." (That is, if we want to keep this pairing of configure() and configureWrapper().

    Also, this is kind of sprawling. Maybe worth breaking it up a little, as is suggested later in the method.

  5. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::accessWrapper().
    +   */
    +  public function accessWrapper() {

    Add a second sentence to the docblock (after a blank line), something along the lines of: "Adds the user-configured per-role, per-path, and per-language visibility settings to all blocks, and invokes hook_block_access()." Also an @see to hook_block_access().

  6. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::access().
    +   */
    +  public function access() {
    +    return TRUE;
    +  }

    Add an inline comment above the return, something like, "By default, the block is visible unless user-configured rules indicate that it should be hidden." If that's accurate.

  7. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +      '#title' => t('Block machine name'),
    +      '#maxlength' => 64,

    So the block ID (machine name) length is 64. That's a good thing, given these issues both with MyISAM and with config of keys butting up against limits (see my first review comment). Is this length also enforced on save? What about the database; is this the size of the field? This is an important enough restriction that we might want to put it in a constant.

  8. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +    $form += $this->configure($form, $form_state);

    Add an inline comment, "Add specific configuration for this block type." or something along those lines.

  9. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::configureValidateWrapper().
    +   */
    +  public function configureValidateWrapper($form, &$form_state) {
    +    if (empty($form['settings']['machine_name']['#disabled'])) {
    +      if (preg_match('/[^a-zA-Z0-9_]/', $form_state['values']['machine_name'])) {
    +        form_set_error('machine_name', t('Block name must be alphanumeric or underscores only.'));
    +      }
    +      if (in_array('plugin.core.block.' . $form_state['values']['machine_name'], config_get_storage_names_with_prefix('plugin.core.block'))) {
    +        form_set_error('machine_name', t('Block name must be unique.'));
    +      }
    +    }
    +    else {
    +      $config_id = explode('.', $form_state['values']['machine_name']);
    +      $form_state['values']['machine_name'] = array_pop($config_id);
    +    }
    +    if ($form_state['values']['module'] == 'block') {
    +      $custom_block_exists = (bool) db_query_range('SELECT 1 FROM {block_custom} WHERE bid <> :bid AND info = :info', 0, 1, array(
    +        ':bid' => $form_state['values']['delta'],
    +        ':info' => $form_state['values']['info'],
    +      ))->fetchField();
    +      if (empty($form_state['values']['info']) || $custom_block_exists) {
    +        form_set_error('info', t('Ensure that each block description is unique.'));
    +      }
    +    }
    +    $form_state['values']['visibility']['role']['roles'] = array_filter($form_state['values']['visibility']['role']['roles']);
    +    $this->configureValidate($form, $form_state);
    ...
    +  /**
    +   * Implements \Drupal\block\BlockInterface::configureSubmitWrapper().
    +   */
    +  public function configureSubmitWrapper($form, &$form_state) {
    +    if (!form_get_errors()) {
    +      $transaction = db_transaction();
    +      try {
    +        $keys = array(
    +          'visibility' => 'visibility',
    +          'pages' => 'pages',
    +          'custom' => 'custom',
    +          'title' => 'subject',
    +          'module' => 'module',
    +          'region' => 'region',
    +        );
    +        foreach ($keys as $key => $new_key) {
    +          if (isset($form_state['values'][$key])) {
    +            $this->configuration[$new_key] = $form_state['values'][$key];
    +          }
    +        }
    +      }
    +      catch (Exception $e) {
    +        $transaction->rollback();
    +        watchdog_exception('block', $e);
    +        throw $e;
    +      }
    +      if (empty($this->configuration['weight'])) {
    +        $this->configuration['weight'] = 0;
    +      }
    +      drupal_set_message(t('The block configuration has been saved.'));
    +      drupal_flush_all_caches();
    +      $this->configureSubmit($form, $form_state);
    +    }
    +  }

    Penny for a code comment.

  10. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +/**
    + * Interface definition for Block plugins.
    + */
    +interface BlockInterface {

    Bit thin. This would be the place, if any, to wax melodramatic about what block plugins are, and explain the architecture in detail.

    Also, blahblahblah, verbs.

  11. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +  /**
    +   * Indicates whether the block should be shown, based on general criteria.
    +   *
    +   * This method should enforce the general contracts that are generally
    +   * applicable to all blocks: role, path, and page-based visibility settings.
    +   *
    +   * This is the method that is typically called externally, so implementations
    +   * of it should make sure to also call BlockInterface::access() and AND the
    +   * result with their own result to provide the final access determination.
    +   *
    +   * @return bool
    +   */
    +  public function accessWrapper();
    +
    +  /**
    +   * Indicates whether the block should be shown, based on internal criteria.
    +   *
    +   * Blocks with access control that should *always* be applied, regardless of
    +   * user-configured settings, should implement this method with that access
    +   * control logic.
    +   *
    +   * @return bool
    +   */
    +  public function access();

    Okay, yay, documentation. Still don't understand why accessWrapper() has the word wrapper in it. This dichotomy also doesn't exist elsewhere in our access control APIs. Maybe the need for it will become clear when I read the base class, but for now at least it strikes me as an architectural oddity and awkward DX. (Also, "contracts"?)

    Also, minor, the return value should be documented, e.g.: "TRUE if the block should be shown, or FALSE otherwise."

    Edit: So I got up to the base class, and it looks like perhaps the two methods are a way to always add the generic, user-configured visibility settings without having to call parent::access() all the time. Is there a usecase for overriding the base implementation of accessWrapper()? If not, maybe we can change the architecture somewhat to make it clear that access() is the only method individual block plugins should probably care about. I guess the same is sorta true of the form stuff.

  12. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +  /**
    +   * Returns the configuration form elements specific to this block plugin.
    +   *
    +   * @return array $form
    +   */
    +  public function configure($form, &$form_state);
    ...
    +  /**
    +   * Returns the full form for block configuration.
    +   *
    +   * This incorporates both
    +   *
    +   * @return array $form
    +   *   The renderable form array representing the entire configuration form.
    +   */
    +  public function configureWrapper($form, &$form_state);

    Looks like this docblock was unfinished. Also, if it's the "full form for block configuration," then why is it called Wrapper?

  13. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +  /**
    +   * Returns the configuration form elements specific to this block plugin.
    +   *
    +   * @return array $form
    +   */
    +  public function configure($form, &$form_state);

    Needs documentation for the parameters and return value. Also, it would be good to clarify how this is related to the "full form".

  14. +++ b/core/modules/block/lib/Drupal/block/BlockInterface.phpundefined
    @@ -0,0 +1,91 @@
    +  /**
    +   * The validation wrapper for the configuration form.
    +   */
    +  public function configureValidate($form, &$form_state);
    ...
    +  /**
    +   * The submission wrapper for the configuration form.
    +   */
    +  public function configureSubmit($form, &$form_state);
    ...
    +  /**
    +   * The validation for the configuration form.
    +   */
    +  public function configureValidateWrapper($form, &$form_state);
    ...
    +  /**
    +   * The submission for the configuration form.
    +   */
    +  public function configureSubmitWrapper($form, &$form_state);

    So uh. The docblocks for these are the opposite of the functions; the ones that say "wrapper" in the docblocks don't say wrapper in the method name. Also what is a "wrapper" in this context? A wrapper function (and why would we put that on the interface)? A stream wrapper? A markup wrapper for rendering? Tinfoil? Holiday paper? Queen Latifah? Especially on the interface, the docblocks should say what the thing does, not just repeat the method name in sentence form.

    Also, code style things: blah, summaries should start with verbs, yada yada. If they were procedural the standard would dictate that the docblocks be in the format "Form submission handler for foo_bar_form()," but as previously stated this is a brave new world (that has such methods in't) so for now just verb it up. For the same reason, we should probably add parameter documentation for $form and $form_state. Presumably there isn't an expected return value.

Followups

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +    // @todo remove this access check and inject it in some other way. In fact
    +    //   this entire visibility settings section probably needs a separate user
    +    //   interface in the near future.

    Do we have a followup yet for this?

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +        // @todo $language->name is not wrapped with t(), it should be replaced
    +        //   by CMI translation implementation.

    Does this have a followup issue yet?

  3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +      // Check other modules for block access rules.
    +      foreach (module_implements('block_access') as $module) {
    +        if (module_invoke($module, 'block_access', $this) === FALSE) {
    +          return FALSE;
    +        }
    +      }

    This is way out of scope, but core is pretty inconsistent about whether access is additive or subtractive. In this method, and in the hook invocation, it's subtractive. IIRC field access is also subtractive. Node access is additive. Etc.

  4. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +      // @todo This loads the default subject. Is this the right place to do so?
    +      $definition = $this->getDefinition();
    +      if (isset($definition['subject'])) {
    +        $this->configuration += array('subject' => $definition['subject']);
    +      }

    Needs a followup.

  5. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +  /**
    +   * Implements \Drupal\block\BlockInterface::accessWrapper().
    +   */
    +  public function accessWrapper() {

    What would you think of having each kind of check in its own method? (Just on the class, not on the interface.) So then we'd have helpers BlockBase::roleAccess(), BlockBase::pathAccess(), and BlockBase::languageAccess(). Just a thought.

Code style crap

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +/**
    + * An abstract block implementation to be inherited from to take care of many
    + * common block setup tasks.
    + */

    Should have a one-line summary of 80 chars or fewer that starts with a verb, with additional detail in subsequent paragraphs. Maybe something like:

    Defines a standard base block implementation that most blocks plugins will extend.

    This abstract class provides the generic block configuration form, default block settings, and handling for general user-defined block visibility settings.

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBundle.phpundefined
    @@ -0,0 +1,26 @@
    + * Block dependency injection container.

    Verbify.

  3. +++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
    @@ -0,0 +1,437 @@
    +        else {
    +          $page_match = FALSE;
    +        }

    This seems unnecessary, since it is FALSE by default. We could either remove this, or return FALSE here.

xjm’s picture

Alright, reviewed the Custom Block module. Nice to come back down (or climb back up?) to earth a bit. :)

  1. First of all, there should be tests for this module. I think I saw some of them sitting around in the parent module yet, but we should move them here, and add any coverage that's missing.
  2. +++ b/core/modules/block/custom_block/custom_block.admin.incundefined
    @@ -0,0 +1,48 @@
    +      'field' => 'cb.info',

    Huh? That's kind of weird. I don't get what this is.

  3. +++ b/core/modules/block/custom_block/custom_block.admin.incundefined
    @@ -0,0 +1,48 @@
    +  $header = array(t('Administrative Title'), t('Operations'));

    Sentence case. We reserve Capitalized Words for things that are proper nouns, like module names. Operations are sentence case.

  4. +++ b/core/modules/block/custom_block/custom_block.admin.incundefined
    @@ -0,0 +1,48 @@
    +  foreach ($results as $result) {
    +    $options[$result->bid] = array(
    +      array(
    +        l($result->info, 'custom_block/' . $result->bid . '/edit'),
    +      ),
    +      array(
    +        theme('links', array('links' => array(array(
    +            'title' => t('edit'),
    +            'href' => 'custom_block/' . $result->bid . '/edit',
    +            'query' => $destination,
    +          )))
    +        ),
    +      ),
    +    );

    Mmmm. Maybe I'm missing something, but it looks here like we can edit but not delete them? Edit: Unless you can only delete from a button on the edit form? #1834002: Configuration delete operations are all over the place

    This also probably wants to be replaced later with dropbuttons, maybe a list controller or administrative view depending on whether we want to call this content or config.

  5. +++ b/core/modules/block/custom_block/custom_block.admin.incundefined
    @@ -0,0 +1,48 @@
    +    '#empty' => t('No custom blocks are available.'),

    IIRC we usually add the "Add one" link in here for usability. (List controllers do not right now due to an outstanding issue, but since blocks aren't config entities and aren't using the list controller, that doesn't apply here.) ;)

  6. +++ b/core/modules/block/custom_block/custom_block.infoundefined
    @@ -0,0 +1,6 @@
    +description = Allows the ability to create custom blocks through the block interface.

    +++ b/core/modules/block/custom_block/custom_block.moduleundefined
    @@ -0,0 +1,63 @@
    + * Allows the ability to create custom blocks through the block interface.

    This is a bit awkward. How about: "Allows the creation of custom blocks through the user interface."

  7. +++ b/core/modules/block/custom_block/custom_block.installundefined
    @@ -0,0 +1,49 @@
    +<?php
    +
    +/**
    + * @file
    + * Install, update and uninstall functions for the custom block module.
    + */

    Seems to be missing a hook_uninstall() to delete its table and clean up its data.

  8. +++ b/core/modules/block/custom_block/custom_block.moduleundefined
    @@ -0,0 +1,63 @@
    +    // We only need this menu item for the block_plugin_ui derivatives.
    +    if (strpos($plugin_id, 'block_plugin_ui') === 0) {

    Glad that there's a comment. However, translation please?

  9. +++ b/core/modules/block/custom_block/custom_block.moduleundefined
    @@ -0,0 +1,63 @@
    +        'title' => 'Add Custom Block',
    ...
    +        'title' => 'Custom Blocks',

    Aha, found it at last. These should be sentence case.

  10. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
    @@ -0,0 +1,53 @@
    + * Provides block plugin definitions for all custom blocks.
    ...
    +class CustomBlock implements DerivativeInterface {

    +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    + * Provides a generic custom block.
    ...
    +class CustomBlock extends BlockBase {

    Maybe "Retrieves block definitions for all custom blocks" and "Defines a generic custom block type" to make it easier for people to understand the relationship between the plugin and its derivatives?

  11. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
    @@ -0,0 +1,53 @@
    +  /**
    +   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinitions().
    +   */
    +  public function getDerivativeDefinitions(array $base_plugin_definition) {
    +    $results = db_query('SELECT * FROM {block_custom}');

    Add a paragraph to the summary, e.g.: "Retrieves custom block definitions from the database." If that's accurate.

  12. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
    @@ -0,0 +1,53 @@
    +      $this->derivatives[$result->bid]['subject'] = $result->info;

    Okay, this is a little confusing. I thought (based on the plugin's configuration form) that 'info' was the key for the administrative block definition, but he're we're using it for the user facing block title?

  13. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
    @@ -0,0 +1,53 @@
    +      unset($this->derivatives[$result->bid]['custom']);

    Inline comment explaining why we need to unset this would be good.

  14. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::getConfig().
    +   */
    +  public function getConfig() {
    +    $definition = $this->getDefinition();
    +    $this->configuration = parent::getConfig();
    +    $this->configuration['status'] = $definition['settings']['status'];
    +    $this->configuration['info'] = $definition['settings']['info'];
    +    $this->configuration['body'] = $definition['settings']['body'];
    +    $this->configuration['format'] = $definition['settings']['format'];
    +    return $this->configuration;

    Add an additional paragraph to the summary: "Adds the user-defined block body and description for each custom block to the block's configuration." (If that's accurate.)

  15. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +  /**
    +   * Overrides \Drupal\block\BlockBase::configure().
    +   */
    +  public function configure($form, &$form_state) {

    Add a paragraph to the docblock: "Adds body and description fields to the block configuration form."

  16. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +    $form = parent::configure($form, $form_state);
    ...
    +    parent::configureSubmit($form, $form_state);

    So I thought that elsewhere, we have the accessWrapper() and configureWrapper() and whatever to avoid having to call parent::whateverCrap(), but here in this class we're doing so anyway.

Followups

  1. +++ b/core/modules/block/custom_block/custom_block.admin.incundefined
    @@ -0,0 +1,48 @@
    +  $results = db_query('SELECT * FROM {block_custom}');

    Followup, make it an entity of some flavor. Did I say that yet?

  2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Derivative/CustomBlock.phpundefined
    @@ -0,0 +1,53 @@
    +  /**
    +   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinition().
    +   */
    +  public function getDerivativeDefinition($derivative_id, array $base_plugin_definition) {
    +    if (!empty($this->derivatives) && !empty($this->derivatives[$derivative_id])) {
    +      return $this->derivatives[$derivative_id];
    +    }
    +    $this->getDerivativeDefinitions($base_plugin_definition);
    +    return $this->derivatives[$derivative_id];

    I'm starting to feel like I've seen this before. Is this exactly the same as all other implementations of this method, just a wrapper for the multiple getter or retrieving it from the object if the list is already set? Would it make sense to abstract this and provide a BlockDerivativeBase, or is that a completely dumb idea with this architecture? (I honestly have no idea, outside of the programmer knee-jerk "saw the same thing twice, refactor.")

  3. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +    list(, $bid) = explode(':', $this->getPluginId());

    Look, it's my favorite explosion again.

  4. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/block/block/CustomBlock.phpundefined
    @@ -0,0 +1,103 @@
    +    drupal_write_record('block_custom', $block, !is_null($block['bid']) ? array('bid') : array());

    Really, we're calling drupal_write_record() here? So for now at least, custom block storage isn't swappable. Maybe a followup? Maybe custom blocks are entities? (Content entities? Config entities?)

Still have to review the procedural block module code and its API docs. Getting close though!

xjm’s picture

Argh, lost a review post. I'll try to recreate it.

xjm’s picture

  1. +++ b/core/modules/block/block.admin.incundefined
    @@ -267,248 +250,21 @@ function _block_compare($a, $b) {
    +function block_admin_configure($form, &$form_state, $plugin_id, $theme = NULL) {

    Nice. This probably needs a docblock update too. The diff is a little weird. So many deleted lines, yay.

  2. +++ b/core/modules/block/block.admin.incundefined
    @@ -729,43 +307,30 @@ function block_add_block_form_submit($form, &$form_state) {
    +function block_admin_block_delete($form, &$form_state, $plugin_id, $theme) {

    Parameter documentation for this needs an update.

  3. +++ b/core/modules/block/block.admin.incundefined
    @@ -729,43 +307,30 @@ function block_add_block_form_submit($form, &$form_state) {
    +  $block = block_load($plugin_id);
    +  $form['id'] = array('#type' => 'value', '#value' => $plugin_id);
    +  $form['theme'] = array('#type' => 'value', '#value' => $theme);
    +  $definition = $block->getDefinition();
    +  $config = $block->getConfig();
    +  $subject = empty($config['subject']) ? $definition['subject'] : $config['subject'];
    +  $form['subject'] = array('#type' => 'value', '#value' => $subject);
    +
    +  return confirm_form($form, t('Are you sure you want to delete the block %name?', array('%name' => $subject)), 'admin/structure/block', '', t('Delete'), t('Cancel'));
    ...
    /**
    - * Form submission handler for block_custom_block_delete().
    + * Form submission handler for block_admin_block_delete().
      *
    - * @see block_custom_block_delete()
    + * @see block_admin_block_delete()
    ...
    +function block_admin_block_delete_submit($form, &$form_state) {
    +  $config = config($form_state['values']['id']);
    +  $config->delete();
    +  drupal_set_message(t('The block %name has been removed.', array('%name' => $form_state['values']['subject'])));
    +  $form_state['redirect'] = 'admin/structure/block/list/' . $form_state['values']['theme'];

    Hmm. Is this for deleting custom blocks, or removing block instances? We should probably clarify the text and documentation a little in that regard. The form builder also needs to have its parameter documentation updated.

  4. +++ b/core/modules/block/block.api.phpundefined
    @@ -11,355 +11,82 @@
    +function hook_block_view_ID_alter(array &$build, \Drupal\block\BlockInterface $block) {
    +  // This code will only run for a specific block. For example, if ID
    +  // in the function definition above is set to "someid", the code
    +  // will only run on the "someid" block.
    ...
    +  // Change the title of the "someid" block.
    ...
    +function hook_block_view_NAME_alter(array &$build, \Drupal\block\BlockInterface $block) {
    +  // This code will only run for a specific block instance. For example, if NAME
    +  // in the function definition above is set to "someid", the code will only run
    +  // on the "someid" block.
    ...
    +  // Change the title of the "someid" block.
    +  $build['#title'] = t('New title of the block');

    This mostly looks pretty good; however, can we use closer-to-reality examples that will illustrate the difference between NAME and ID is?

    Also, if we're not calling block deltas "deltas" anymore, then that word should also go away elsewhere. (I think I mentioned this before.)

  5. +++ b/core/modules/block/block.installundefined
    @@ -380,6 +344,13 @@ function block_update_8005() {
    /**
    + * Enable the Custom Block module.
    + */
    +function block_update_8006() {
    +  update_module_enable(array('custom_block'));

    Maybe we want to do this conditionally, based on whether or not the site has any custom blocks defined? (This is a question we still need to decide for Views, as well, and possibly worth its own separate followup discussion.)

  6. +++ b/core/modules/block/block.moduleundefined
    @@ -54,16 +54,21 @@ function block_help($path, $arg) {
    +      if (module_exists('custom_block')) {
    +        $output .= '<dt>' . t('Creating custom blocks') . '</dt>';
    +        $output .= '<dd>' . t('Users with the <em>Administer blocks</em> permission can <a href="@block-add">add custom blocks</a>, which are then listed on the <a href="@blocks">Blocks administration page</a>. Once created, custom blocks behave just like default and module-generated blocks.', array('@blocks' => url('admin/structure/block'), '@block-add' => url('admin/structure/block/list/block_plugin_ui:' . variable_get('theme_default', 'stark') . '/add/custom_blocks'))) . '</dd>';

    Should this be in custom_block_help() instead?

  7. +++ b/core/modules/block/block.moduleundefined
    @@ -127,51 +132,40 @@ function block_menu() {
    +  // Block administration is actually tied to theme and plugin definition so
    +  // that the plugin can appropriately attach to this url structure.

    I'm glad there's a comment here, but it's still clear as mud. :) I think what we want to say is something like: "Define an administrative path for each block plugin in each theme" or something. Right? and an example of what they look like in a subsequent comment would be ducky.

  8. +++ b/core/modules/block/block.moduleundefined
    @@ -127,51 +132,40 @@ function block_menu() {
    +    list($plugin_base, $key) = explode(':', $plugin_id);

    By now you know what I'm going to say here.

  9. +++ b/core/modules/block/block.moduleundefined
    @@ -356,23 +302,27 @@ function _block_get_renderable_region($list = array()) {
    +      $key_components = explode('.', $key);
    +      $id = array_pop($key_components);

    @@ -673,20 +406,28 @@ function block_themes_enabled($theme_list) {
    +      $block_config = config($config_id)->get();
    +      $machine_name = explode('.', $config_id);
    +      $machine_name = array_pop($machine_name);
    +      $new_config_id = 'plugin.core.block.' . $theme . '.' . $machine_name;
    +      $new_block = config($new_config_id);

    Seriously. If we keep leaving food out for these string manipulation sequences all over the place without controlling the population with sensibly abstracted class methods or at least the occasional inline comment to keep an eye on them, they're going to overrun our cities, then evolve intelligence, become self-aware, and take their revenge. Do you want to think of what beings who grew out of explode() and array_pop() and strtr() would do to living tissue? Me neither.

    Er. By which I mean, see #1760358: Provide a way to extract the ID from a config object name. :D

  10. +++ b/core/modules/block/block.moduleundefined
    @@ -725,178 +463,43 @@ function block_list($region) {
    + * @param array $conf
    + *   An optional configuration array for creating a block instance from php
    + *   instead of relying on configuration xml.
    ...
    +  if (!$block = $manager->getInstance(array('config' => $plugin_id))) {
    +    $block = $manager->createInstance($plugin_id, $conf);
       }

    Ghosts! XML? :) Also, this is way weird. This function doubles as a load function and as a create function? Do we normally do that? Hmmm... entity_load() doesn't, but Config::load() does.

    All this with block having its own one-off CRUD feels SO D7, but that's followup-land. ;)

  11. +++ b/core/modules/block/block.moduleundefined
    @@ -909,50 +512,50 @@ function block_block_list_alter(&$blocks) {
    +function block_build($block) {
    +  // Allow modules to modify the block before it is viewed, via either
    +  // hook_block_view_alter(), hook_block_view_ID_alter(), or
    +  // hook_block_view_NAME_alter().
    +  $id = str_replace(':', '__', $block->getPluginId());
    +
    +  $config = $block->getConfig();
    +  $config_id = explode('.', $config['config_id']);
    +  $name = array_pop($config_id);
    +
    +  $build = $block->build();
    +  drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build, $block);
    +  return $build;

    Okay, interesting.

    Here, the string manipulation is building the template for the hook name, so that makes sense to me. The code comments explain what the string manipulation is doing in this case, and it's specific to invoking the hooks, rather than being something that's a pattern we need to use over and over.

  12. +++ b/core/modules/block/block.moduleundefined
    @@ -1016,10 +622,21 @@ function template_preprocess_block(&$variables) {
    +  // We can safely explode on : because we know the Block plugin type manager
    +  // enforces that delimiter for all derivatives.
    +  $parts = explode(':', $variables['elements']['#block']->getPluginId());

    Yay comments! A method maybe? Related issue: #1701014: Validate config object names.

  13. +++ b/core/modules/block/block.moduleundefined
    @@ -1016,10 +622,21 @@ function template_preprocess_block(&$variables) {
    +  while ($part = array_shift($parts)) {
    +    $variables['theme_hook_suggestions'][] = $suggestion .= '__' . strtr($part, '-', '_');
    +  }

    Yeaghhh. At a minimum this should have an inline comment.

    Also, does the while loop mean that derivatives can have derivatives?

  14. +++ b/core/modules/block/block.moduleundefined
    @@ -1016,10 +622,21 @@ function template_preprocess_block(&$variables) {
    +    $config_id = explode('.', $variables['block']->config_id);
    +    $machine_name = array_pop($config_id);
    +    $variables['block_html_id'] = drupal_html_id('block-' . strtr($machine_name, '-', '_'));
    +    $variables['theme_hook_suggestions'][] = 'block__' . $machine_name;

    Hmm, more "eesh". Related: #1760358: Provide a way to extract the ID from a config object name, #1760358: Provide a way to extract the ID from a config object name

  15. +++ b/core/modules/block/block.moduleundefined
    @@ -1028,23 +645,29 @@ function template_preprocess_block(&$variables) {
    /**
      * Implements hook_menu_delete().
      */
    function block_menu_delete($menu) {
    -  db_delete('block')
    -    ->condition('module', 'menu')
    -    ->condition('delta', $menu['menu_name'])
    -    ->execute();
    -  db_delete('block_role')
    -    ->condition('module', 'menu')
    -    ->condition('delta', $menu['menu_name'])
    -    ->execute();
    +  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
    +  foreach ($block_configs as $config_id) {
    +    $config = config($config_id);
    +    if ($config->get('id') == 'menu_menu_block:' . $menu['menu_name']) {
    +      $config->delete();
    +    }
    +  }

    I find myself pondering whether block should actually be invoking this hook. Menu is the module providing the menu block plugin. (And then, too, the eventual shape of this will depend on what happens with menu conversions to configuration and/or entities.) Note to self: look up those issues.

  16. +++ b/core/modules/block/block.moduleundefined
    @@ -1062,19 +685,16 @@ function block_admin_paths() {
    + * Cleans up any block configuration for uninstalled modules.
    ...
    function block_modules_uninstalled($modules) {
    ...
    +  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
    +  foreach ($block_configs as $config_id) {
    +    $config = config($config_id);
    +    if (in_array($config->get('module'), $modules)) {
    +      $config->delete();
    +    }

    See #1776830: [META-1] Installation and uninstallation of configuration provided by a module that belongs to another module's API. I think this should be the Configuration system's responsibility, not Block's; otherwise, we're going to end up repeating patterns like this over and over. (Note that the point does not have consensus in that issue.)

    This case is interesting because, unlike Views, this configuration is not created with ConfigEntity. It gets us into the weird DMZ between ConfigEntity and the plugin system.

  17. +++ b/core/modules/block/block.moduleundefined
    @@ -1084,9 +704,16 @@ function block_modules_uninstalled($modules) {
    +  $block_configs = config_get_storage_names_with_prefix('plugin.core.block');
    +  foreach ($block_configs as $config_id) {
    +    $config = config($config_id);
    +    $languages = $config->get('visibility.language.langcodes');
    +    if (isset($languages[$language->langcode])) {
    +      unset($languages[$language->langcode]);
    +      $config->set('visibility.language.langcodes', $languages);
    +      $config->save();
    +    }
    +  }

    So: Need to double-check that there's test coverage for this. I half-remember asking something related to the language/block integration tests back in the Dead Marshes.

Followups

  1. +++ b/core/modules/block/block.api.phpundefined
    @@ -11,355 +11,82 @@
    -function hook_block_configure($delta = '') {
    ...
    -function hook_block_save($delta = '', $edit = array()) {
    ...
    -function hook_block_view($delta = '') {

    We are removing these hooks entirely? That's another pretty stark API change. Or is it a followup to reintroduce them? CRUD, entities, etc.; merits a followup discussion.

  2. +++ b/core/modules/block/block.moduleundefined
    @@ -909,50 +512,50 @@ function block_block_list_alter(&$blocks) {
    +  // Don't bother to build blocks that aren't accessible.
    +  if ($element['#access'] = $block->accessWrapper()) {
    +    $build = block_build($block);
    +    if ($build) {
    +      if (isset($build['#title'])) {
    +        $element['#title'] = $build['#title'];
           }
    +      $element += $build;

    Aha, two questions of "who calls this?" answered in one snippet, both the access wrapper and the block hook invocation wrapper.

    Kind of a weird little underscore-prefixed procedural wrapper for a procedural wrapper for the OO code, but that makes sense in a conversion like this. Maybe wants a followup, depending how it's used. Note to self: look up callers for this.

    +++ b/core/modules/block/block.moduleundefined
    @@ -725,178 +463,43 @@ function block_list($region) {
    + * Loads blocks' information from the configuration management system.
      *
      * @return
      *   An array of blocks grouped by region.
      */
    function _block_load_blocks() {
    ...
    +  global $theme;
       $blocks = array();
    -  foreach ($block_info as $block) {
    -    $blocks[$block->region]["{$block->module}_{$block->delta}"] = $block;
    +  $instances = config_get_storage_names_with_prefix('plugin.core.block.' . $theme);
    +  $manager = drupal_container()->get('plugin.manager.block');
    +  foreach ($instances as $plugin_id) {
    +    $block = $manager->getInstance(array('config' => $plugin_id));
    +    $config = $block->getConfig();
    +    $blocks[$config['region']]["$plugin_id"] = $block;
       }
       return $blocks;

    Again with the weird underscore-prefixed procedural wrappers adding a bit of logic around OO code. Something else to look up.

    +++ b/core/modules/block/block.moduleundefined
    @@ -403,252 +353,35 @@ function _block_get_renderable_region($list = array()) {
    function _block_rehash($theme = NULL) {

    What a weird function name. Another one to look up.

  3. +++ b/core/modules/block/block.moduleundefined
    @@ -54,16 +54,21 @@ function block_help($path, $arg) {
    +  if ($arg[0] == 'admin' && $arg[1] == 'structure' && $arg['2'] == 'block' && (empty($arg[3]) || $arg[3] == 'list') && empty($arg[5])) {
    +    if (!empty($arg[4])) {
    +      list(, $demo_theme) = explode(':', $arg[4]);
    +    }
    +    else {
    +      $demo_theme = variable_get('theme_default', 'stark');
    +    }

    AHHHHHH MY EYES BURNING THE BURNING

  4. +++ b/core/modules/block/block.moduleundefined
    @@ -909,50 +512,50 @@ function block_block_list_alter(&$blocks) {
    + * @todo Move the alter invocation into a plugin method.
    + *

    @todo should be at the bottom of the docblock; also, is there a followup issue for this?

  5. +++ b/core/modules/block/block.moduleundefined
    @@ -909,50 +512,50 @@ function block_block_list_alter(&$blocks) {
    function _block_get_renderable_block($element) {
       $block = $element['#block'];
    ...
    +    else {
    +      $element = array();
         }

    So, interesting. We're expecting it come in as a render array, but there's no typehint, and we're deliberately making the array empty if the block isn't accessible, even if it already had stuff in it, which the previous code didn't do. Maybe there should be a followup to add a typehint.

  6. +++ b/core/modules/block/block.moduleundefined
    @@ -403,252 +353,35 @@ function _block_get_renderable_region($list = array()) {
    -/**
    - * Implements hook_form_FORM_ID_alter() for user_profile_form().
    - */
    -function block_form_user_profile_form_alter(&$form, &$form_state) {

    So this is where per-user block customization used to live, I think. I haven't seen it reanimated yet anywhere. If we're removed that functionality deliberately, it should get its own, separate change notification, or a followup issue if we're planning to add it back.

Code style crap

  1. +++ b/core/modules/block/block.moduleundefined
    @@ -698,14 +439,11 @@ function block_theme_initialize($theme) {
    + *   An array of block objects, indexed with the CMI key that represents the
    + *   configuration. If you are displaying your blocks in one or two sidebars,
    + *   you may check whether this array is empty to see how many columns are
    + *   going to be displayed.

    We should probably say "configuration object name" rather than "CMI key". CMI is a Drupal 8 initiative tied to this release cycle, but configuration objects are forever. Like diamonds.

  2. +++ b/core/modules/block/block.moduleundefined
    @@ -725,178 +463,43 @@ function block_list($region) {
    + * @param string $plugin_id
    + *   The plugin id to load.

    The definition of id is:

    the part of the mind in which innate instinctive impulses and primary processes are manifest.

    (s/\<id\>/ID/g)

  3. +++ b/core/modules/block/block.moduleundefined
    @@ -909,50 +512,50 @@ function block_block_list_alter(&$blocks) {
    + * Wrapper for $block->build to allow altering of blocks.

    Verb.

And... http://www.youtube.com/watch?v=n9D7oeM3zd8

Now I'll roll in what cleanups I can from the second half of my reviews here, and then, dare I say, read the issue summary and maybe some of the comments. ;)

xjm’s picture

StatusFileSize
new1.06 KB

I rebased the branch for latest 8.x HEAD and re-pushed. One small merge conflict from #1811828: Use #attached to find the css/js of a view (see attached).

xjm’s picture

tim.plunkett’s picture

StatusFileSize
new404.16 KB
FAILED: [[SimpleTest]]: [MySQL] 49,388 pass(es), 5 fail(s), and 25 exception(s).
[ View ]

Reroll! Will update the branch if this is green.

xjm’s picture

Status:Needs review» Needs work

Note to self: do not attempt to roll patches at what would be 3:00a your local timezone.

The fails are likely from #1811828: Use #attached to find the css/js of a view, though. I will consume coffee before trying to fix them, so that I fix them in a way that is not nonsense.

xjm’s picture

What this patch contains

  1. A BlockInterface defining block configuration settings, configuration forms, access control, and rendering.
  2. An abstract BlockBase implementing the interface for a base plugin, with default settings, form handling, and access control.
  3. A block_build() procedural wrapper for invoking block view alter hooks.
  4. A BlockBundle to add the block manager to the container.
  5. A BlockManager to locate and instantiate block plugins.
  6. Modifications to the Block module's procedural API to use the new plugins.
  7. A workaround in the DerivativeDiscoveryDecorator for #1780396: Namespaces of disabled modules are registered during test runs.
  8. A ConfigMapper class that creates a plugin instance from data stored in the configuration system.
  9. A basic API for creating administrative UIs for plugins.
  10. A block-specific implementation of the plugin UI which adds browsable "facets".
  11. Modifications of the Block module's administration to replace the existing UI with this faceted plugin UI.
  12. The addition of a non-required Custom Block module for user-created blocks.
  13. The replacement of hook_block_info() and hook_block_info_alter() with annotated plugin discovery.
  14. The removal of hook_block_configure(), hook_block_save(), hook_block_list_alter(), and hook_block_view() (but not hook_block_view_alter()).
  15. hook_block_view_MODULE_DELTA_alter() renamed to hook_block_view_ID_alter()
  16. The addition of hook_block_NAME_alter().
  17. The addition of hook_block_access().
  18. The addition of hook_block_alter().
  19. The removal of user-specific block visibility settings.
  20. The migration of blocks to the new API for the standard and minimal profiles, and for the following modules:
    • Aggregator
    • Book
    • Comment
    • Forum
    • Help
    • Language
    • Menu
    • Node
    • OpenID
    • Poll
    • Search
    • Shortcut
    • Statistics
    • System
    • User
    • Views
  21. Updates to the test coverage for the above modules' blocks.
  22. Updates to Block's own tests.
  23. Decoupling of various tests from the previous block system.
tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new409.99 KB
PASSED: [[SimpleTest]]: [MySQL] 49,396 pass(es).
[ View ]
new7.16 KB

Okay, I should have fixed the Views failures. I ran this interdiff by @dawehner, he approved :)

xjm’s picture

StatusFileSize
new17.21 KB
new411 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/block/lib/Drupal/block/Plugin/Type/BlockManager.php.
[ View ]

I added all the minor code style cleanups listed above. There's still a lot of work to be done on the documentation here, and I'll work on that more later.

xjm’s picture

StatusFileSize
new812 bytes
new411 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Ahem.

jibran’s picture

Status:Needs review» Needs work
+++ b/core/modules/block/block.moduleundefined
@@ -1016,10 +621,21 @@ function template_preprocess_block(&$variables) {
+    $machine_name = array_pop($config_id);
+    $variables['block_html_id'] = drupal_html_id('block-' . strtr($machine_name, '-', '_'));
+    $variables['theme_hook_suggestions'][] = 'block__' . $machine_name;
+  }
}

no need for strtr. drupal_html_id replace _ with -

xjm’s picture

Status:Needs work» Needs review
StatusFileSize
new776 bytes
new411 KB
PASSED: [[SimpleTest]]: [MySQL] 49,427 pass(es).
[ View ]