Create a block for every field (which'll take the entity as context)

Make title a little clearer.

I've started working on this!

Here's a first pass at this! It needs to do all the field configuration stuff, and generate better admin labels for the blocks, but it does render fields! :-)

I fiddled with this a bit during the in-between times today. All it does is include better category and block labels.

And once you start, it's hard to stop. :-) Here's a patch which starts to figure out information about available field formatters. Ok, time to go to bed!

This has it storing and using the formatter options that apply to all formatters. Still haven't gotten the actual formatter settings form loading via AJAX yet.

Alright! This finally loads the formatter settings form via AJAX and allows those values to be saved and used. It also sets a label for the entity context and makes it required. I think this is ready for wider review!

I read through the patch and found some little things to clean up. I promise I won't touch this anymore until someone else can review it. :-)

Those test failures were wizard failures. Probably something that needs to be updated for latest HEAD - I'd guess connected with the "Dynamic Page Cache" changes.

Blergh! My interdiff from before was correct, but the main patch was the same as the last one. This one should be correct.

Stupid wizard tests! Marking as NR - hopefully for the last time today. :-)

Here's an issue to fix the wizard tests #2573413: Fix wizard tests

Chatted with EclipseGC about this on IRC. He told me it's possible to add constraints to the context definition for the entity bundle, so that the block won't be available for entity bundles that don't have the given field. What he didn't know is if you can put multiple constraints for the same value.

Here's some example code from src/Plugin/Condition/NodeType.php:

    if (count($this->configuration['bundles']) == 1) {
      $bundle = array_values($this->configuration['bundles']);
      foreach ($this->getContextMapping() as $definition_id => $context_id) {
        $contexts[$context_id]->getContextDefinition()->addConstraint('Bundle', ['value' => $bundle[0]]);
      }
    }

We'll need to some code that calls ->addConstraints() in the deriver.

So, the BundleConstraint can take multiple bundles (in fact, it converts the string to an array). But setting this constraint makes it so that Page Manager won't list the blocks as an option to add when overriding the node/% path.

This is from this code in ContextHandler::getMatchingContexts():

      // If any constraint does not match, this context is invalid.
      foreach ($definition->getConstraints() as $constraint_name => $constraint) {
        if ($context_definition->getConstraint($constraint_name) != $constraint) {
          return FALSE;
        }
      }

Basically, since the contexts from Page Manager don't have the same constraints, it decides that the context is not available, and doesn't return the block as an option. But Page Manager can't add constraints about the bundle, because in the admin UI, it still doesn't know the specific entity.

So, I'm not sure we can add this to the context definition...

Here's the change to this patch that would add the bundle constraint mentioned in #19 just for reference

Here's a new version of this patch which has the deriver descend from EntityDeriverBase.

Since the "bundle constraint thing" (from #18, #19 and #20) ended up not being workable (although, not having the constraint doesn't hurt anything as far as I can tell) I think this now addresses all the informal review from EclipseGC in IRC! So, setting back to Needs Review.

Bah! This is a Git failure on my part, but my last patch in #21 accidentally included the "bundle constraint thing" change which is represented in the interdiff in #20. Here's what the patch should have been. The interdiff from #21 is still correct. Sorry for the confusion!

Alight! Here's a new patch that addresses all of Tim's points in #23. The interdiff is super messy because code is moved around, but it is what it is.

@tedbow: Thanks! I didn't know there was a whole project for that...

In any case, CTools will definitely do this (it did it in D7 too, we just didn't use core blocks in D7), it's just a question of nailing down the implementation. Also, we see CTools as an incubator for core, so it's possible we'll propose this for inclusion in core in a point release (like 8.1, 8.2, 8.3, etc..).

Just a note to self: Do we need to specifically implement ::blockAccess()? Marking as "Needs work" to figure that out and to address the review in #25.

The plan is also for CTools to include improvements to the core block UI, because there are several problems (not just this one) once contrib starts including more blocks and conditions. For example, enabling the Rules module totally breaks the core block UI too.

Tomorrow I'll start making some new issues for the UI improvements! I was hoping to get this one in first and work on the UI stuff second (because CTools is still in -alpha), but I'd be fine with doing it the other way around if necessary. :-)

I created a meta issue for improvements to the core block UI:

#2604162: [meta] Improvements to core block UI

It's got the 3 issues we've discussed in the past, but this is the one that applies to this situation:

#2604178: Modify core block UI to seperate blocks into categories with tabs

Here's a new patch to address the review from #25! Thanks!

Regarding points #4 and 5: I really don't know how we'd reuse code from Views, and actually, even if I knew how I'm not sure it'd be a good idea coupling this to Views code since this functionality doesn't depend on views.

Still TODO:

1. Determine if we need to implement ::blockAccess()
2. Check if we're getting the render cache info correct
3. Move to a new 'ctools_block' module per discussion we had about this issue in a SCOTCH meeting a while back

This patch should finally handle access correctly! It also switches away from the deprecated EntityManagerInterface and uses the two services it actually needs.

Also, I moved my remaining tasks up into the issue summary.

Actually, we can access the entity context from ::blockAccess() so we don't need to check twice!

Here's a patch that moves this code into a new ctools_block module, with no other functional changes. I'm not including an interdiff because it's just moving code.

Here is a patch that adds the boilerplate for writing the tests, but none of the actual tests. I put some @todo's in the EntityFieldBlockTest for the sort of tests we need to write. All of them are hardcoded to fail for now!

Ok, apparently what I did in #36 is wrong, because we don't actually have access to the entity from context in ::blockAccess(). Sorry, @phenaproxima!

Re #42: EclipseGC, phenaproxima and I were getting a fatal error that was fixed by making the change in #39. I'll dig into this now and see if I can't get to the bottom of why that works for you and not us, and why get a fatal for stuff that is working for us. :-)

Also, I added a TODO to the issue summary about the config schema so we don't forget about that!

Thanks, Tim! The config schema stuff is hard, even without two core bugs. :-) Unfortunately, I don't think testbot will run our tests because ctools_block is a new module added in this patch, so the TEST/FAIL patches won't do their thing. :-/

