Posted by xjm

Problem/Motivation

This is a followup issue for #1535868: Convert all blocks into plugins.

Most (if not all) classes, methods, etc. for the new blocks architecture have the minimum required documentation, but there are no extended descriptions of how the architecture works and how the various components fit together, and it is very difficult to understand without a prior understanding of the plugin system.

Proposed resolution

Add a detailed explanation of the architecture (probably on BlockInterface). Explain the "big picture" of how all the pieces fit together--plugin discovery, saving to configuration, per-theme derivatives, etc. Include references to other related classes, interfaces, procedural code, etc. (The goal should not be to duplicate what is already documented on individual classes and interfaces, but to help developers understand how it all fits together.)

Remaining tasks

Some of the needed documentation (around how blocks are stored, loaded, etc.) should probably be added following #1871696: Convert block instances to configuration entities to resolve architectural issues.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Issue tags: +Blocks-Layouts
xjm’s picture

Status: Postponed » Active
xjm’s picture

Status: Active » Needs review
FileSize
3.13 KB

@EclipseGc suggested tonight that we might be able to add an @Plugin to an abstract class without it being parsed as an annotation, so let's see how that goes. The attached is only the very beginning of the docs we need.

xjm’s picture

So... nothing blew up. That's cool. The only other thing we need to take into account then is making sure the API module can parse an annotation in the middle of a docblock like this when it's not being parsed by Drupal.

xjm’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.phpundefined
@@ -15,6 +15,42 @@
+ *   derivative = "Drupal\my_module\Plugin\Derivative\MyBlockDerivatives"

Missing comma here, and an extra newline near the end.

jhodgdon’s picture

Status: Needs review » Needs work

Change in status per #5 to get this out of my "probably ready to commit" queue. :)

Can I also suggest that the best place to make "Big Picture" documentation of how a system works is with a @defgroup rather than in a particular class's documentation? Then you can add the relevant classes to the topic using @ingroup.

xjm’s picture

@jhodgdon, any feedback on #4? In this case here, the annotation is not parsed, because the class is abstract. I am not sure if that makes things more difficult for the API module or not.

A defgroup is a great suggestion, and that would also be a good way to get around the "how do we document an example annotation" problem. In fact, the plugin system itself should probably have one.

jhodgdon’s picture

Ah, sorry, didn't notice that question on #4.

So:
- The example of @Plugin should be put into @code/@endcode tags. Otherwise there is zero chance it will be formatted nicely on api.drupal.org.
- The API module will probably not choke on the @Plugin() where it is in the patch above. There is a special case in the API module for handling @Plugin where it assumes @Plugin() is an actual annotation and handles it like @code/@endcode. But the regexp is
'/@Plugin(.*)$/'
so unless there is a ) right at the end of the entire docblock, it shouldn't think the @Plugin in the middle is an actual annotation. It could be a problem if someone added @see function_name() at the very end though, since it would then probably think that almost the entire docblock was an @Plugin annotation.
- The API module doesn't use the actual annotation parser, just this regexp.

Does that help? Basically: Should be OK, but put it in @code/@endcode for best formatting. And I would also suggest adding something to the documentation saying that these @Plugin annotation things come at the end of docblocks. That is not explicitly stated in the documentation. I realize it's clear to people that invented the system, but... :)

xjm’s picture

Thanks @jhodgdon!

And I would also suggest adding something to the documentation saying that these @Plugin annotation things come at the end of docblocks. That is not explicitly stated in the documentation. I realize it's clear to people that invented the system, but... :)

Well, that's true of annotations generally, and we do have a mention for it at http://drupal.org/node/1354#annotations (first point under the example). There should be documentation for the plugin system itself in the codebase, though (out of scope here, but definitely something most everyone who hasn't gotten used to annotations via 300 Views plugins has been confused about).

jhodgdon’s picture

Should we have a @defgroup that explains annotations and the plugin system as well then? It seems like that would be useful if we don't yet have one. It could be a short couple of paragraphs with a link to the plugin API documentation http://drupal.org/node/1637614

xjm’s picture

