Problem/Motivation
Block plugins have the API plumbing to be contextual, but they don't have the UI exposed to allow for that sort of mapping.
Proposed resolution
Leverage the abstracted UI component built (and already in core) for Condition plugins for Blocks as well.
Remaining tasks
- Get consensus on existing test coverage.
- RTBC
- Get someone to commit it!
User interface changes
For blocks that accept context, it will show a select box to map that context from one of the available context values. If only one possible value is available, it'll just select that one.
However, no block in core demonstrates the changes. #2550199: Add a ContextProvider for Feeds will introduce appropriate changes for aggregator feed block, but that has been descoped from this issue.
API changes
None.
Beta phase evaluation
Issue category | Bug because every block has the plumbing to accept contexts, but (before this patch) each and every block would need to implement the UI and config storage themselves. It would not be possible to implement something in CTools that would affect every block, because we'd need the block to make sure their configuration schema can accept the mapping, and use the same configuration keys. Since core blocks are 99% of the way there, it seems only a bug that they wouldn't go the last 1% and save us from having to make a new BlockBase to use in contrib. |
---|---|
Issue priority | Major because the amount of work and tension from having a split block eco-system (ie. contrib using a new BlockBase class from CTools, rather than core's BlockBase ) or a completely non-standardized eco-system is much greater than simply connecting the last of the dots in the core Block API. Also, if we merged this patch later (ie. in 8.1.x), making an upgrade path would be problematic because how contrib handles config storage of context mapping without this patch is essentially undefined. |
Prioritized changes | The main goal of this issue is nail down the block_settings configuration (so it's standardized in how context mappings are stored) and provide UI for the otherwise complete block context handling already in core. This needs to be done before 8.0.0, because doing it later would be difficult to upgrade the configuration. |
Disruption | Not disruptive for core and contributed modules because it's only adding new UI and config for blocks. It doesn't make any API changes at all - the internals are already pretty much already set. |
Comment | File | Size | Author |
---|---|---|---|
#164 | 2377757-block-164.patch | 16.86 KB | tim.plunkett |
#164 | interdiff.txt | 1.39 KB | tim.plunkett |
#163 | 2377757-block-163.patch | 16.62 KB | tim.plunkett |
#163 | interdiff.txt | 2.08 KB | tim.plunkett |
#163 | Configure block | Drupal-2015-09-24.png | 63.46 KB | tim.plunkett |
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedI'm thinking something along these lines to start + a conversion and tests, or at least tests that properly cover it. (this is dependent upon #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types so until it lands we can't properly test this)
Eclipse
Comment #2
EclipseGc CreditAttribution: EclipseGc commentedComment #3
tim.plunkettWow this is ugly. Another +1 for #2371853: Add more helper methods around temporary FAPI storage
Why do we need this here?
Still don't need this hunk. It's done in BlockAccessControlHandler, which is triggered by the access call two lines away.
We have TestContextAwareBlock in block_test available for tests.
Comment #4
EclipseGc CreditAttribution: EclipseGc commented3.1: Yup
3.2: You might be right, I'll try with the next patch
3.3: nope, this never maps for the block itself, only conditions
I've converted the AggregatorFeedBlock. It has real contexts now... which is pretty awesome. I had to introduce an AggregatorFeedContext, and I have some questions around it. Perhaps there's a better way to do what it does, but as a proof of concept, I'm happy for the moment.
Eclipse
Comment #5
EclipseGc CreditAttribution: EclipseGc commentedoops, 4 included 2339151 & other changes... This excludes what's in 2339151 but still depends on it.
Eclipse
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedOk, I tried to not do 3.2 but it doesn't save the context mapping properly w/o.
Eclipse
Comment #7
tim.plunkettYou're right on 3.2, that's my bad.
And you're also right, 3.3 is needed, but it should move into BlockAccessControlHandler, right above/below where condition context is applied.
This will speed things up for disabled blocks (the first check in BlockAccessControlHandler).
I have some fixes for ContextHandler, but they're out of scope for this issue and need to go into a blocker, which I'll open tomorrow.
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedOk, essentially starting anew here. I will provide interdiffs going forward from this patch. Now that #2378585: Multiple context requirements cannot be satisfied by a single value & #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types have both landed, I can write a patch that could actually pass or fail tests appropriately!
Let's see how this goes.
Eclipse
Comment #9
tim.plunkettYou can just do
$context_mapping = $settings->getValue('context_mapping', []);
We have setContexts() twice now.
Comment #11
EclipseGc CreditAttribution: EclipseGc commented1203? That ain't bad! :-D
9.1: I'll check
9.2: DOH!
Eclipse
Comment #12
larowlansome minor nitpicks
Needs to be Contains \Full\Path\To\Class and no .php
Needs a docblock
c/p fail?
Comment #13
EclipseGc CreditAttribution: EclipseGc commentedI'll address 12 in my next patch.
Fixed some tests, getting some weirdness around drupalPostForm(), not sure why yet.
Eclipse
Comment #14
EclipseGc CreditAttribution: EclipseGc commentedopps
Comment #16
tim.plunkettWe don't have any sanity checks around the contents of $contexts, that feels wrong.
Aggregator doesn't depend on block, this has to be optional :(
Also, do we really want to load all of these on runtime?
Comment #18
EclipseGc CreditAttribution: EclipseGc commentedWe're only loading blocks of the aggregator_feed_block plugin_id for the current theme. So we should only be loading ones we KNOW to be relevant (not all of them). I'm fine with putting sanity checks in there, but the block's configuration should ensure we get a mapping. Yes at the raw plugin level, you could technically create one w/o context, so I'm fine with sanity, but it's definitely an edge case.
Eclipse
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedI put the $feeds check in there to prevent us from processing the same feed multiple times if there are multiple blocks placing the same feed.
This is IN block... why are we checking if block module exists?
Comment #20
tim.plunkett1) If you're going to check $feeds, you should actually add stuff to it... Leaving out for now.
2) I meant to put this in aggregator.module
Comment #22
tim.plunkettWomp womp #2363351: Cached services can't be used in service providers/modifiers
Comment #23
tim.plunkettAlso #2371853: Add more helper methods around temporary FAPI storage went in, so we can rewrite the temporary FAPI calls.
Comment #24
EclipseGc CreditAttribution: EclipseGc commentedSo we have a weird thing where posts aren't happening properly. I changed a couple things, and want to see where we stand on tests. Tim pointed out https://github.com/md-systems/file_entity/blob/8.x-2.x/src/FileEntitySer... which was a great fix to our trials with module handler in the service provider.
Eclipse
Comment #26
EclipseGc CreditAttribution: EclipseGc commentedThis MAY actually pass... let's see.
Eclipse
Comment #27
tim.plunkettSeems strange that we wrap this in an if() in one method but not the other.
This change is rather pointless, it doesn't matter when we call this as long as it's before access()
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedFixing stuff from 27
Eclipse
Comment #29
EclipseGc CreditAttribution: EclipseGc commentedOk, I've moved the context mapping form element to be directly before the block plugin's specific configuration form. If this passes, I'm ready to take this to the next review level.
Eclipse
Comment #30
webchickSince this touches the UI, needs to be checked over for Usability. We also need a beta phase evaluation template for this issue.
I tried to give the patch a whirl, but can't seem to see an Aggregator/Feed block no matter what I do. Steps:
1) Apply the latest patch
2) drush si
3) drush en aggregator
4) admin/config/services/aggregator
5) Add a feed to http://rss.cbc.ca/lineup/topstories.xml (Canada FTW)
6) admin/structure/block
No errors, just no block is visible.
Comment #31
EclipseGc CreditAttribution: EclipseGc commentedApparently some stuff changed in head that causes blocks to only be listed by available contexts. Need to update this patch accordingly.
Eclipse
Comment #32
EclipseGc CreditAttribution: EclipseGc commentedI'll update the docblocks in the next patch, I just wanted to see if this would pass.
Eclipse
Comment #34
japerryThanks for pushing this forward EclipseGC! For panels this is a critical component for us that really shouldn't live in contrib.
Comment #35
tim.plunkettComment #36
dasjoworks well with #2287073-12: Allow views contextual filters to expose the context using argument validation plugins
Comment #37
EclipseGc CreditAttribution: EclipseGc commentedOk, ran into a bug we'd all missed while doing this. The code changes made to \Drupal\Core\Annotation\ContextDefinition to support TranslationWrapper was completely neglecting that the only way we get that wrapper is by calling the get() method of the Translation annotation. This made it impossible for me to rework the form element to display a form description for the context. It was a minor change, but good to know we got that squared away. Should probably add in some tests that verify we've got title and description text into the context definition properly.
I'm on vacation and took this morning to try to move this patch forward a bit more. It might be a few more days before I can do anything with it. (I expect the previous test to still fail. Tim and I need to discuss it still.)
Eclipse
Comment #39
dsnopekUpdated remaining tasks per EclipseGC at SCOTCH meeting.
Comment #40
EclipseGc CreditAttribution: EclipseGc commentedUntested Reroll, let's see what happens.
Eclipse
Comment #42
EclipseGc CreditAttribution: EclipseGc commentedMissed a use statement.
Eclipse
Comment #44
EclipseGc CreditAttribution: EclipseGc commentedAnother use statement...
Eclipse
Comment #46
tim.plunkettOur previous test coverage was proving that we didn't support context-aware blocks.
In order to maintain our test coverage now that we do support them, I added a block that has an unsatisfied context (entity:foobar), and tweaked the original test.
Comment #47
aspilicious CreditAttribution: aspilicious commentedCan we have a couple of screenshots?
Comment #48
EclipseGc CreditAttribution: EclipseGc commentedYes you may! I've included explanations as relevant. We are using Aggregator Feed block as an example of how this can work, aggregator is attempting to solve a problem the context system was invented for and solves it badly currently, I can illustrate this in the existing UI. Most of what aggregator does wrong would probably apply to most modules needing something similar.
Current HEAD
After installing Aggregator, we visit the block layout admin w/o creating a feed yet and we are treated to this:
If we attempt to configure this block without having created a Feed yet, we get this:
This is kind of ridiculous because there are no feeds, and we're placing a block that cannot even function yet. Once we create feeds we get:
for 1 feed and:
for N feeds respectively.
Summary of HEAD:
We have the ability to place aggregator feed blocks before they're useful. This can lead to misconfigured blocks that don't function.
After Patch:
Leveraging the context system brings some uniform benefits to the UI, first on the list is that before you've created any feed entities, the block layout system won't even give you the erroneous option of placing feed blocks:
After we've created feeds, the block system shows us that we can actually use the block:
If we've only created a single feed, the block doesn't prompt the user for unnecessary configuration, it knows there's only one possible feed to use, and it just auto-configures that single value:
Once multiple Feeds have been created, the user interface expands accordingly:
and
Summary of Patch
The patch progressively discloses to the user features that they can use as they become available. If there's not a feed, the block can't be placed, if there's only one feed, the block doesn't ask which feed you want to use, it's only once you have multiple feeds that the UI presents you with the maximum number of questions.
This would extend to all context consuming blocks.
Eclipse
Comment #49
aspilicious CreditAttribution: aspilicious commentedThnx! I had no clue what the patch was doing. This is awesome btw!
Comment #50
EclipseGc CreditAttribution: EclipseGc commentedSpoke with Alex Pott some and he pointed out a few things I'd missed or done improperly. Let's see how this goes.
Comment #51
Wim LeersThanks for #48. Awesome explanation!
IMHO it'd be better if at https://www.drupal.org/files/issues/06%20Block%20Config%20-%20post%20pat... we'd show that "Drupal Planet" was the feed that was used for this block. Otherwise, the user doesn't know this block is associated with that feed, which may lead the user to think that it'll show all feeds; i.e. that once an additional feed is created, it will also be listed in the block.
Comment #52
EclipseGc CreditAttribution: EclipseGc commentedSo, you'd just prefer some sort of visual queue that can't be interacted with?
Comment #54
Wim Leers#52: Yes, using
'#type' => 'item'
or something like it.Comment #55
EclipseGc CreditAttribution: EclipseGc commentedOk, I think Bojhan has this on his list for today, if he feels the same way, I'd be happy to do it, but I'm not going to change the existing UI work until I hear from him. I am definitely not opposed to that sort of a visual queue.
Eclipse
Comment #56
Wim LeersHere's an initial review.
Interesting change. Does this point to a gap in HEAD's test coverage?
This will actually contain a UUID, right? I know we don't have a specific config schema type for that, but a comment noting that here would be welcome.
C/p remnant.
I don't know contexts well, so please forgive any stupid mistakes.
But if I understand this patch well, then classes like this one (
AggregatorFeedContext
) will become pretty widespread. Which means their DX is important.I think it's very confusing that these 3 lines are duplicated between these 2 required methods. With only a tiny subtle difference. It's not at all clear why that difference exists.
And this feels like dark magic.
Needs documentation to explain what it's doing.
We always format these across multiple lines.
Nice :)
Even nicer!
Now needs IDE hint.
Comment #57
EclipseGc CreditAttribution: EclipseGc commented1.) Possibly, yes
2.) I had not considered UUID, I'll look at that in more detail as I think that makes a ton of sense but it's been a while since I wrote that bit.
3.) bah
4.) Yeah... so you're not wrong, but aggregator is sort of a weird instance... in fact most example in core are weird cause we do core weird, but the difference between an administrative context and a run time context is that run time needs to have a real data object to back it, while administration does not. In the case of Aggregator though, we know what available feeds we have (since we don't generally display these on a site exclusively through route parameters like we would nodes or terms and we generally have fewer of them). The only difference in your snippet there is the labeling, which is done so that administratively, we know which feed context corresponds to which feed entity. There are existing NodeRouteContext classes and also one for global user. Node has empty administrative contexts (making the difference clearer I hope) but user (since it's current user and we can always get that) suffers the same confusion you're pointing out here.
TL:DR;
If the context object we're dealing with can be resolved to a real data object during administration, these two methods are going to look similar, if it can't they'll look really different.
5.) If your comment in 2 is feasible, I think this would go away.
6.) Errr... we do? I'm not opposed, but we're hardly using ContextDefinitions in core except in Conditions, and I hadn't seen this there. Is there a code standard we have that dictates this? I'm happy to do it, just didn't realize.
7.) :-D
8.) Yup, this is actually the beauty of plugin contexts is that we build constructs responsible for this per calling code use case instead of hard coding the approach into the plugin itself.
9.) Yup, that seems fair.
Eclipse
Comment #58
Bojhan CreditAttribution: Bojhan commentedThanks for asking for review :)
I am really happy to see this patch, and it looks like its 90% there in terms of the UI approach.
WimLeers is correct in stating that we should inform the user about the connected feed, even when there is only one. This sets the expectation for the next one that you can select it, and informs the user about the content of the block. Which is crucial for understanding the underlying concepts.
In terms of the labeling, I suggest to go with:
"Feed" - no description.
The rest should probably be a given, the point of a selector is to "select" something so you don't have to mention it and the point of a block is to "display" something. So if you connect the concept of which feed, you should be fine from a informing the user point of view of the result.
Comment #59
alexpottI think the aggregator block context should also include block_count - ie. everything from AggregatorFeedBlock::blockForm()? Also the schema for the context mapping should be defined by the block.settings.aggregator_feed_block schema.
Comment #60
EclipseGc CreditAttribution: EclipseGc commentedNo, the block count is user input from the configuration form. The context is a statement of requirements. If you don't have a context that satisfies the block's needs, we don't even allow you to place the block. While it could be done as an optional context, it's an abuse of context to leverage it that way, and would be a pretty big change since that'd imply that all plugin settings should run through context across all types.
Aggregator feed block, as it stands today, is a pretty classic case of some data object solving the context needs of some plugin. This clarifies the use of the plugin in question and simplifies various other aspects of the plugin's approach to rendering information associated with that data object. That's classic Context.
Eclipse
Comment #61
fagoI must say it's really strange to me that the context provider class is responsible of injecting the context into the block plugin. There should be a separate component that's in charge of applying context mappings based on available context. The job of this class is providing context and that should be it.
More generally, I do not think it makes much sense to expose the context of each entity globally as done for feed entities in this patch via "agreegator.feed:{ID}" - that way we'd end up with tons of contexts. Instead, that's actually a case where context is full-filled without a context mapping, but by directly provided value by the user (direct input value in Rules slang). For solving that properly adding something along the lines of registering "static context" or more sophisticated context configuration processing in order to support "direct input values" would be required. But imho that's far out of scope for this patch - so maybe we should come up with a better, easier to solve example here?
One I could think of in core is "users", on the user profile page you have both the context of the viewed and of the current user available.
This highlights perfectly that there shouldn't be a hard separation between context vs configuration parameter. Also see #1976758: Plugins miss metadata about configuration.
Comment #62
EclipseGc CreditAttribution: EclipseGc commented1.) "One I could think of in core is "users", on the user profile page you have both the context of the viewed and of the current user available."
The Global user is available through a similar methodology as this and the user profile is available via the routing mechanisms that fubhy could well describe. The "strange"-ness of this approach is due to core's block placement notions. I don't disagree with your basic statement about how strange that is, but it's because of core, and this gives us the appropriate underlying structure required in order to create more sophisticate context resolution processes in contrib. The specific code you've highlighted here is an optimization to prevent every single aggregator feed entity from being loaded into the global context needlessly and only loads them for the blocks that need them. YES there are way better ways to get at these context; NO core can't really afford to contemplate them at this point. That's why this approach is isolated in a block specific context event and block config entities.
2.) I'd prefer to NOT continue to argue this at this point. The amount of work required against core blocks to even contemplate such an undertaking is completely ridiculous at this point, and on top of that, we don't agree. I'd also argue that thinking of context and configuration as "the same thing" is exactly what has made the current aggregator feed block as problematic as it is. You're just trying to come at it from the opposite angle. Even if I could be convinced of this (and I'm still on the fence) that ship has sailed for D8. Let's get reasonable context object mapping into core instead.
Eclipse
Comment #63
japerryHere is a re-roll of the patch. Not sure why, but the annotation doesn't appear to be working. Following webchick's steps in #30, I get to this on the block:
Comment #64
japerryComment #65
japerryAfter talking a bit with EclipseGC, I've re-rolled again, where it seems to be working correctly with feed aggregator, it also fixes a problem introduced with #1763964: Use #type => link for theme_aggregator_block_item() -- because it defines the link incorrectly (uses href instead of url)
We added a markup value when only one context is available instead of hiding it from the user. Adding this also shows up in conditions, so we know which contexts are being added.
Comment #70
japerryThis patch takes a slightly different approach to #2429501: AggregatorFeedBlock does not output item links, which should get committed before this.
Comment #71
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, we introduced some exceptions with the last patch, let's see how this one fairs.
Changes:
Eclipse
Comment #73
EclipseGc CreditAttribution: EclipseGc at Acquia commentedTrying to fix tests.
Eclipse
Comment #74
EclipseGc CreditAttribution: EclipseGc at Acquia commentedWell... that was novel...
Comment #75
Wim LeersPoints to missing test coverage?
Already remarked in #56.1… but never addressed.
Is the ucfirst appropriate? Isn't this enforcing the rules for English?
"default value" vs "context slot" -> strange
See #56.2 + #57.2. Also never addressed.
In fact, I see that *nothing* in #56 was ever addressed. The points were replied to, but the patch was never updated. Marking NW for that.
If we keep writing reviews without those reviews being actually addressed, we're going to be stuck in an endless loop here.
Where is this translated string exposed? "route" likely is a too technical term.
Comment #76
EclipseGc CreditAttribution: EclipseGc at Acquia commented75.1: Yeah I've not attempted to fix the test coverage yet. This is totally true.
75.2: I don't care. Changed.
75.3: Tried to add a clearer comment.
75.4: True I didn't add a comment (fixed) however I certainly did reconfigure the entire patch to work on feed entity uuids, so 57.2 was definitely addressed. (my bad for no interdiff)
75.5: It displays in the context title or selection elements. I've changed it to "Node from url" for the time being.
I'll work on 56.1 now, but I want to make sure this still passes tests and everything.
Eclipse
Comment #77
EclipseGc CreditAttribution: EclipseGc at Acquia commentedAnyone know why that doesn't appear to have been queued for testing? Well here's another anyway. With the context system now at block's disposal properly we can abstract the cacheTag retrieval for virtually all blocks that require context. :-D
Eclipse
Comment #78
Wim Leerss/avaialable/available/
s/#This/# This/
Let's put single quotes around the example, and add a trailing period.
s/url/URL/
Testbot is broken ATM: #2467621: Tests not being added to the testbot queue… :( Almost my entire workday already…
Comment #79
Wim LeersThe #77 interdiff looks awesome! Generically doing this FTW! Except… I wonder if we actually want
\Drupal\Core\Plugin\ContextAwarePluginBase
to do that already?ContextAwarePluginBase
could implementCacheableDependencyInterface
and then do that.And then it actually could and should also do the analogous thing for
::getCacheMaxAge()
+::getCacheContexts()
. Because the context that the block depends on may only be valid for a limited amount of time (max-age) or may vary depending on some context, e.g. negotiated language (contexts).Comment #80
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, I think this addresses everything in 79.
Eclipse
Comment #83
dsnopekIssue summary changes: Added steps to test and put a link to the comment which shows the UI changes.
Comment #84
dsnopekIn manually testing this (following the steps in the issue summary), the aggregator block wouldn't show up until I cleared the cache. With all the caching changes lately (including enabling the page cache by default: #606840: Enable internal page cache by default), maybe this is the cause of the tests failures on the latest patch?
Also, the change in #76 that removed
ucfirst()
(per Wim's review in #75.2) causes the field label in the aggregator block to be "feed value" (with a lower-case first letter) which doesn't match Drupals normal capitalization standards:Looking at the patch, it looks like it's setting the label for the context to "Aggregator feed":
... so, I'd expect that to be what appears on the form!
Comment #85
dsnopekPer the field label stuff from my last comment, is there a reason we don't switch to:
Or even just use the
$definition->getLabel()
as the title straight up, like this:In the plugin annotation, the label is inside
@Translation(...)
where as$context_slot
is the machine name, so this should be better from a multilingual perspective too...Comment #86
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, I'm an idiot... This doesn't fix any review post 80, I just wanted to get the bug fixed.
Eclipse
Comment #87
Bojhan CreditAttribution: Bojhan commentedI wonder if we can just go with the @context as label itself, so in this case "Feed". It might seem rather direct, but at core that is what its about and how we also show many other labels in our UI. For example "Title" in this screen isn't "Block label value".
Comment #88
dsnopekThe latest patch in #86 fixes the issue I had with the 'Aggregator feed' block not appearing until I cleared the cache which I mentioned in #84.
Here's a very cut down version of the steps I followed:
drush cr
then the block would appearBut with the patch from #86, the block will appear filled with data correctly at step 5!
(Note: this problem only happened with these specific steps - adding the aggregator feed then running cron before adding the block didn't have this problem...)
Anyway, it's fixed now!
Comment #89
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, wrote a test for 56.1 and fixed all the rest of the issues I know about with this.
Eclipse
Comment #90
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI had to have a fall back to the context_slot string because the label can be NULL for a context, so we can't depend on that solution. Still pretty minor all things considered and much prettier when the plugin is setup nicely.
Eclipse
Comment #92
EclipseGc CreditAttribution: EclipseGc at Acquia commentedComment #93
dsnopekAwesome, it's looking good to me!
I had another random question/comment about the field title:
Is running this through
$this->t()
really necessary? My understanding of@Translation()
in annotations is that the string gets run throught()
when the annotation is evaluated:... from https://www.drupal.org/developing/api/8/localization
If this is just about sanitizing
$context_slot
in the case that it's used, we could useString::checkPlain($context_slot)
when setting the$title
variable, right?Anyway, I don't do as much Drupal 8 development as you guys, so maybe the
$this->t()
is necessary or part of a coding style thing. If so, please just let me know and I'll stop nit-picking this miniscule bit of code. :-)Comment #94
EclipseGc CreditAttribution: EclipseGc at Acquia commentedBlah, you are correct. We do need to make the strings translatable if they aren't already though, so we conditionally swapp $this->t() or $definition->getLabel().
Eclipse
Comment #95
EclipseGc CreditAttribution: EclipseGc at Acquia commentedOk, we can't really use t() effectively this way, so I've just removed it. We'll file a follow up to make the ContextDefinition annotation label required which will solve this weird situation.
Eclipse
Comment #96
dsnopekThanks! This is RTBC as far as I'm concerned. :-) But I'll leave it to someone more "D8 qualified" to do review and actually update the status.
Comment #97
Wim LeersWould you consider moving the test coverage + associated bugfix in #89/#56.1 into a separate issue? We could get that committed easily, and it's a bug independent from this issue; it was merely uncovered in this issue. You get progress, we get a simpler patch to review.
An almost comprehensive review:
Where does this magical string come from? Where is it documented?
Correct me if I'm wrong, but these changes could also be moved into an issue of their own?
Also: missing test coverage.
Why can the label be NULL?
But this base class doesn't use that trait, so shouldn't we do this conditionally?
It looks like
class_uses()
is the only option PHP gives us here.Is aggregator the only place that uses/needs this?
The IS doesn't mention aggregator at all. Really, the IS is quite vague, having read it, I barely understand what this issue is about, and I definitely don't understand the consequences. I think it'd be *very* helpful if you could list the concrete consequences and benefits :)
block context? I don't think the context is block-specific at all?
s/aggregator.feed:uuid/'aggregator.feed:UUID'/
I think everything after the comma can be omitted. Just everything before the comma is sufficient to understand the next line.
These look very bizarre. But not a problem introduced by this patch.
But that DX looks … painful. So much repetition, or what looks like repetition, it's slightly different every time. I fear this will lead to a lot of frustration.
See #56.6.
Nice! :)
This looks like a regression though. If the items used in::build()
would use entity rendering instead of custom rendering though, then this wouldn't be a problem.Not the fault of this patch, but unfortunately this patch shows again how crappy this block's rendering is. I'm happy to help with that.Oops, no, 2. deals with that :)
Comment #98
EclipseGc CreditAttribution: EclipseGc at Acquia commentedI'll try to get the IS updated to more clearly communicate what this issue is doing.
Thanks for the review Wim, I'll post a patch including 97.7 in the next update.
Eclipse
Comment #99
tim.plunkettHere's the interdiff from #65 to #95.
I think we need more tests for some of these changes.
Comment #100
Wim Leers#98:
BlockBase
didn't previously return anything for those, because that didn't make sense. So there's no test coverage for those methods in particular. The existing test coverage lives at\Drupal\block\Tests\BlockViewBuilderTest
. But for this, you'll want an integration test that has several dummy contexts, and that verifies that their cacheability metadata is merged as expected by these methods.You asked me in IRC what test coverage is needed:
This looks like magical code, needs test coverage exerting several different scenarios.
The necessary test coverage for this is already explained above.
Needs a unit test probably.
Hrm, weird, shouldn't this already have test coverage in HEAD?
If it should, then please move this into a separate issue, so we can get that in already.
Comment #101
dsnopekComment #102
jibranComment #103
BerdirHere's a reroll, proudly presented by the delayed Swiss LX 327 flight to Zurich :p
No major problems during the reroll, updating the block list builder and block repository to use the new block repository was already done as part of applying the patch, so that is not visible in the interdiff.
The interdiff just contains changes to update the new aggregator feed context to the new API. Which is IMHO *so much* easier. No more messing with block entities and parsing their configuration. We get a list of UUID's to load, can query them, load the feeds and return them. Trivial :)
Didn't run any tests but manual testing worked fine.
Oh, and cacheability metadata seems to be bubbling up just as it should. Tested manually that the aggregator feed tag is added to the page.
Comment #105
Fabianx CreditAttribution: Fabianx for Acquia commentedThis is an _awesome_ win!
This shows exactly why the other issue was critical, because else this would have had to load all things or have to implement its own complicated API as seen above.
+1
Do we really need to load all the feeds for that to work?
Isn't the ContextDefinition with label and uuid enough?
Or does entity storage not allow to just get a list of defined properties?
In any case we should not be setting the runtime context here, then we can at least rely on GC to cleanup the feed object, but I think this could explode quite quickly and we have seen things like that explode in D7 (with bean), so this is also a more generic question.
Comment #106
Berdir2. You can't get the label of an entity without loading it.
And the context system still works so that all possible context is loaded and presented so that you can select one.
That's not different from the current code, although that could now very easily use an entity selection widget to select a feed with a nice and scalable autocomplete textfield.
So, yes, I'm interested to get this functionality into core but I'm not convinced at all that aggregator feed blocks is a good example with the way context configuration works now.
The way I solved that with my static context patch in page_manager is to have a configuration form to select a specific entity using such an autocomplete widget and then just expose that as available context.
I understand that it is useful as an example to demonstrate this. But given that this is a normal task (given the current state in of block context in D8, it could probably also be classified as a major bug) I don't see how we can get this committed given that this comes with data changes.
So my recommendation revert those changes and just get the UI and other needed fixes in. And write some tests using a test block plugin.
Comment #107
Fabianx CreditAttribution: Fabianx for Acquia commentedThat sounds good, indeed major bug.
I still think we are able to omit the:
$context->setContextValue($feed)
here and only rely on the definition and label.
Comment #108
BerdirAlso, this conflicted already again with #2509600: AggregatorFeedBlock should return render array, keeping the changes in the build method minimal and not removing the if.
Comment #110
BerdirAccidently defined the service in block.services.yml, that wasn't the idea.
Comment #112
EclipseGc CreditAttribution: EclipseGc commentedHaving reviewed the interdiffs, I'm very onboard here. I think this patch might pass the test, some very very minor string fixes for the most part.
Eclipse
Comment #113
EclipseGc CreditAttribution: EclipseGc commentedoops
Comment #115
EclipseGc CreditAttribution: EclipseGc commentedOk, the block tests for contextual block satisfaction needed to be updated for the new block UI changes and the block placement screen needed to have the context repository. Pretty simple stuff.
The interdiff is missing docs on the contextRepository property, but the full patch has them. Apologies.
Eclipse
Comment #116
EclipseGc CreditAttribution: EclipseGc commentedComment #117
Wim LeersWe somehow completely lost track of this.
What's still left here? AFAICT this part from #106:
Comment #118
dsnopekre #117 and #106:
At the SCOTCH meeting today we discussed doing exactly that: splitting the aggregator feed block changes out into it's own issue, and having this issue just be about API and UI changes to make it possible. EclipseGC is going to work on it!
For what it's worth, the tests already in this patch seem pretty good! So, I don't think anything additional needs to be added, just the aggregator changes removed, and the issue summary updated.
Comment #119
Wim LeersSounds great! Let's get this done.
Comment #120
tim.plunkettRemoved the aggregator stuff. Also fixed a bad reroll.
Comment #121
tim.plunkettNoticed extra code in the BlockListBuilder.
Comment #122
tim.plunkettNot posting *another* patch right now, these could even be fixed on commit:
Tests
Remove these :)
Comment #123
Berdirmight have been discussed before, but why this change exactly?
I know we already use that term in this context, but context_slot seems quite weird to me. But I guess it's just consistent to continue use it.
Yet another thing that doesn't properly consider optional contexts btw: #2484645: Assigning context mapping: allow empty selection for optional contexts.
are a => is a?
Comment #124
Wim Leers#123.1: I asked the same in #56.1, then in #75.1, and in #76.1 @EclipseGc suggests it's to fix a bug, but he hasn't fixed the test coverage yet.
Comment #125
dsnopekI created an issue which just has the aggregator changes from #115: #2550199: Add a ContextProvider for Feeds
Comment #126
EclipseGc CreditAttribution: EclipseGc commentedComment #127
Wim LeersThe beta evaluation in #92 is very outdated. This issue does not touch caching at all anymore, that was done in #2375695: Condition plugins should provide cache contexts AND cacheability metadata needs to be exposed.
I guess this is the test coverage for #123.1.
But I wonder if it's a test-only thing, the use of
Translation
versusTranslationWrapper
?This needs a much clearer description, for example:
Don't we need test coverage for this new part of the UI?
No test coverage for these UI changes either.
The comment now seems to apply to the
setContextMapping()
call too. This is misleading.Let's add another comment just before the
if
-statement?Why do we need to check if
$condition->getContextDefinitions()
contains anything, if the body of theif
statement doesn't use that?It'd make more sense if we were checking if
$values['context_mapping']
was not empty.So, what am I missing?
I only see additions here, no removals. together with the fact that we haven't changed
ContextHandler
at all, this makes me wonder how this works at all in HEAD?So this is the only UI test change. Which means we don't test it.
Shouldn't we have at least a cursory test?
For example, this could be expanded to use the context mapping UI, couldn't it?
(Or, preferably, an additional context-aware block that doesn't use the
CurrentUserContext
.)This suggests missing test coverage?
Comment #128
Frando CreditAttribution: Frando as a volunteer commentedEdit: ignore, patch includes cruft from git gone havoc. #133 it is.
Comment #129
Frando CreditAttribution: Frando as a volunteer commentedComment #130
Frando CreditAttribution: Frando as a volunteer commentedEdit: ignore.
Comment #131
Frando CreditAttribution: Frando as a volunteer commentedEdit: ignore.
Comment #132
Frando CreditAttribution: Frando as a volunteer commentedEdit: ignore.
Comment #133
Frando CreditAttribution: Frando as a volunteer commentedEdit: Ignore #128 - #132. Git played games with me, sorry for the spam.
So, I was trying to get this to work with #2561935: Create a block for every field (which'll take the entity as context) and the core block layout UI (not with page manager), and it threw errors about no context being present, even though the block placement edit UI showed the mapping correctly. I started to debug, and after an hour arrived at attached interdiff. Not sure if this is correct, but without, the context is lost, because only the block ID and not the already-assigned context is transferred when BlockViewBuilder creates a #lazy_builder.
Didn't address anything else, was for the moment just trying it out and debugging to get the context assignement to work at all. Interdiff does not include rebase on HEAD (no intervention needed but some moved hunks).
Setting to NR to get a test run.
Comment #137
Fabianx CreditAttribution: Fabianx for Acquia commentedNot sure what Wim's opinion is, but we talked a lot about context dependent plugins in the lazy builder issue:
#2543340-50: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) and following has the reasoning we came up with.
It seems context aware blocks do not work, because some Block* manager injects the contexts prior to displaying, which means indeed that - while the context is available in theory (we are in the same request context) - the lazy builder does not have the necessary information on where to get it.
Edit: It is _this_ issue which does that and hence it needs to work around that, core is fine.
Therefore I think opting out blocks with the one line fix of #133 is the right way to go.Could we push that to another major bug issue though as follow-up to #2543340: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) - as it could easily get lost in here?Thanks for working on that.
Context is unfortunately not much supported in core and such bugs like these are hard to find and fix - if they don't make tests fail.Comment #138
Wim Leers#133 + #137: See #2543340-41: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered) and #2543340-48: Convert BlockViewBuilder to use #lazy_builder (but don't yet let context-aware blocks be placeholdered). Please read those comments in full.
In a nutshell:
#lazy_builder
, but then we have to read the block plugin's annotation, look at the contexts it may consume, and pass those in the#lazy_builder
's argumentsComment #139
Fabianx CreditAttribution: Fabianx for Acquia commentedIgnore #137, an extra issue is not needed, the problem is introduced in this code.
This is the problem.
You are not allowed to inject the contexts here, but will need to do so in the callback called by #lazy_builder.
Then lazy_builder continues to work and the opt-out is not needed.
Comment #140
Frando CreditAttribution: Frando as a volunteer commentedYes, this makes totally sense. Works as expected.
#138: Do we have to pass the context annotation to #lazy_builder? We load the plugin anyway later on, and the config is present then anyways I think (or not?).
Other feedback still not addressed, and this needs tests. I likely won't be able to work on this in the next week.
Comment #141
tim.plunkettThis change should fail tests. If not, we are missing coverage.
Note the last line of the code I highlighted. We check access here, and we ABSOLUTELY need the contexts applied before calculating that.
Both of these services should be injected, unless this hunk is reverted.
Comment #142
Frando CreditAttribution: Frando as a volunteer commented#141.1: Right, I just realized this too. I misread the code in
BlockAccessControlHandler::checkAccess()
that injects contexts - it only injects contexts into each of the block's conditions, but not into the block plugin itself. So this hunk has to be reverted, yes.It also makes me wonder though if we shouldn't just move the context injection right into block loading, because this seems really easy to get wrong: Whenever you load a block (to display it somewhere, I think block usage in D8 contrib will extend to other places than just the region assignment) you have to inject context, otherwise access cannot be checked - so why not inject context directly when loading the block, or within the access handler? Are their situations where this is not desired?
#141.2: It's a static method, I don't think we can use DI there
Comment #143
tim.plunkettHm, I was reviewing the interdiff, not the patch.
This is the actual change made in the patch...
Because in HEAD, \Drupal\block\BlockAccessControlHandler::checkAccess() has the getRuntimeContexts() and applyContextMapping() calls.
Comment #145
Frando CreditAttribution: Frando as a volunteer commented# 143: Isn't that just an empty line that sneaked in?
Otherwise I don't really get what you're refering to - yes, in HEAD (and unchanged in patch) contexts are injected in BlockAccessControlHandler::checkAccess(), and this is why, I think, we don't have to inject contexts in BlockRepository::getVisibleBlocksPerRegion(). That's what my last interdiff did, and what you seemed to imply in #141 would be a nogo.
Comment #146
Fabianx CreditAttribution: Fabianx for Acquia commented#145: Your patch is correct. The interdiff was what was confusing.
Also it is confusing that there are conditions and contexts.
BUT:
conditions == access - work great on HEAD currently - unchanged here
context == variation - does not work in HEAD, is changed here
Tim thought you moved the conditions around, but you only moved the context, so it is all good :).
If you want to inject the things, than you have two possibilities:
- Load the block_view_builder via getting the entity view builder for block and calling a method there from the static method
OR
- create a service for building blocks
OR
- expose block view builder as a service (no idea how that would work).
Overall:
I don't think its possible to DI that properly, so I would make that a follow-up.
@Tim: Thoughts?
Comment #147
tim.plunkettStraight reroll first. s/TranslationWrapper/TranslatableString
Comment #148
tim.plunkettWorking on tests for this.
Comment #149
tim.plunkett#127
1) This mimics the use of @Translation in plugin annotations.
2) Done
3) Yes, see 9
4) Yes, see 9
5) Done
6) Investigating next, not addressed here.
7) Reverted, that seems to be cruft from earlier patches.
8) Yes, see 9
9) See 3/4/8... :) Yes, I added test coverage for the UI here. It revealed a schema issue, glad to catch that now.
10) I don't personally think this needs coverage, it just enhances the UX as per this change:
Comment #150
tim.plunkettOkay I looked into #6 more.
The getContextDefinitions() check is a copy/pasted performance micro-optimization. In this method we update the context mapping of both the block and of each condition, yet in the second one we check the pre-munged values. This fixes that.
Comment #151
tim.plunkettSomeone added a beta evaluation since last tagged.
Comment #152
dsnopek@tim.plunkett: Thanks for picking this up!
I did some manual testing with this and this CTools patch #2561935: Create a block for every field (which'll take the entity as context) (which adds a bunch of context-aware blocks) and everything worked great! (Although, I did find some things to improve in the CTools patch ;-))
Unfortunately, the code changes here are a little beyond me for the most part. :-/ But the tests look correct to me. So, from the outside I think this is doing all the things it should be doing.
I only noticed one nit-picky thing:
Not sure why the translated label is commented out? If it works, we should definitely use the Translation object. If not, then we should put a comment as to why it doesn't.
Comment #153
dsnopekA few more thoughts after further testing:
This may be from me misunderstanding how contexts work in D8, though. :-)
Comment #154
dsnopekre #153.2: It looks like @Berdir brought up the same thing in #123.3 and there's an issue to fix the code as it exists in core - #2484645: Assigning context mapping: allow empty selection for optional contexts. It sounds like @Berdir was saying that this patch (as of #123 - it might have changed) was making things even worse? I'm not sure if it makes sense to merge that fix in here because now core will be using it, or if it should stay a seperate issue...
Comment #157
tim.plunkettConflicted with #2458763: Remove the ability to configure a block's cache max-age.
Addressed #152.
Also removing as many unnecessary changes as possible.
16 files changed, 294 insertions(+), 13 deletions(-)
Comment #158
EclipseGc CreditAttribution: EclipseGc commentedPlease fix.
Otherwise this is looking pretty great.
Eclipse
Comment #159
tim.plunkettI revisited the @Translation change again, and it *is* out of scope. So is the other change to submitting conditions.
Fixing the @todo as well.
14 files changed, 222 insertions(+), 10 deletions(-)
Comment #160
dsnopekTook a stab at updating the beta evaluation.
Comment #161
japerryI agree that the translation part should be removed for now, since the test written doesn't appear to be right from the beginning.
If there is missing test coverage for translation, it probably can be taken care of in a future issue.
Comment #162
Bojhan CreditAttribution: Bojhan commentedWe are not changing UI, yay! Looks good overall, I hope we can find a solid integration point in 8.1
Comment #163
tim.plunkettWe have one weird thing remaining. When you have a context, but only one, this patch is adding a weird UI element.
It doesn't need to, it doesn't convey any helpful information, and it is out of scope here (I found it while trying to reduce changes).
Comment #164
tim.plunkettSomewhere along the way, the original test for #2277981: Provide a service for handling context-aware plugins was broken.
Since the Block UI changed, the xpath needs to change too.
Comment #165
dsnopekRegarding the changes in #163: this was something requested by Wim in #51. I know it's fewer changes, and not something we did in D7 (in the Panels world) but I kinda liked it. Since "Current user" is always available, when you select a block that shows data from the user, it's instructive to see "Current user" on the settings page so you know where this data is coming from.
In any case, I'd be fine with either including on not including that feature - getting this committed is most important! - just to give it's history and say that I liked it. :-)
Comment #166
dsnopekAssuming this passes tests, this should be ready to go! Marking as RTBC
Comment #167
EclipseGc CreditAttribution: EclipseGc commentedCompletely ok with this UI (non) change. In fact I prefer it. RTBC+1
Eclipse
Comment #168
BerdirSee also #2484645: Assigning context mapping: allow empty selection for optional contexts, which is somewhat related, I think it would be a easier to just always make the select visible. I think it does have useful information as you might not actually know what kind of context the block uses and what is used. And as shown in the issue above, it is currently impossible to *not* select a context for an optional context if at least one is available that would match.
Comment #169
tim.plunkettI think that will likely be the solution in that other issue. But the logic around the select is not changed in this patch.
It needs further usability review and discussion over there.
Comment #170
Wim LeersThis looks great to me now. Fewer, more focused changes. A clear scope really helps. #165++ and #169++ regarding my remark in #51: we can do that in the other issue, it does indeed merit usability review.
Comment #171
webchickReviewed the screenshots in #48 with @tim.plunkett, @EclipseGc, @japerry, @mikepotter, et al at DrupalCon Barcelona.
The one thing I had questions around was the title/description of the selector, but this is set per-context definition, which means they can decide what makes the most sense for their use case. +1.
The code has been reviewed by many awesome people, this is a contrib project blocker, let's get it done.
Committed and pushed to 8.0.x. Thanks! :D
Comment #173
dsnopekHuzzah! Thanks everyone! :-)
Comment #175
Wim LeersUnpostponed #2550199: Add a ContextProvider for Feeds.
Comment #176
StryKaizer