Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
#2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager needs an update path
Proposed resolution
General description how we deal with updates
There are some types of updates:
- Updates which changes values:
- For simple cases for which Drupal core controls all the possible values, or it is a free text field anyway (system.site:page.front) , we can just use config API to change the value.
-
For cases for which Drupal core does not know all possible values (like this issue), we need to ensure that the known data is migrated and the unknown data is moved to a backup key like;
{$issue_id}_{$previous_key}
, so that contrib modules can access it and deal with it in their custom update functionality.We need to ensure that we don't loose data or show bad information, so depending on the information we should for example hide the block, in case we could not resolve all the data.
The schema for the backup key has to be created and will stick around.
- Updates which renames the keys
Renaming keys should be doable in the sense, that we need to ensure that the schema is updated. - Updates which require basically both at the same time
Remaining tasks
User interface changes
API changes
Data model changes
Screenshot
Comment | File | Size | Author |
---|---|---|---|
#190 | interdiff.txt | 600 bytes | dawehner |
#190 | 2528178-190.patch | 17.24 KB | dawehner |
#189 | interdiff.txt | 786 bytes | dawehner |
#189 | 2528178-189.patch | 17.26 KB | dawehner |
#185 | interdiff.txt | 1.31 KB | dawehner |
Comments
Comment #1
catchComment #2
xjmComment #3
jibranAre we adding tests here as well? There are some issues #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface and #2514784: \Drupal\views\Plugin\views\argument\ArgumentPluginBase should allow subplugins to specify a more specific url cache context in which we have written update hooks but are blocked due to tests.
Comment #4
catchAt minimum we need to ensure existing update testing executes the update implicitly and that the database dump has relevant data, but assertions that it properly completed would be good too.
Comment #5
xjmComment #6
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere we go. The first patch has only the upgrade path + tests and the second one also contains #2354889-228: Make block context faster by removing onBlock event and replace it with loading from a ContextManager.
Comment #9
dawehnerI mean it doesn't test the language aspect of things yet, but well, I think its fine to not require to cover all usecases.
From my point of view, this is looking great!
We should throw an exception for non existing entries, so that contrib could recover and fix their contexts ...
Comment #11
jibraninterdiff to fix update hook docs.
Comment #12
larowlanDo we normally do this?
In many instances $visibility is null, eg see block.block.bartik_search.yml - we should make sure that doesn't error either
Comment #13
larowlanprodding
Comment #14
larowlanaddresses 9, 11, 12
Comment #15
dawehnerNot a bad idea ...
Should we explain why we deal with the config objects directly?
Berdir suggested to use a drupal_set_message() and skip unknown entries, so the update proccess itself runs through and people can fix their files manually OR actually more realistic, contrib modules can run their own update functions to update the remaining parts.
Comment #16
larowlanFixes #15
@catch or @xjm - could you add @jibran to the commit credits as I used his interdiff.
Comment #17
dawehnerGiven that @amateescu tested it manually I think this is good as far as it is.
Comment #18
larowlanAdded a test for the unknown case.
drupal_set_message isn't respected in update hooks, but returns are.
for unknown cases, we also need to disable the block in order to prevent hosing the site.
Comment #19
tim.plunkettStupid question: now that they're both RTBC, why not merge them?
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedDiscussed with @dawehner and @Berdir, we think that getting it in separately is better for review-ability reasons :)
Comment #21
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe parent issue is in, re-uploading the last patch.
Comment #22
catchThis is handy. Should be 8.0.0-beta though no?
Would be good to document exactly why we avoid the config entity API - this is going to be the first config update in core, and it'll get copied from a lot.
So just thinking this through:
- if we throw an exception here, then the update doesn't complete, and then the site will likely throw an exception because this update hasn't run, meaning the only option would be for the site admin to roll back to a previous version of the code base. Which because other updates may have run means restoring a database backup.
So yes, not throwing an exception is good :)
However more questions:
1. Can we put the block config names or some other identifying information into the message, so give a hint which block needs to be sorted out?
2. There is no test coverage for a. the block with invalid context being disabled b. whether editing it works without throwing an exception or similar.
It would be good to have coverage for that. I see the data in the dump, but not the post-update checks.
Or if (and only if) it doesn't work maybe rather than disabling the block the visibility condition should be removed instead? That risks theoretical information disclosure but could leave the site in maintenance mode after update if we're worried about that (I'm not really during beta, but this is worth thinking through for similar issues later).
Glad this still works, phew.
Comment #23
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThanks for the review! Working on it now.
Comment #24
catchOn point #3 the advantage of removing the visibility information from the block would be that the configuration is valid from the point of view of system consistency, even if it's incorrect from the point of view of the previous data.
Also the site admin won't be able to get it corrected until the contrib/custom module providing the update has been updated no? So we might need to add a note about that.
Given there'll be a delay between the update and sorting out the contrib module, I'm not sure about leaving the blocks disabled with invalid configuration - seems like that could store up problems for later - i.e. maybe the admin never fixes the block or saves it again, then the missing key would be there for later updates too.
Comment #25
BerdirI think for visibility contexts, it's pretty safe.
If a visibility condition context is missing, then we consider that block to be inaccessible.
The only scenario where something could be shown when it is not supposed to is if you have an optional context and the condition allows access when the context isn't there. Then it would be possible that the context would not be there on a page where it was before and the block is shown.
And saving the block will force you to update your context mapping.
+1 To showing a message that includes the block and visiblity condition but that should be fine then.
Comment #26
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedAddressed all points from #22/#24.. hopefully :)
Comment #27
jibranIn d7 we have
"addtogroup updates-7.x-extra"
that's why I added@addtogroup updates-8.0.x-beta
.Comment #28
xjm(Adding @jibran to the issue credit per #16.)
Comment #29
dawehnerGood idea indeed!
What about using an inline template?
Comment #30
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedNo opinion really, waiting for more people to chime in.
Comment #31
catchSo I think we should both remove the visibility conditions from the blocks, and disable them - if we really want to ensure the blocks are in a safe state they should be internally consistent after the update.
If we don't remove the visibility conditions, then another update could re-enable the blocks (certainly a custom update might do that), and you get exceptions on your site.
Also I'm not sure about for this update avoiding the config entity API.
Certainly if changing config schema you'd want to avoid the API.
For this we're changing a value (although it is the format of the value that causes code errors if not updated, so not just a value change as such), and the config, when saved, will be valid.
i.e. if I just write an update to change the front page setting in system.site, then I'd expect to be able to use the API (and get cache tags invalidated correctly for me etc.).
I know berdir had the opposite view but as with the rest of this it'd be good to make sure we're doing it the right way and document why for the dozens of copy/paste from this update function there'll be later.
Comment #32
catchSo thinking about it more..
The problem with this update is that while we only change a value, the value references a specific service (i.e. we're not just changing true to false or whatever).
This has two implications
1. we can't reliably update all the data in the system since there's no guarantee that contrib modules are up to date at the time the update runs and no way to know the names of the new services.
2. invalid data left behind results in an exception
What if as well as changing the data, we also change the name of the config key.
Then when we know the new value, we just migrate the data and drop the old key.
When we don't know the value, then the stale key in the config object won't trigger the exception, it's just waiting there to be migrated over.
This means another API change for exported config - but again those won't trigger exceptions if they aren't updated yet (i..e from an install profile) and we've just committed an API change for them changing the format anyway.
We can still disable the blocks due to the visibility changes if we want to.
Comment #33
catchOr similarly.
When updating, if the context services aren't found move both those and the visibility to a backup key. Then contributed could handle migration from there but still no exceptions.
Comment #34
larowlanPlaying devil's advocate - do we know of any context providers outside core?
We might be overthinking this.
Comment #35
BerdirAFAIK, the idea is to provide a good example, because this will likely be copied a lot by core and contrib update functions. But yes, it is possible that we're overthinking a bit ;)
Comment #36
andypostAlso glad to get more opinions about language context provider #2529616: Move CurrentLanguageContext to Core\Language\ContextProvider
Comment #37
jibranIn the critical issues discussion meeting on Friday @catch explained that yes we were overthinking a bit here but he wanted this issue to be an example for rest of the issues which would write the upgrade path. @Gábor Hojtsy, on the call, and @Berdir, looking at #35, agrees with this. I'm already using these tests as an example to write an upgrade path test in #2464427: Replace CacheablePluginInterface with CacheableDependencyInterface. NW for #31 and #32.
Some review points.
Let's add @see to update hook instead of issue id.
It's already in the block namespace let's remove block prefix form the class name.
Let's convert this file to gzip as well. Is it a good idea to move this file to block module?
Comment #38
webchickAt 11 (!) criticals, it's possible that the next beta is our last beta. Therefore, we should make this a "beta target" for one to try and get in before the next beta (tentatively July 22) so we could have at least one release with beta-to-beta upgrade paths.
Comment #39
mpdonadioCan someone elaborate on what is being overthought, this test/issue in particular or the upgrade tests in general? I am trying to wrap up #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter before the next beta, and was planning on using this test as an starting point.
Comment #40
dawehnerI try to figure out where we are at the discussion at the moment. I think this should move to the issue summary and be filed out, so we can move forward here.
Configuration update changes
There are some types of updates:
For simple cases for which Drupal core controls all the possible values, or it is a free text field anyway (system.site:page.front) , we can just use config API to change the value.
For cases for which Drupal core does not know all possible values (like this issue), we need to ensure that the known data is migrated and the unknown data is moved to a backup key like;
{$issue_id}_{$previous_key}
, so that contrib modules can access it and deal with it in their custom update functionality.Once we dealt with them in one release we could write another update function for the next release to got rid of them.
Renaming keys should be doable in the sense, that we need to ensure that the schema is updated.
Comment #41
Wim LeersThis now also blocks #507488: Convert page elements (local tasks, actions) into blocks — that issue needs an upgrade path, and it also touches block config entities. Since this issue is supposed to provide the canonical example to look at for config entity upgrade paths, that other issue needs this issue to set that example first.
Comment #42
mpdonadio#2455125: Update EntityViewsData use of generic timestamp to use Field API formatter has passing update path+tests if anyone wants to take a look, if that can serve as an example instead.
Comment #43
webchickI would love to encourage us not to get wrapped up in this issue providing the perfect example. It's an upgrade path blocker, and only has to run successfully on each D8 site one time, so we should get it in as soon as it's ready. We can always refactor update hooks/tests later if we find a more efficient way of doing something.
If we need reference examples, we should be writing better docs instead: #2521776: Update documentation for hook_update_N() for Drupal 8
Comment #44
catch@webchick we can't write accurate docs for config updates (at least not relatively complex config entities like blocks) until we know what should happen with the stored data, which is what's being discussed here. This isn't about efficiency, it's about not losing data when the update runs.
If we didn't want a critical upgrade path issue, we could have held off requiring hook_update_N() until the parent issue of this one was in, which was the plan for a long time anyway. We can always not have this upgrade path in HEAD, and move this issue to head2head, then not have to worry about it until the next critical upgrade path issue for a complex config entity comes in. I'd be fine with that too, except it means we can't support beta to beta updates until after the next beta. What I don't think is sensible is committing knowingly lossy upgrade paths to core.
Comment #45
webchickRight, I agree with "as soon as it's ready" (and not before), which means figuring out where data needs to go. Just saying please don't couple fixing this data lossiness with also trying to flesh out all possible use cases, which is the direction this seemed to be heading, based on some of the recent comments. But by all means, loop jhodgdon in on those use cases at #2521776: Update documentation for hook_update_N() for Drupal 8, since she now has a D8 Accelerate grant to get those docs polished up.
Comment #46
dawehnerWe talked about that on the d8 critical call.
Moved the thing in #40 and updated with the current description.
Comment #47
dawehnerComment #48
jhodgdonRegarding documentation, I'm working on documenting the Update API (hook_update_N() and testing stuff) on:
https://www.drupal.org/node/2535316
If you have comments about the outline or suggestions, please comment on issue:
#2521776: Update documentation for hook_update_N() for Drupal 8
Comment #49
dawehner@jhodgdon
It is a great piece of documentation!!
The doc page about config updates is missing the points discussed in this issue so far, so for example creating a backup key.
Comment #50
dawehnerHere is an update of the patch itself. Neither the patch nor the test is tested yet.
Comment #52
dawehnerAh, this time we actually store the value.
Comment #54
dawehnerThere we go.
Comment #55
lauriiiI was also working on this during my train trip but I'm happy to see a green patch here because this is blocking the other issue! I did manually test this with existing/working cache contexts and missing/broken cache contexts. What I found out was that there was no notification while running this using drush updb. Otherwise the upgrade path seems to be working.
Comment #56
dawehnerWelcome back!
This is a great observation. I tried to have a look at the update batch code in drush but it was not entirely obvious whether they actually provide support for the log messages
which are returned bei hook_update_N() functions. This seems to be more of a missing feature of drush.
Comment #57
lauriiiThanks! My intention wasn't to be so much offline but hotels/airbnbs without wifi took care of that.
Otherwise the patch looks good. I don't still think I'm eligible to RTBC this because I don't have that much experience of update hooks so leaving for someone else to push the button.
Comment #58
jhodgdon@dawehner / #49: thanks for looking at the docs! I totally missed the part about backing up the data. Added to https://www.drupal.org/node/2535454 now.
Comment #59
jhodgdonLet's continue the discussion about docs on the other issue though? #2521776: Update documentation for hook_update_N() for Drupal 8
Comment #60
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIt seems a bit ugly for backup data from an update to have to leak into several non-update-related places like this. What about keeping the config clean and saving the backup data into state or some other kv store instead?
Comment #61
dawehnerI really like a KV/state entry. Berdir, fabianx and myself talked a bit about that on IRC and we went with KV, because state() might be cached for runtime purposes, but this data is not needed on runtime.
Comment #63
Wim Leers#61: oh, that looks so much better already!
Comment #64
xjmUsing
statekv store generally makes a ton of sense. (edited)I think this is one place we missed removing the protected property? Edit: and also why the test failed.
Comment #65
Wim LeersI think a comment like:
would be helpful, because it'd document why only those 3 are listed.
When a user manually fixes a block, shouldn't we also automatically remove the backup keys?
If not, what if the user first manually fixes a block, then the contrib module is updated, and it tries to update the backed up data onto the Block again? Can't that result in conflicts?
I think this is no longer necessary as of the last patch? Pretty sure this is also the cause of the single test failure :)
Out of scope for this issue, but I think this is extremely painful.
Comment #66
xjmMany plus for stating this so clearly in the inline docs.
Should we put a message in the update or possibly
hook_requirements()
or such that the blocks were disabled and need to be placed again?Comment #67
dawehnerThanks xjm :)
This fixes it for me.
Ideally I think we would have some list of available backups and not have to do that manually for every
hook_update_N()
Comment #68
Fabianx CreditAttribution: Fabianx as a volunteer commented+1 to using K/V here.
Besides #65 and #66 I think we are good to RTBC again :).
Comment #69
dawehnerMh, this is a good question. I think contrib modules should also provide updates. In case they do, they would do the same as the user does though, so there is no negative harm in keeping it, I guess?
Painful in which sense, I don't see anything problematic in there? Everything in there is really explicit, which is IMHO always better than short filenames.
Tried to improve the message a bit:
Comment #71
dawehner/me sighs
This will pass, I'm "convinced".
Comment #72
Wim LeersIt does mean though that the operations performed by context providing contrib modules do need to be idempotent though.
Somewhat related: are
Block
config entities the only things whose context mappings need to be updated? In core they are. But what about contrib? i.e. what if other config entities use contexts/context mappings? I guess the answer is simple: contrib modules providing those config entities that use contexts/context mappings are responsible for that themselves.Comment #73
dawehnerWell, they are converting contexts over, and if they actually care remove duplicates. This is all, right?
Yeah, core can just take care about core. Too bad, we haven't invented an actual brain with Drupal 8.
Comment #74
BerdirThe old block context discovery was block module specific. It wasn't really possible to use it with something else.
page_manager for example also uses blocks with context but has it's own context system with separate context ID's.. and wasn't affected by this change at all. It might be in the future if it decided to use the new global contexts from core instead of duplicating them. But then needs to figure out the upgrade path for itself.
Comment #75
tim.plunkettWow, I thought we were further away from finishing this, thanks @effulgentsia for the K/V suggestion, and thanks @dawehner for getting that written so quickly.
+1 for RTBC
Comment #76
catchGiven that the list of condition plugin IDs is dynamic for a site, how can we assume that $condition_plugin_id will be found in $condition_plugin_id_ui_label_map? This has the same problem as the context IDs, except we're not actually updating them.
Since this is only used for the message, I think we could drop it and just show the block label - we do know that and that's enough of an idea where to look. Another option would be the raw ID in the message.
Alternatively could probably get the full list via plugin discovery, but I don't think adding that dependency in the update hook is worth it just for the message.
Now that we remove the bad visibility from the block config, this message is no longer true.
We do still need to disable the block, but that's only to prevent unintended information disclosure or similar and prompt admins to review the blocks that are disabled.
Whole message should really be like:
Drupal::translation()->translate('@label Visibility: @plugin_ids') or similar. Concat won't work for RTL. On the other hand, should we be translating this at all? It's a one-time error message.
Also this looks like XSS on the block label?
Comment #77
jhodgdonI think a few things in this patch could be improved, especially the messages presented *to the user* from the update if it fails, and a few code comments:
This block of code could use a comment because I still am not really sure what it means.
Normally we put comments before the code they are commenting... normally preceded by a blank line and followed by a code line. This block of comments is backwards?
This is a fine developer message, but it's going to the UI. Is a user going to have a clue what "unexpected context mapping key" means?
Also this is a comma splice. Make it two sentences between "mapping key, one or more". Or add "and".
So this is showing the ID of the visibility plugin... is there not a human-readable label that would be better to show? Can we see an example of what this error message would actually look like to a user?
nitpick: mappings are or mapping is, not mapping are
#2455125: Update EntityViewsData use of generic timestamp to use Field API formatter
Comment #78
dawehnerI went with the label on the condition plugin itself.
Well, yeah, I think so.
Good point, let's move things around.
Well, this just happens in case there is contrib going on.
Just used the screenshot from simpletest :)
We, we wrote all those helper functionality to be able to deal with DB dumps, which is more down to the metal of what actually might happen on a real site.
Comment #79
jibranNeeds rebase
Comment #80
dawehnerSure, let's do that quickly.
Comment #81
jhodgdonThe error message seems OK to me. Gets the point across to developers probably, plus I think a non-developer would at least figure out "Oh, something's wrong with my block" and at least know where to look. But one small nitpick: let's say "...contributed or custom module" rather than "contrib module".
A few other minor suggestions:
Nitpick: end buildConfigurationForm with ().
When I first read this, I thought you had by mistake forgotten to delete the first few lines here because you were redefining the same variable. My eye didn't pick out in these long variable names that they were two different variables, and the comment above doesn't really explain why you need those special names for the ones Core knows about, but you need something for the others too. What's so bad about just using the label for all of them anyway? If the labels are so bad maybe they should be fixed?
What I was trying to say above was that this structure of block config objects is a pain to generate, and if you need to make a small change, kind of a pain to maintain, whereas it's easy to export a block config to a YAML file, and then import/parse it in the text. Plus I think YAML in YAML format is easier to read than in PHP array format. But, whatever. ;)
Comment #82
jibranCan we mention at someplace in comment that we are replacing
'user' => 'user.current_user'
withuser: '@user.current_user_context:current_user'
or'node' => 'node.node'
withnode: '@node.node_route_context:node'
? I think having this in the update hook would help contrib developers while looking at the backup keys.Comment #83
dawehnerHa, true.
Its a bit of a bad position. So for example we talk about 'Node bundle' vs. 'Content types'. I changed the code a bit to be more friendly, by just having one variable.
Oh yeah I got what you mean now. +1 to that idea
Comment #84
jhodgdonWay nicer. :) All of my concerns have been addressed now. Thanks!
Comment #85
jhodgdonDang I should know not to look at patches that closely... I was going to mark this RTBC and I found two total nitpicks unworthy of a critical...
nitpick: needs comma after "update".
lables => labels
Comment #86
dawehnerThere we go.
Comment #87
jhodgdonI think this all looks great.
Incidentally, I updated
https://www.drupal.org/node/2535454
(docs page on how to do config updates with hook_update_N() ) today to conform to this latest patch. It seems very clear and I was able to justify all the steps well on that docs page, which says to me these were all good choices to make in the code. I think we should get this in.
Comment #88
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThis looks great to me as well, but as it's our first hook_update_N() for Drupal 8, assigning to catch for review/commit.
Comment #89
jibranAny word on #82
Comment #90
jhodgdonThat is a good idea (#82).
So how about if at the top of block_update_8001() we would put a comment in the function body, something like this:
This updates the visibility context mapping for blocks. Core visibility context plugins are updated automatically; blocks with unknown plugins are disabled and their previous visibility settings are saved in key value storage; see change record ......... for more explanation.
Then I would add something to the change record for the parent issues that explains the key value storage, and link it here.
Thoughts?
Comment #91
jhodgdonThis is the change record for the parent:
https://www.drupal.org/node/2527840
It already explains the data model change. It would need an addition to explain what the update did and where the backup information was stored, and how to retrieve it. Also an explanation of what a contrib module providing one of these contexts would need to do in order to update their plugins. This is NOT clear to me right now.
Comment #92
jhodgdonActually I don't even think the change notice is correct about the data model change. It definitely needs some attention.
Comment #93
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think #90-#92 are all good ideas worth doing, but also not terrible if they happen just after the beta is tagged vs. before (before is better, just not critical). So, re-RTBC'ing in case catch is willing to commit as-is. If someone wants to upload a new patch with only docs changes and/or improve the CR, please don't let the RTBC status discourage you from that.
Comment #94
Fabianx CreditAttribution: Fabianx as a volunteer commentedAgree, on leaving this RTBC, while docs can still be improved. If this gets in before, could still improve the docs.
Comment #95
catchOK I'd hoped to give this a final review and commit, but found more issues.
So this comment could use some clarification I think, and made me realise the points in the rest of the review.
When you are just updating from one valid value to another value (let's say changing from published to unpublished or similar), then updates should use the entity API.
This is because there might be hook implementations that respond to changes in values (say conditionally invalidating a cache tag), and they equally need to know about the change whether it happens in an update or not.
When those updates using the entity API run into invalid configuration because of another update, they'll need to add update dependencies (i.e. to depend on this update hook). Not perfect but that's what we have.
In this update however, we're not changing a value, what we're doing is changing the 'format' of a value for the context mapping. The value is identical as long as it's successful. So not using the entity API here is right.
Except this is not quite true for the whole update because...
Here we're just updating from one valid value to another - enabled to disabled. It is quite feasible that a hook implementation might want to react to that change, so it should use the entity API I think. When we save the block, the config will be valid*.
In irc, @amateescu noted that we could do this in a separate update function - based on the backup information we store in k/v.
This allows for a couple of things to happen:
- contrib modules could update context mappings for things they know about, and clean up the k/v table, so that blocks don't get disabled at all by the subsequent update (by using update dependencies to insert between the two). That's a much smoother experience for site admins since they could still come out of a single update run with a completely intact site then.
- *if there was a second module update that made block config invalid, it could also ensure it runs between the two updates to clean up the data, then the entity API usage should be fine again in the second update here. This is the real distinction between using the entity API or not I think. That is a bit of an update dependency snake pit, but it would theoretically work and is very worst case (two updates from different modules to block config structure + N additional contrib updates based off them).
On disabled blocks, that gets us to this:
When we store the backup values, we only store the context - so k/v gets block IDs + context and nothing else.
This means that we're losing information about the status of the block before it was disabled. Because nothing stops a block having block visibility and being disabled already.
jhodgdon's question about what contrib modules should do here made me realise this - ideally you'd re-enable the block after fixing the context, but you definitely can't do that if the block was supposed to be disabled in the first place. So we'd need to store this in k/v too.
I'm not sure if even then it's a good idea for contrib to try to re-enable the block, but we should keep that data in the system somewhere rather than just the message so it can be referred back to.
All of this made me revisit the suggestion to have a 'context mapping hook' to allow contrib to add their context mappings to this update - but even then not every module might implement it, so while it would save theoretical contrib/custom modules some other work, it doesn't save this update any work/complexity at all. And even if a module implemented that hook, they might do after a site has updated with just core, so generally I think that's best left alone - nothing to do about then, but just noting it.
Comment #96
dawehnerI just doubt that this will work great in reality. You'll start with updating core, and well contrib is not updated at that point.
Comment #97
catchYep, that's exactly where I got to:
Comment #98
catchAlso #82/#90 are both good ideas.
Comment #99
dawehnerHey, drupal.org please don't forget my patches. Well then let's adress #82 and #90 as well.
Comment #100
dawehnerI think I took into account now everything.
Comment #101
Fabianx CreditAttribution: Fabianx as a volunteer commentedthat message should definitely move to 8002. Else contrib cannot fix the data in between 8001 and 8002.
Comment #102
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think #95 gives great reasons for using the entity API to do the block status change, so +1. But, I think there's a potential problem. What if block.module has a future time in which it creates a block_update_8003() that makes another format change? In this case, there's no way to setup dependencies to make block_update_8003() run before block_update_8002(). But the codebase is now such that block_update_8002() is loading and saving entities with invalid formats, which either the entity API itself or another module's hook_block_(load|save|etc.)() might choke on. I discussed this with catch, and he suggested that the way to deal with this will be to at that time empty out block_update_8002() and move its implementation to block_update_8004(). That seems fine to me.
1) Anyone foresee problems with this suggestion or other problems with using the entity API like this during an update?
2) Where's the right place to document this?
Comment #103
catchI thought of one other way to deal with this. If that second format-changing update was added in 8.4.x, then we could remove block_update_8002() and previous, and implement hook_update_last_removed() - forcing people to update incrementally via the minor releases. So the update hook copy-and-empty only needs to happen if there are two API-breaking changes to the same thing within the same minor release. For core I hope we don't run into that, I'm sure contrib will but this at least points to a way to deal with those I hope.
Comment #104
jhodgdonAlso, remember that the entire doc block before a hook_update_N() function is displayed to the user when they run the update.php script. So you need to just have an explanation that is somewhat "user-friendly" in that doc block, and put the rest in a code comment inside the function.
Comment #105
dawehnerReally good point. Moved it.
Yeah that is a bit sad, that its working in that way.
I'm curious, does this issue still needs change record updates?
Comment #106
jhodgdonRegarding change records, yes. Right now if I were a contrib module maintainer trying to figure out what I needed to do to update my contrib module, I would be clueless. The existing change record is (a) inaccurate in the Before/After data model (at least, it doesn't match the parent issue summary version), and (b) doesn't tell me how I would update a contrib module.
Comment #108
dawehnerFixed the failures.
@jhodgdon
I agree that the CR is by far not perfect. Do you think we need to fix that before we can finish this issue?
Comment #109
jhodgdonRE the change record: I think so, actually. Right now I have no idea how a contrib module would do the update. Until that is articulated, I wouldn't be sure it is feasible or defined, and since this issue is about making sure there is a viable update path, it seems like that would be part of the issue?
Regarding the current patch, some of the docs are a bit garbled:
these lines go over 80 chars (nitpick)
==> In this update function, however, we deal with
==> ... in the next update function, however, we can use Entity API.
Nice. Let's get this information into the change record too.
But... In the parent issue's summary, it says that the new format is:
node.node@block.node_route_context
But here it's
@node.node:block.node_route_context
One of them is wrong. I think the change record agrees with what's in the code here, but the issue summary on parent disagrees. Would be good if they all agreed.
Comment should end in . and probably drop the extra line between comment and code?
nitpick: which -> that
And maybe this should say "from the previous update" in this comment?
Or better yet:
Disable all blocks whose visibility context could not be updated.
its -> it's
and the comma should be a ; or a .
Comment #110
xjm@catch said @jhodgdon's feedback above includes what he was going to post, so unassigning for now to make it clear this is actionable. :)
Comment #111
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSetting to needs work, per #109 and #110.
Comment #112
dawehnerI hope I didn't skipped something.
Thank you @jhodgdon for your fast feedback.
Hei, it was consistent on being wrong.
Yeah that was earlier versions of the earlier issue. We changed in the meantime. Will work on it after posting the patch.
Comment #113
catchI found two more nits which could be either ignored or fixed on commit, very minor:
Do we want to explicitly note that contrib modules can insert their updates using hook_update_dependencies() to update context mapping keys and clean up the k/v so that blocks don't get disabled. That could also just live in the change record though.
Should this say 'unknown context mapping key'?
Not RTBC-ing mainly because it's late and I've done a few quick reviews rather than long one since #95. I really hope I don't have anything left though.
Comment #114
dawehnerJust a good night patch.
Comment #115
jhodgdonI fixed the parent issue summary so it says node: @node.node_route_context:node now.
I also think this latest patch is good to go so I am +1 on RTBC, except that the change record has not been written, so I still don't really understand how a contrib module would update itself. Some of the recently-added comments do help a bit... but not totally there and it needs to be on a change record (and since we've linked to the existing one https://www.drupal.org/node/2527840 for the parent issue, that is where it needs to go).
Not sure if the change record update needs to happen before or after commit; will leave up to committers on that one, but my preference would be before since contrib cannot really update without that information.
Comment #116
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #102, here's a follow-up proposal: #2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates. I filed that as Major, not Critical, because there are other ways of handling things in the meantime, such as what's discussed in #102 and #103.
Comment #117
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRTBC'ing for catch to make the call on whether this is committable before the CR update addressing #91 is done. If someone writes it in the meantime, then he won't have to make that call :)
Comment #118
chx CreditAttribution: chx commentedWell then this is a good night very quick review. Just a very quick scan, I had a long flight from New York.
What about
array_column(\Drupal::service('plugin.manager.condition')->getDefinitions(), 'label')
instead? We did go PHP 5.5 so let's use it.is this allowed in the Drupal coding standards these days?
on the other hand, I always recommend (very strongly) an array_walk and a closure instead of foreaching with references. This is not PHP 7, there be dragons. http://drupal4hu.com/node/384.html
baloney: baloney.spam
sniff, i liked when bunnies were used for nonsense much better ;)
Comment #119
chx CreditAttribution: chx commentedSorry, crappy hotel wifi crashed before that was posted last night and now I can't reassign to catch :/ please someone do. The above are nits, no need to hold up a critical.
Comment #120
dawehnerOh nice! Yeah we should use PHP 5.5 and educate people by using it.
No idea, but I don't think its that bad.
Love it.
BTW. currently we not use the human readable name for the context IDs, since the extra update function.
Comment #121
dawehnerThe array_walk at the end was a little bit too much of a rewrite, as we have
unset($visibility[$condition_plugin_id]);
This though triggered some thoughts about the other code, which indeed seems to no longer try to resolve the label of the context.
Comment #122
dawehnerThis time again with
Comment #123
catch#2538108: Add hook_post_update_NAME() for data value updates to reliably run after data format updates brought up another issue, which is that Config::save() itself fires an event. That puts us into a catch-22 though, since any lower than the config API and we're dealing with storage classes or the database directly - which then wouldn't run on sites using alternative configuration storage. That's an update infrastructure issue and this issue can't deal with it, but I think we need a critical follow-up to figure out what to do with that, so I've opened one at #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly.
Comment #124
Fabianx CreditAttribution: Fabianx as a volunteer commented#122 is still RTBC (x-post with #123)
Comment #125
catch@effulgentsia pointed this out on that issue:
Hesitate to say this, but should the first update here be using that to avoid the event?
Comment #126
dawehnerThis itself doesn't work, the test fails due to some problem on the actual site.
Comment #127
catchBumping back to CNR rather than CNW - if this is workable I think it's the right approach, if it's not then we need to discuss whether to keep going here or not.
Comment #128
Wim LeersThat doesn't invalidate cache tags of the affected blocks. Simple config cache tags are invalidated not in config storage, but in
\Drupal\Core\Config\StorableConfigBase::save()
.\Drupal\Core\Config\CachedStorage::write()
will not invalidate cache tags.Comment #129
dawehnerAfter quite some debugging the problem is in
This storage relies on config events, which is why it no longer works. Sadly it also doesn't support clearing and rebuilding at the moment.
Comment #130
effulgentsia CreditAttribution: effulgentsia at Acquia commentedThat's fine. block_update_8001() doesn't and shouldn't perform any change that affects anything whose cache depends on the block, so those cache tags don't need invalidation. Only the blocks that are disabled in block_update_8002() need corresponding tag invalidation.
I'm unclear on what specifically this means. So far, I found that there's a problem with getting the keystore synced (table=keyvalue, collection=config.entity.key_store.block, name=theme:bartik), but I think that might be a problem with the drupal-8.block-context-manager-2354889.php fixture, which adds to only the {config} table without also updating that keystore.
Comment #131
tim.plunkett\Drupal\Core\Config\Entity\Query\QueryFactory::onConfigSave(), which is subscribed to ConfigEvents::SAVE, calls $this->updateConfigKeyStore().
I think that's your problem, and what @dawehner was referring to.
Comment #132
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRight, but what's saved is just the ID of the config, which block_update_8001() doesn't change. So the problem I think is that the test fixture creates a pre-update db state that isn't what a site would actually have. And if it created the correct state, then I think #126 would pass.
Comment #133
Berdir#132 makes a lot of sense. There's nothing here that would *change* whatever we have indexed there, so there shouldn't be a need to update yet.
Yes, other config changes might have to do exactly that, but I don't think we need to solve that problem here.
Comment #134
dawehnerDoes that mean we should config API to import our entries?
Comment #135
dawehnerHere is that particular setup but honestly, this now fails on testing, as there is a /user/login which tries to render the blocks.
We have some ways to fix that:
$update_free_access
and expect people to do the same.Comment #137
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think /user/login should be expected to work when your database isn't up to date. For people (as opposed to the test), I think the options are either $update_free_access or be logged in as someone who can run update.php before deploying your code update. I think ideally, our tests would use the latter approach, but if that's hard to do, then I think $update_free_access in the meantime is ok.
Comment #138
xjm@alexpott, @catch, @effulgentsia, @webchick and I discussed this issue today and agreed that it needs to remain critical and be resolved in core before the next beta. We discussed the option of temporarily reverting #2354889: Make block context faster by removing onBlock event and replace it with loading from a ContextManager until this was finished, but determined that would be infeasible at this point without adding additional critical work unnecessarily.
We also discussed whether adding the update to head2head instead was an option, but decided against that. We need to ensure that this update can be handled in core and also provide a testable example of a working update for configuration data.
Comment #139
dawehnerAnyone knows how to login a user in a simpletest no using the api?
user_login_finalize(User::load(1));
itself does not work.Alright, let's try out the
$update_free_access
route ... now we run into the problem thatupdate.php
cannot be accessed with just$update_free_access
, let's fix that. ...Comment #140
Fabianx CreditAttribution: Fabianx as a volunteer commenteduhm?
lol :D
Comment #141
dawehnerThank you for your quick feedback!
Ups.
Finally I actually understood why some of the behaviour I've seen on update.php was so weird.
Comment #142
Fabianx CreditAttribution: Fabianx as a volunteer commentedI think we are ready again, so I am giving a RTBC + 1.
Comment #143
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedJust a few cosmetic changes :) I also reviewed the code and I think this is ready.
Comment #144
Fabianx CreditAttribution: Fabianx as a volunteer commentedThen lets make it so.
Comment #145
BerdirWe just discussed on the critical call that we should avoid config validation/casting since that could change over time and our config might no longer match the final schema. But that we do think that running config events is OK.
I'm not sure if we want to switch this back or implement it according to that in #2538228: Document that Config save/delete/rename events may be dispatched during hook_update_N(), what that means for subscribers, and fix core subscribers accordingly ?
Comment #146
dawehnerGiven that this issue was about providing a good example how to exactly do it, I would vote for doing it right here.
Comment #147
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere we go, back to using the config api + the trusted data feature.
Comment #149
dawehnerLet's fix the failures.
Comment #150
amateescu CreditAttribution: amateescu for Pfizer, Inc. commented#147 and #149 implemented what was agreed upon in the critical issue meeting today, so back to RTBC to get committer feedback.
Comment #151
catchSo there is a minor point with dependencies that's still a bit tricky. I think the current state of the patch is OK for it, but writing this up to document at least.
Let's assume:
A: this two-part update function A8001 and A8002
B: several contrib module updates that just want to update context mappings - Ba8001 Bb8001 Bc8001
C: a contrib update function that also changes block entity config schema structure - C8001
With just A and B, it is easy to get an order like A8001 Ba8001 Bb8001 C8001 with hook_update_dependencies()
Once C comes along, this gets trickier, because we need an order like:
A8001 C8001 Ba8001 Bb8001 A80002
Not like:
A8001 Ba8001 C8001 Bb8001 A8002
since that could blow up.
hook_update_dependencies() however doesn't give us anything to guarantee the first ordering, unless either all of A know about C or C knows about all of A.
But what saves us, is that this ordering should be fine:
C8001 A8001 Ba8001 Bb8001 A8002
If it wasn't, we might need to do make block_update_8002() empty and move the contents to block_update_8003() - then document that contrib should all go between 8001 and 8002, leaving space between 8002 and 8003.
But I think just going before 8001 for another structure update would work for this case (and there's no previous update before that to worry about here).
After all that, I think this is OK.
However the patch doesn't apply because block.install got added, and it's 10:45pm on a Friday night.
Comment #152
tim.plunkett#2535284: Move block_install hook to block.install file added that file. Straight reroll.
Comment #153
jibranYay!!! we are really close. Just few minor points and sorry for the distribution caused by #2535284: Move block_install hook to block.install file as it was my doing. :)
Nice work everyone especially @dawehner.
We are not returning any string to display after this update. IMO we should return something.
This is not helpful description in UI. What previous update? It can be more elaborating.
No need to store block storage just make it chained. We are not using it again anywhere in the function AFAICS.
$block_storage
returnsBlockInterface
:)Is there an issue for this to store these UI labels in plugin manager? We store all kind of details for entities.
Why are we calling
\Drupal::translation()
again and again? We should store this in local variable. Something like old times$t = get_t();
we can do$t = \Drupal::translation();
Comment #154
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #153.3: what about using
Block::loadMultiple()
instead?Re #153.6: what about using
t()
instead? Drupal 7 update functions call t(), so I'm not clear why we can't or shouldn't in Drupal 8.Comment #155
dawehnerAgreed.
While I think its a super bad argument to argue with Drupal 7, but I think using
\Drupal::translation()->translate()
is not really better code,just with worse readability.
Well, I disagree with the usage, because we want to educate people.
Not sure what you mean with this. Well, its out of scope to change that behaviour, I think.
Yeah that is overthinking, IMHO.
Comment #157
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedLet's fix it.
Comment #158
dawehneroh, that is embarrassing.
Comment #159
Fabianx CreditAttribution: Fabianx as a volunteer commentedBack to RTBC
If we want to get more nit-picky consider running phpcs on it and fix the too long lines, if not ignore:
nit - should split across two lines.
nit - now longer than 80 characters.
Comment #160
Fabianx CreditAttribution: Fabianx as a volunteer commentedComment #161
dawehnerLet's fix the array, this improves readability.
Comment #163
dawehnermh
Comment #164
jibranThanks @dawehner everything looks super cool now.
Oh! I agree it is out of scope. I just wanted to suggest that we should get this kind of information from plugin manager. For example entity has id, label, bundle_of|bundle_label, singular label and plural label all kind of information maybe we should add it to context services as well. something along the lines of #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed and #23298: Entity bundles should declare a plural form of their label.
Comment #165
jibranIsn't it just a context manager now?
Comment #166
jhodgdonRegarding the t() vs. translate() thing above, I think actually you *must* use t() or the PO extractor will miss it. But I could be wrong.
Comment #167
jhodgdonOh, and +1 on RTBC, as soon as the change record is updated so that contrib knows how to update these things. So far that is still not even clear to me, and I've been at least peripherally involved in this issue. I would actually vote for not committing this without the change record update, because it is a good demonstration that there *is* a viable procedure if someone can document it. If it cannot be documented, it may need some more work. It seems pretty convoluted now, given some of the discussion above, so I'd like to see it documented before this goes in.
Comment #168
dawehnerWhile writing the CR I realized that this doesn't work yet perfectly. In order to update those entries you need to know at which conditions your context IDs are missed,
working on an update
Comment #169
dawehnerThere we go.
Comment #171
dawehnerThere we go, let's fix the test failures.
Comment #172
jibran#169 got me thinking, Is it a good idea to add a test module which implements an update hook which runs between block_update_8001 and block_update_8002 and then test that it works fine?
Comment #173
dawehnerWell for me this is not about the actual update path of contrib modules, because all of them are handled by one person anyway, but more about the level of thinking
about the problem space.
Comment #174
BerdirHm, was hoping to be able to RTBC this but this doesn't look ready to me yet.
Actually, I'm not sure this is true.
The problem has been mentioned before I think.
Just because this update function doesn't change the structure doesn't mean that one in the future won't and then you have a problem.
Instead, we should just explicitly document to never use entity API IMHO. And not do so in the second update function.
Working with $key here seems wrong.
$key can be anything. It's the name of the context in the condition plugin. You can name that @banana, the context mapping API/UI will map that based on the configuration/type.
Instead, you need to use $new_context_id[0] and obviously do the explode first. There's also no guarantee that there actually is a . in there, this wasn't ensured by the API.
Also, $new_context_id really isn't a good name for this. I'd just use list($old_context_prefix, $old_context_id) instead and adjust the documentation accordingly.
The documentation here needs to be clearer, update the examples according to the suggested variable names above.
Hm, what about?
// Mark the resulting configuration as trusted data. This avoids issues with future schema changes.
Is it worth optimizing this to only save blocks that actually had something changed?
I thought we discussed in some issue that using t() in update functions is pretty pointless because you won't have the translation for it imported by the time the update runs. But I don't really care...
Same here, if the block was already disabled, there's no point in re-saving ?
Comment #175
dawehnerI see what you mean:
So node.node is the actual context ID and the key is coming from the form?
Comment #176
BerdirThe key is for example from the NodeType condition plugin. That's node for that, but to get back to my example, you can also write a condition plugin that has
Then the the config would be "banana: node.node".
Comment #177
dawehnerSo you fear about changes of the update function in between 8001 and 8002? Maybe should document that. On the other hand I could totally imagine
that contrib module code does special things on status flags changes, like hide something else based upon that. If we don't trigger the entity events we show other related stuff really easy.
Comment #178
BerdirNo, I fear about changes in 8005, that will for example add a new property to block config entities, or rename one. And then we resave here without that or with the old property name, then you have some code or a hook that relies on this. Many possibilities to mess up your site.
Yes, maybe someone would want to do something if a block is disabled, but then they have to write their own update to handle this.
It's the same for entity API in 7.x, that falls apart if you change the schema in a later update. I really think the less side-effects you have in update functions, the better. It's also a lot easier to document and follow imho: "Never use entity API" is a lot easier to understand and follow than "sometimes you can use it and sometimes not".
Comment #179
catchI think this is covered by #102.
block_update_8002() just updates blocks based on the information we stored in block_update_8001().
So when block_update_8005() arrives, there are two types of sites;
1. Sites that have already run block_update_8001() and block_update_8002()
2. Sites that are going to run block_update_8001() all the way to block_update_8005() in one go.
We replace the contents of block_update_8002() with something set in state or key value - so that it's possible to differentiate between #1 and #2.
Then the contents of block_update_8002() get moved to block_update_8006() - allowing block_update_8001() and block_update_8005() a clear run to update the block structure without any config saves.
Once block_update_8005() has run, it's OK to use the config API to save blocks again.
This is very convoluted, but we can also reduce the likelihood of situations like this by clearing out updates every minor release and implementing hook_update_last_removed() so at least the list of consecutive updates doesn't grow endlessly.
Comment #180
BerdirHm. IMHO, if 8002 uses a (non-entity) config save with trusted data, then everything just works? 8001, 8002 and 8005 all do a partial update for whatever they need and the data is fine at the end? We invoke config events, but we document those that they need to be able to handle cases like this.
#179 sounds quite complicated to me, and doesn't solve cases where e.g. a contrib module updates its block settings using entity API between 8002 and 8005?
Comment #181
dawehnerI totally agree with berdir that we should reduce the surface of attack as much as possible. Opening up entity API is quite bug, if you are honest,
especially because this also "tells" people that using entity API for content entities might be fine.
Was something similar implementation in the D7 update path?
So I decided to provide a full mapping of the old context ID to the entire full new one. It just makes the code better to read, IMHO
I didn't got one. Nothing yet disabled the block. Do yo mind to explain it a bit?
Comment #182
catchI've opened #2540416: Move update.php back to a front controller to talk about entity API in updates and related. I'm OK with using config saves here and avoiding config entities until we flesh that out more.
Comment #183
dawehnerAfter some discussion around #2540416: Move update.php back to a front controller let's remove the comment about entity API for now.
Comment #184
Berdirnitpick: shouldn't this have a . at the end?
the first entry seems to be bogus and not actually in the required format? The loop is what defines the actual language contexts?
I haven't reviewed the tests in detail, but the update functions look good to me now. Just a bunch of nitpicks and then I think I can RTBC this.
I'd just write "// Replace the context ID based on the defined mapping.". Those examples IMHO confuse more than it helps. Definitely like to just hardcode it instead of doing some magic things.
Comment #185
dawehnerThere we go.
Comment #186
catchAdded #2528178: Provide an upgrade path for blocks context IDs #2354889 (context manager) as a major follow-up for jiban's point in #172.
Comment #187
jibranI think @catch means #2540546: Add a test implementation of the contrib update path for #2528178
Comment #188
BerdirOk. I've reviewed the change record one more time, made some small changes, looks OK to me now. We have the follow-up to actually implement and test it.
I think this is really ready now. We have follow-ups for everything that we're not sure about but doesn't have to block this/beta13.
This was RTBC'd 8 times already. Let's see if the 9th is going to stick :)
Comment #189
dawehnerLet's also add the optimization.
Comment #190
dawehnerCopied the documentation of berdir.
Comment #191
catchComment #193
catchLast few changes look good. We have several follow-ups for this, and sadly more updates to write before an RC. So committed/pushed to 8.0.x, thanks!
(I had to resist putting the last bit in block caps).
Comment #194
jhodgdonI just updated https://www.drupal.org/node/2535316 (the page on how to make update functions for config changes) to agree with what was just committed here.
Comment #195
XanoFollow-up: #2547581: Missing configuration schemas for condition plugins.