Issue tags: +Block plugins
jibran’s picture

Status: Needs work » Needs review
FileSize
2.47 KB

Here is the reroll.

  • Fixed #5.
  • Added @code/@endcode tags.
  • Removed BlockInterface changes because seems irrelevant now.

Should we have a @defgroup that explains annotations and the plugin system as well then?

I like this suggestion but It is out of scope here because this issue related block system's architecture. We can create a follow up for this and decide where to put @defgroup that explains annotations.

jhodgdon’s picture

Status: Needs review » Needs work

This documentation seems pretty clear to me, except this one place:

+ * - settings: (optional) The default configuration settings for the block.
+ *   You can also add to or override these settings in
+ *   BlockBase::blockSettings(). By default, your plugin will inherit the
+ *   settings of the base class.

I am confused by this... what does it mean? Maybe it is just a typo and "these" in the second line should be "the", so that it is trying to say you can also put settings in your annotation to override the default settings?

I have also not reviewed the documentation for accuracy. Someone familiar with the block system needs to do that.

jibran’s picture

Status: Needs work » Needs review
FileSize
1.67 KB
1.59 KB

Status: Needs review » Needs work

The last submitted patch, drupal8.documentation.1871762-14.patch, failed testing.

jhodgdon’s picture

Where do all of the other settings go now?

jhodgdon’s picture

Issue summary: View changes

Removing myself from the author field so that I can unfollow the issue. --xjm

jhodgdon’s picture

We actually need a @defgroup topic for this to be linked to from the new Drupal 8 api.d.o front page for API docs. See #2148255: [meta] Make better D8 api.d.o landing page, linked to high-level overview topics, and put it in Core api.php files

I'll be making a stub of the defgroup on that issue; we'll need to fill it in.

jhodgdon’s picture

We now have a stub for the block API stuff. It's in file core/modules/system/core.api.php where it says
@defgroup block_api
strangely enough. :)

The idea is for this to be filled in with a couple of paragraphs about how to define blocks in a module, and a link to the page on drupal.org that explains it more thoroughly, as well as @see links to relevant classes/interfaces that people would need to reference.

jhodgdon’s picture

Actually, not @see links. What we need is for the relevant classes/interfaces/etc. to have

@ingroup block_api

added to their doc blocks.

joachim’s picture

(Passing thought: could we use wiki pages on gdo to work collectively and incrementally on long documentation texts such as this, prior to making a patch?)

Here's a first stab, drawing from https://drupal.org/documentation/blocks and the CR https://drupal.org/node/1880620

Blocks are the boxes of content that together make up the pages output by Drupal. These include the "User Login", "Who's online", and "Main page content" blocks.

Each block is a combination of a configuration entity and a plugin. The configuration entity stores placement information (theme, region, weight) and any other configuration information that is specific to the block's plugin. The plugin class provides the render array for the block's output in its build() method, and elements of the block configuration form for its specific configuration information. This is the same plugin/configuration entity dual mechanism that is used for actions, editors, migrations, views etc.

jhodgdon’s picture

I don't think we really want an extremely long doc page added as a @defgroup. What we want is a long docs page under https://drupal.org/developing/api (and those pages are effectively "wiki" in the sense of "anyone can edit" already), and a short summary in a @defgroup with a link to the longer docs page.

So the text in #20 looks like a good start... I think I would get rid of the last sentence referencing other plugins (we want to keep this short and relevant to blocks), and maybe reverse the order of the sentences about the plugin and the config entity... I guess I would also say that the plugin "does the work of rendering the block" and "builds the plugin-specific part of the configuration form", rather than the text there -- more active and more descriptive of the reason it does what it does rather than describing the implementation details in that overview paragraph?

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
4.62 KB

Here's a stab at rolling in the info from the existing patches in this thread and some additional information and examples. This fills out the @defgroup block_api, and adds a few classes to the group.

I could use some help with the annotation/comment in the example. I can't figure out the correct syntax to have a comment inside a comment and have the API module render it correctly.

And of course both a technical and grammar review.

joachim’s picture