@jonhattan: re #41 - ah, ok, sorry, I misunderstood! I'll look into it!

This gets the render cache tests passing! It also shows what needs to be changed to fix #2631120: "Entity view" block showing first entity rendered on all pages . However, I think one part of the test isn't really testing anything:

+++ b/modules/ctools_block/src/Tests/EntityFieldBlockTest.php
@@ -0,0 +1,105 @@
+    $text = 'This is my text. Are you not entertained?';
+    $a->body->value = $text;
+    $a->save();
+    $this->drupalGet('node/' . $a->id());
+    $this->assertText($text);

Since the body is displayed on the node view page anyway, it'll always find it here. We need to look specifically at the block somehow (or use a different field that isn't displayed on the node view page).

On the other tests too, I think we should test not just that the block label is there, but that the contents look right. We could use a similar or the same approach for that.

Unassigning for now...

Blergh! Tests are failing because my last patch (which phenaproxima's is based on) is missing Tim's config schema. :-/ Here's a patch that adds it back. At least testbot is running the tests for ctools_block now. :-)

re #48:

At the beginning of that test method, the body component is specifically removed from the entity view display in order to ensure that the test is meaningful. :)

Aah, sorry, I missed that. Actually, since we're creating a special content type for this and providing the config for the display, maybe we should just always hide all the fields from normal display?

I'll do something like this is in the next patch!

Alright! This patch removes the fields from normal display in the config bundled with the tests, so if we see the content, we're sure it's from the block. So, I added tests for the actual content where that made sense. Since we aren't actually uploading an image, this made the image field test kinda useless, so I removed that for now (rather than spend time figuring out how to do that - let's just get this patch committed!).

As far as I'm concerned this is ready enough to be merged - we can address anything else in follow-ups.

@chx: I think the main reason is this is how we did it in Drupal 7, and we're working (slowly) towards replicating the experience in Panopoly where you have the blocks divided clearly by category.

That user experience works really well for us, where users drill down in conceptual areas. For example, first they say, "I want to put something with users, so they click 'User'" and get a list of those blocks, which includes both fields and other user-y things, without the user really needing to know the difference between say a "field" and a "view" that takes a user context - they just know they want things that come from users. If they had to select a "User field" or "Entity field" block and then configure it, it requires them to know Drupal-y things like what a field and an entity are.

@tedbow: the issue to replace the current block selection UI, with an experience more like D7 Panopoly is #2604178: Modify core block UI to seperate blocks into categories with tabs

For the most part these changes look great! As far as I'm concerned this is RTBC, but I'm too close to this patch to do the honors. :-)

One minor thing (that you can feel free to ignore):

+++ b/modules/ctools_block/src/Plugin/Block/EntityField.php
@@ -138,25 +139,16 @@ class EntityField extends BlockBase implements ContextAwarePluginInterface, Cont
+    // Set the cache data appropriately.
+    $build['#cache']['contexts'] = $this->getCacheContexts();
+    $build['#cache']['tags'] = $this->getCacheTags();
+    $build['#cache']['max-age'] = $this->getCacheMaxAge();

This is cool! However, it kind of sucks having to set each of those #cache values independently.

What if we implement CacheableDependencyInterface (we're already implementing it's methods) and then did something like:

CacheableMetadata::createFromObject($this)->applyTo($build);

That should be sufficient and it's friendlier to read.

That said, it's probably slightly slower and really doesn't matter. :-)

Woohoo! Thanks. :-) I somehow missed this yesterday..