Looks like a good start!

  1. +++ b/core/modules/system/core.api.php
    @@ -61,21 +61,68 @@
    + * Blocks are defined by classes that implement the \Drupal\block\BlockPluginInterface
    

    You've just said a Block is two things. This needs to say which of the two are you are now talking about here.

  2. +++ b/core/modules/system/core.api.php
    @@ -61,21 +61,68 @@
    + * Core provides the \Drupal\block\BlockBase class that can be extended to
    

    Same here. Is BlockBase a plugin or the config entity base?

A great source of mystery to me is plugin bags. Should we mention those, at least briefly with a @see to the topic on that?

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! A few thoughts:

a)

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -22,6 +22,10 @@
  * This abstract class provides the generic block configuration form, default
  * block settings, and handling for general user-defined block visibility
  * settings.
+ *
+ * For additional information see the @link block_api Block API topic @endlink.
+ *
+ * @ingroup block_api

The @ingroup block_api says the same thing as the preceding line. Let's just have the @ingroup.

b)

+++ b/core/modules/system/core.api.php
@@ -61,21 +61,68 @@
  * - @link https://drupal.org/list-changes API change notices @endlink
  * - @link https://drupal.org/developing/api/8 Drupal 8 API longer references @endlink
  */
-
 /**

Please do not remove that blank line.

c) Similar to joachim's review in #23... let's think about the content of the block_api topic...

1. First paragraph:

+ * Blocks are the boxes of content that together make up the pages output by
+ * Drupal. These include the "User Login", "Who's online", and "Main page
+ * content" blocks. Blocks can be placed into regions provided by any active
+ * theme via the Block layout UI.

We aren't really even defining what a "block" is in block_help() currently, which is the help displayed in Drupal by the Help module (maybe we should, but it isn't currently being done). So, I don't really know that we need to define them here in this kind of detail. I think we can make certain assumptions about people reading api.drupal.org, and one of them is probably that they know what a block is? Let's just leave this paragraph out.

2. Next paragraphs... So... Our audience here is module developers, who presumably want their module to provide a block that Drupal's block module can display, right? So ... let's make this topic describe what you actually need to do, and maybe instead of providing a worked-out example here, point people to a drupal.org docs page that does that and/or a class or two in Core that are good examples, and/or the Examples for Developers project (which has a block example).

So I think what all we should really say is (in complete sentences etc.):
- Blocks are examples of Plugins See the @link plugin_api Plugin API topic @endlink for more information about plugins.
- To define a block, you need to make a class that extends BlockPluginInterface and has Block annotation.
See the @link annotation Annotations topic @endlink for more information about annotations.
- There is a base class that you will probably want to use, to make this easier: BlockBase [I don't think we need to include details about config]
- For examples, see (an example of a Core block without config options and one with config options)
- For more information, see https://drupal.org/developing/api/8/block_api [I added an alias to https://drupal.org/node/2168137] or the Block example in https://drupal.org/project/examples

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
3.42 KB

Okay. Here's a slightly less wordy version that I think does a good job of explaining the steps necessary to implement a custom block in your module as well as linking to other blocks/documents as examples instead of having inline code examples. Thank you both for the feedback.

jhodgdon’s picture

Status: Needs review » Needs work

I think this is much better, thanks! Now we're down to nitpicks... A few thoughts:

In this section:

+ *
+ * Drupal core provides the \Drupal\block\BlockBase that provides a common
+ * configuration form for Blocks as well as an easy way to access the
+ * configuration entity associated with the block. In most cases custom blocks
+ * should extend \Drupal\block\BlockBase.
+ *
+ * To define a block in a module you need to:
+ * - Create a Block plugin. See the @link plugin_api Plugin API topic @endlink
+ *   for more information about plugins.
+ * - Implement the \Drupal\block\BlockPluginInterface and use the annotations
+ *   defined by \Drupal\block\Annotation\Block. See the @link annotation
+ *   Annotations topic @endlink for more information about annotations.

a) I think the 1st paragraph here could be more easily understood in the context of the bullet list below. So the bullets could be: (1) Create a block plugin class (2) It must implement () (3) usually you will want to extend () (4) It needs to be annotated with (). [schematically], and then we could get rid of the first paragraph here entirely. Does that make sense?

b) @link ... @endlink has to be all on one line, which can go over 80 characters if it has to (but start it on a new line if it won't all fit on the previous line).

c) You talk in the very first paragraph of the docs about configuration entities, and then don't mention them again. So maybe there needs to be a 5th bullet point that explains what you need to do about the config entities, if anything? Either that or maybe say something above that the config entity is something that you don't have to worry about as a developer? Because after reading this I was left thinking, huh, um, what about those config entities then?

d)

+ * The \Drupal\system\Plugin\Block\SystemPoweredByBlock provides a simple example
+ * of defining a block. And \Drupal\book\Plugin\Block\BookNavigationBlock
+ * is an example of a block with a custom configuration form.

Not sure if it needs "The" at the beginning? If it does, the 2nd sentence would also need a "the" after "And", wouldn't it? I think both are better off without "the", myself.

The last submitted patch, 25: 1871762-25_block-api-docs.patch, failed testing.

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
3.39 KB

I merged the paragraph about BlockBase into the bullet list and added a little extra text about how BlockBase allows you to interact with the configuration entity associated with the block. Getting warmer. Thanks again for the feedback.

I have no idea why all those tests failed, but let see what happens this time.

Out of curiosity "@link ... @endlink has to be all on one line" is this just part of the doc standards? The API parser handles it spread across multiple lines with no problem.

jhodgdon’s picture

Status: Needs review » Needs work

RE @link/@endlink - standards: https://drupal.org/node/1354#link -- I must have fixed the problem with the API module not recognizing it if it spans two lines. :)

RE: test fails - yeah, looks like a glitch on the bot.

So this is now very close! Just a couple of things:

a)

+ * To define a block in a module you need to:
+ * - Create a Block plugin. See the @link plugin_api Plugin API topic @endlink
+ *   for more information about plugins.

Can we say something here spelling out that a block plugin is a class, what namespace it needs to be in, mention PSR-4, and put in a link to the OO section (which hasn't been written yet but will be sometime)?

b)

 * - Usually you will want to extend the \Drupal\block\BlockBase class which

Before "which", you need a comma.

c) next line:

+ *   provides a common configuration form, and utility methods for getting and

I don't think the comma is good here.

d)

+ *   \Drupal\block\Annotation\Block. See the @link annotation Annotations topic @endlink

Start the @link on the next line, to avoid going over 80 characters.

e)

+ * \Drupal\system\Plugin\Block\SystemPoweredByBlock provides a simple example of
+ * defining a block. \Drupal\book\Plugin\Block\BookNavigationBlock is an example
+ * of a block with a custom configuration form.
+ *
+ * For more information, see https://drupal.org/developing/api/8/block_api or
+ * the Block example in https://drupal.org/project/examples.

This could be combined into one paragraph, or perhaps a bullet list? "Further information and examples:" maybe?
Bullet lists are easy to scan...

eojthebrave’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

"Can we say something here spelling out that a block plugin is a class, what namespace it needs to be in, mention PSR-4, and put in a link to the OO section (which hasn't been written yet but will be sometime)?"

I kind of feel like this was in my original patch but we opted to remove it because we felt it was better to point to examples of existing blocks than to write a full example in the documentation. I've made a couple of wording changes, add "create a class" and that if you want to learn more about how blocks are discovered read up on it in the plugin API topic.

If we want to say more than then I think we should just go back to having a fully fleshed out example honestly.

I cleaned up the other grammar issues. And turned the ending part into a bullet list as per your recommendation. Thanks again for the review.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think this is great! Let's ship it! Thanks!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Perfect! Love the pointers off to existing code for examples, etc.

Committed and pushed to 8.x. Thanks!

  • Commit 39edc27 on 8.x by webchick:
    Issue #1871762 by eojthebrave, jibran, xjm: Add detailed documentation...

Status: Fixed » Closed (fixed)

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