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:

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

  3. Updates which renames the keys
    Renaming keys should be doable in the sense, that we need to ensure that the schema is updated.
  4. Updates which require basically both at the same time

Remaining tasks

User interface changes

API changes

Data model changes

Screenshot

CommentFileSizeAuthor
#190 interdiff.txt600 bytesdawehner
#190 2528178-190.patch17.24 KBdawehner
#189 interdiff.txt786 bytesdawehner
#189 2528178-189.patch17.26 KBdawehner
#185 interdiff.txt1.31 KBdawehner
#185 2528178-185.patch17.14 KBdawehner
#183 interdiff.txt1003 bytesdawehner
#183 2528178-183.patch17.25 KBdawehner
#181 interdiff.txt4.71 KBdawehner
#181 2528178-179.patch17.49 KBdawehner
#171 interdiff.txt2.35 KBdawehner
#171 2528178-171.patch16.74 KBdawehner
#169 interdiff.txt613 bytesdawehner
#169 2528178-169.patch16.7 KBdawehner
#163 2528178-161.patch16.69 KBdawehner
#161 interdiff.txt729 bytesdawehner
#161 2528178-161.patch16.7 KBdawehner
#157 interdiff.txt628 bytesamateescu
#157 2528178-157.patch16.66 KBamateescu
#155 interdiff.txt3.46 KBdawehner
#155 2528178-155.patch16.67 KBdawehner
#152 2528178-block-152.patch16.83 KBtim.plunkett
#149 interdiff.txt681 bytesdawehner
#149 2528178-149.patch16.8 KBdawehner
#147 interdiff.txt2.42 KBamateescu
#147 2528178-147.patch16.79 KBamateescu
#143 interdiff.txt4.65 KBamateescu
#143 2528178-143.patch16.91 KBamateescu
#141 interdiff.txt955 bytesdawehner
#141 2528178-141.patch16.81 KBdawehner
#139 interdiff.txt4.25 KBdawehner
#139 interdiff-all.txt4.25 KBdawehner
#139 2528178-139.patch17.74 KBdawehner
#135 interdiff.txt3.91 KBdawehner
#135 2528178-135.patch15.63 KBdawehner
#126 interdiff.txt2.72 KBdawehner
#122 interdiff.txt775 bytesdawehner
#122 2528178-122.patch14.55 KBdawehner
#121 interdiff.txt5.43 KBdawehner
#121 2528178-121.patch14.65 KBdawehner
#114 interdiff.txt2.55 KBdawehner
#114 2528178-114.patch13.8 KBdawehner
#112 interdiff.txt2.65 KBdawehner
#112 2528178-112.patch13.65 KBdawehner
#108 interdiff.txt2.27 KBdawehner
#108 2528178-108.patch13.64 KBdawehner
#105 interdiff.txt3.37 KBdawehner
#105 2528178-105.patch13.63 KBdawehner
#100 interdiff.txt2.22 KBdawehner
#100 2528178-100.patch13.58 KBdawehner
#86 interdiff.txt763 bytesdawehner
#86 2528178-86.patch12.24 KBdawehner
#83 interdiff.txt11.07 KBdawehner
#83 2528178-83.patch12.24 KBdawehner
#80 2528178-80.patch11.99 KBdawehner
#78 interdiff.txt5.17 KBdawehner
#78 2528178-78.patch42.08 KBdawehner
#78 Screen Shot 2015-07-21 at 18.24.06.png214.16 KBdawehner
#71 interdiff.txt1.04 KBdawehner
#71 2528178-71.patch11.5 KBdawehner
#69 interdiff.txt1.29 KBdawehner
#69 2528178-69.patch11.48 KBdawehner
#67 interdiff.txt503 bytesdawehner
#67 2528178-65.patch11.4 KBdawehner
#61 interdiff.txt3.63 KBdawehner
#61 2528178-61.patch11.89 KBdawehner
#54 interdiff.txt503 bytesdawehner
#54 2528178-54.patch13.05 KBdawehner
#52 interdiff.txt2.64 KBdawehner
#52 2528178-52.patch12.56 KBdawehner
#50 interdiff.txt2.75 KBdawehner
#50 2528178-50.patch11.8 KBdawehner
#26 interdiff.txt6.06 KBamateescu
#26 2528178-26.patch10.97 KBamateescu
#21 2528178-21.patch8.92 KBamateescu
#18 block-context-upgrade-2528178.do-not-test.patch8.92 KBlarowlan
#18 block-context-upgrade-2528178.18.patch67.88 KBlarowlan
#18 interdiff.txt4.27 KBlarowlan
#16 block-context-upgrade-2528178.do-not-test.patch7.49 KBlarowlan
#16 block-context-upgrade-2528178.16.patch66.45 KBlarowlan
#16 interdiff.txt1.52 KBlarowlan
#14 block-context-upgrade-2528178.do-not-test.patch7.02 KBlarowlan
#14 block-context-upgrade-2528178.14.patch65.98 KBlarowlan
#14 interdiff.txt2.64 KBlarowlan
#11 interdiff.txt491 bytesjibran
#6 main_patch_plus_upgrade_path.patch64.99 KBamateescu
#6 2528178.patch6.03 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue tags: +D8 upgrade path
xjm’s picture

Title: Provide an upgrade path for 2354889 » Provide an upgrade path for #2354889 (block context manager)
Assigned: Unassigned » amateescu
jibran’s picture

catch’s picture

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

xjm’s picture

Issue tags: +Configuration system
amateescu’s picture

Status: Active » Needs review
Issue tags: -Needs tests
FileSize
6.03 KB
64.99 KB

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

The last submitted patch, 6: 2528178.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 6: main_patch_plus_upgrade_path.patch, failed testing.

dawehner’s picture

I 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!

+++ b/core/modules/block/block.install
@@ -0,0 +1,35 @@
+  $context_service_id_map = [
+    'language' => 'language.current_language_context',
+    'node' => 'node.node_route_context',
+    'user' => 'user.current_user_context',
+  ];
...
+        foreach ($condition['context_mapping'] as $key => $context) {
+          $new_context_id = explode('.', $context, 2);
+          $condition['context_mapping'][$key] = '@' . $context_service_id_map[$key] . ':' . $new_context_id[1];
+        }

We should throw an exception for non existing entries, so that contrib could recover and fix their contexts ...

The last submitted patch, 6: main_patch_plus_upgrade_path.patch, failed testing.

jibran’s picture

FileSize
491 bytes

interdiff to fix update hook docs.

larowlan’s picture

  1. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    @@ -0,0 +1,77 @@
    + * @see https://www.drupal.org/node/2354889
    

    Do we normally do this?

  2. +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    @@ -0,0 +1,86 @@
    +  'visibility' => [
    

    In many instances $visibility is null, eg see block.block.bartik_search.yml - we should make sure that doesn't error either

larowlan’s picture

Assigned: amateescu » larowlan

prodding

larowlan’s picture

Assigned: larowlan » amateescu
Status: Needs work » Needs review
FileSize
2.64 KB
65.98 KB
7.02 KB

addresses 9, 11, 12

dawehner’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,47 @@
    +/**
    + * @addtogroup updates-8.0.x-beta
    + * @{
    + */
    ...
    +/**
    + * @} End of "addtogroup updates-8.0.x-beta".
    + */
    

    Not a bad idea ...

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,47 @@
    +
    +  $config_factory = \Drupal::configFactory();
    +  foreach ($config_factory->listAll('block.block.') as $block_config_name) {
    +    $block = $config_factory->getEditable($block_config_name);
    +
    

    Should we explain why we deal with the config objects directly?

  3. +++ b/core/modules/block/block.install
    @@ -0,0 +1,47 @@
    +          if (!isset($context_service_id_map[$key])) {
    +            throw new \UnexpectedValueException(sprintf('Encountered an unexpected context mapping key %s', $key));
    +          }
    

    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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
66.45 KB
7.49 KB

Fixes #15

@catch or @xjm - could you add @jibran to the commit credits as I used his interdiff.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Given that @amateescu tested it manually I think this is good as far as it is.

larowlan’s picture

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

tim.plunkett’s picture

Stupid question: now that they're both RTBC, why not merge them?

amateescu’s picture

Status: Reviewed & tested by the community » Postponed

Discussed with @dawehner and @Berdir, we think that getting it in separately is better for review-ability reasons :)

amateescu’s picture

Status: Postponed » Reviewed & tested by the community
FileSize
8.92 KB

The parent issue is in, re-uploading the last patch.

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,61 @@
    + * @addtogroup updates-8.0.x-beta
    

    This is handy. Should be 8.0.0-beta though no?

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,61 @@
    +    // We deal with config objects directly to avoid the config entity API.
    

    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.

  3. +++ b/core/modules/block/block.install
    @@ -0,0 +1,61 @@
    +            $message = \Drupal::translation()->translate('Encountered an unexpected context mapping key, one or more mappings could not be updated. Please manually review your block placements.');
    

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

  4. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    --- /dev/null
    +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    

    Glad this still works, phew.

amateescu’s picture

Status: Needs review » Needs work

Thanks for the review! Working on it now.

catch’s picture

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

Berdir’s picture

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

amateescu’s picture

Assigned: amateescu » Unassigned
Status: Needs work » Needs review
FileSize
10.97 KB
6.06 KB

Addressed all points from #22/#24.. hopefully :)

jibran’s picture

This is handy. Should be 8.0.0-beta though no?

In d7 we have "addtogroup updates-7.x-extra" that's why I added @addtogroup updates-8.0.x-beta.

xjm’s picture

(Adding @jibran to the issue credit per #16.)

dawehner’s picture

In d7 we have "addtogroup updates-7.x-extra" that's why I added @addtogroup updates-8.0.x-beta.

Good idea indeed!

+++ b/core/modules/block/block.install
@@ -53,9 +66,19 @@ function block_update_8001() {
+    $message = \Drupal::translation()->translate('Encountered an unexpected context mapping key, one or more mappings could not be updated. Please manually review your visibility settings for the following blocks:');
+    $message .= '<ul>';
+    foreach ($disabled_blocks as $disabled_block_id => $disabled_block_label) {
+      $message .= '<li>' . $disabled_block_label . ' (' . \Drupal::translation()->translate('Visibility') . ': ' . implode(', ', $problematic_visibility_plugins[$disabled_block_id]) . ')</li>';
+    }
+    $message .= '</ul>';

What about using an inline template?

amateescu’s picture

What about using an inline template?

No opinion really, waiting for more people to chime in.

catch’s picture

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

catch’s picture

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

catch’s picture

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

larowlan’s picture

Playing devil's advocate - do we know of any context providers outside core?
We might be overthinking this.

Berdir’s picture

AFAIK, 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 ;)

andypost’s picture

Also glad to get more opinions about language context provider #2529616: Move CurrentLanguageContext to Core\Language\ContextProvider

jibran’s picture

Status: Needs review » Needs work

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

  1. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    @@ -0,0 +1,93 @@
    + * @see https://www.drupal.org/node/2354889
    

    Let's add @see to update hook instead of issue id.

  2. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    @@ -0,0 +1,93 @@
    +class BlockContextMappingUpdateTest extends UpdatePathTestBase {
    

    It's already in the block namespace let's remove block prefix form the class name.

  3. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    @@ -0,0 +1,93 @@
    +      __DIR__ . '/../../../../system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php',
    

    Let's convert this file to gzip as well. Is it a good idea to move this file to block module?

webchick’s picture

Issue tags: +beta target

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

mpdonadio’s picture

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

dawehner’s picture

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

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

    Once we dealt with them in one release we could write another update function for the next release to got rid of them.

  2. Updates which renames the keys
    Renaming keys should be doable in the sense, that we need to ensure that the schema is updated.
  3. Updates which require basically both at the same time
Wim Leers’s picture

Issue tags: +blocker

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

mpdonadio’s picture

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

webchick’s picture

I 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

catch’s picture

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

webchick’s picture

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

dawehner’s picture

Issue summary: View changes

We talked about that on the d8 critical call.

Moved the thing in #40 and updated with the current description.

dawehner’s picture

Issue summary: View changes
jhodgdon’s picture

Regarding 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

dawehner’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.8 KB
2.75 KB

Here is an update of the patch itself. Neither the patch nor the test is tested yet.

Status: Needs review » Needs work

The last submitted patch, 50: 2528178-50.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
12.56 KB
2.64 KB

Ah, this time we actually store the value.

Status: Needs review » Needs work

The last submitted patch, 52: 2528178-52.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.05 KB
503 bytes

There we go.

lauriii’s picture

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

dawehner’s picture

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

lauriii’s picture

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

jhodgdon’s picture

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

jhodgdon’s picture

Let's continue the discussion about docs on the other issue though? #2521776: Update documentation for hook_update_N() for Drupal 8

effulgentsia’s picture

+++ b/core/modules/block/config/schema/block.schema.yml
@@ -30,6 +30,12 @@ block.block.*:
+    visibility_2528178:
+++ b/core/modules/block/src/Entity/Block.php
@@ -48,6 +48,7 @@
+ *     "visibility_2528178",
+++ b/core/modules/block/src/Entity/Block.php
@@ -99,6 +100,13 @@ class Block extends ConfigEntityBase implements BlockInterface, EntityWithPlugin
+  protected $visibility_2528178 = [];

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

dawehner’s picture

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

Status: Needs review » Needs work

The last submitted patch, 61: 2528178-61.patch, failed testing.

Wim Leers’s picture

#61: oh, that looks so much better already!

xjm’s picture

Using state kv store generally makes a ton of sense. (edited)

+++ b/core/modules/block/src/Tests/BlockStorageUnitTest.php
@@ -104,6 +104,7 @@ protected function createTests() {
+      'visibility_2528178' => array(),

I think this is one place we missed removing the protected property? Edit: and also why the test failed.

Wim Leers’s picture

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,95 @@
    +  $context_service_id_map = [
    +    'language' => 'language.current_language_context',
    +    'node' => 'node.node_route_context',
    +    'user' => 'user.current_user_context',
    +  ];
    

    I think a comment like:

    // These 3 are all the contexts that Drupal core provides.
    

    would be helpful, because it'd document why only those 3 are listed.

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,95 @@
    +            // fix their block placements manually OR alternatively contrib
    

    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?

  3. +++ b/core/modules/block/src/Tests/BlockStorageUnitTest.php
    @@ -104,6 +104,7 @@ protected function createTests() {
    +      'visibility_2528178' => array(),
    

    I think this is no longer necessary as of the last patch? Pretty sure this is also the cause of the single test failure :)

  4. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    --- /dev/null
    +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    
    +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    @@ -0,0 +1,158 @@
    
    @@ -0,0 +1,158 @@
    +<?php
    +
    +/**
    + * @file
    + * Contains database additions to drupal-8.bare.standard.php.gz for testing the
    + * upgrade path of https://www.drupal.org/node/2354889.
    + */
    

    Out of scope for this issue, but I think this is extremely painful.

xjm’s picture

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,95 @@
    +  // We deal with config objects directly in order to avoid invoking Entity API
    +  // hooks. They can be problematic when two modules implement a specific hook,
    +  // their code is updated at the same time, yet the update functions run
    +  // sequentially so the second hook implementation can not rely on the data not
    +  // being altered by the first hook.
    

    Many plus for stating this so clearly in the inline docs.

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,95 @@
    +            // Remove the visibility condition for unknown context mapping
    +            // entries, so the update process itself runs through and users can
    +            // fix their block placements manually OR alternatively contrib
    +            // modules can run their own update functions to update mappings
    +            // that they provide.
    

    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?

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
503 bytes

Thanks xjm :)

I think this is one place we missed removing the protected property? Edit: and also why the test failed.

This fixes it for me.

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?

Ideally I think we would have some list of available backups and not have to do that manually for every hook_update_N()

Fabianx’s picture

+1 to using K/V here.

Besides #65 and #66 I think we are good to RTBC again :).

dawehner’s picture

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?

Mh, 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?

Out of scope for this issue, but I think this is extremely painful.

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.

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?

Tried to improve the message a bit:

'Encountered an unexpected context mapping key, one or more mappings could not be updated. Please manually review your visibility settings for the following blocks, which are disabled now:'

Status: Needs review » Needs work

The last submitted patch, 69: 2528178-69.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
11.5 KB
1.04 KB

/me sighs

This will pass, I'm "convinced".

Wim Leers’s picture

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?

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

dawehner’s picture

It does mean though that the operations performed by context providing contrib modules do need to be idempotent though.

Well, they are converting contexts over, and if they actually care remove duplicates. This is all, right?

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.

Yeah, core can just take care about core. Too bad, we haven't invented an actual brain with Drupal 8.

Berdir’s picture

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

tim.plunkett’s picture

Wow, 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

catch’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +            $problematic_visibility_plugins[$block->get('id')][] = $condition_plugin_id_ui_label_map[$condition_plugin_id];
    

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

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +        // This block will have an invalid context mapping service and must be
    +        // disabled to avoid the 'You must provide the context IDs in the
    +        // @{service_id}:{unqualified_context_id} format' exception.
    +        $block->set('status', FALSE);
    

    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.

  3. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +      $message .= '<li>' . $disabled_block_label . ' (' . \Drupal::translation()->translate('Visibility') . ': ' . implode(', ', $problematic_visibility_plugins[$disabled_block_id]) . ')</li>';
    

    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?

jhodgdon’s picture

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

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +  $condition_plugin_id_ui_label_map = [
    +    'node_type' => \Drupal::translation()->translate('Content types'),
    +    'request_path' => \Drupal::translation()->translate('Pages'),
    +    'user_role' => \Drupal::translation()->translate('Roles'),
    +  ];
    

    This block of code could use a comment because I still am not really sure what it means.

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +  $disabled_blocks = $problematic_visibility_plugins = $backup_values = [];
    +  // We deal with config objects directly in order to avoid invoking Entity API
    +  // hooks. They can be problematic when two modules implement a specific hook,
    +  // their code is updated at the same time, yet the update functions run
    +  // sequentially so the second hook implementation can not rely on the data not
    +  // being altered by the first hook.
    +
    +  $update_backup = [];
    +  foreach ($config_factory->listAll('block.block.') as $block_config_name) {
    

    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?

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +    $message = \Drupal::translation()->translate('Encountered an unexpected context mapping key, one or more mappings could not be updated. Please manually review your visibility settings for the following blocks, which are disabled now:');
    

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

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,96 @@
    +      $message .= '<li>' . $disabled_block_label . ' (' . \Drupal::translation()->translate('Visibility') . ': ' . implode(', ', $problematic_visibility_plugins[$disabled_block_id]) . ')</li>';
    

    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?

  3. +++ b/core/modules/block/src/Tests/Update/BlockContextMappingUpdateTest.php
    @@ -0,0 +1,95 @@
    +   * Tests that block context mapping are updated properly.
    

    nitpick: mappings are or mapping is, not mapping are

  4. Just a note on the ugliness of the PHP file used to set up the db for the test... This other issue did it better -- by exporting the config into text files and then using a few lines of PHP to parse and save to the database. Much easier to maintain, I think?
    #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter
dawehner’s picture

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

I went with the label on the condition plugin itself.

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.

Well, yeah, I think so.

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?

Good point, let's move things around.

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?

Well, this just happens in case there is contrib going on.

Can we see an example of what this error message would actually look like to a user?

Just used the screenshot from simpletest :)

Just a note on the ugliness of the PHP file used to set up the db for the test... This other issue did it better -- by exporting the config into text files and then using a few lines of PHP to parse and save to the database. Much easier to maintain, I think?

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.

jibran’s picture

Status: Needs review » Needs work

Needs rebase

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.99 KB

Sure, let's do that quickly.

jhodgdon’s picture

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

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,102 @@
    +  // \Drupal\node\Plugin\Condition\NodeType::buildConfigurationForm.
    

    Nitpick: end buildConfigurationForm with ().

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,102 @@
    +  $condition_plugin_id_ui_label_map = [
    +    'node_type' => \Drupal::translation()->translate('Content types'),
    +    'request_path' => \Drupal::translation()->translate('Pages'),
    +    'user_role' => \Drupal::translation()->translate('Roles'),
    +  ];
    +
    +  $condition_plugin_id_label_map = array_map(function($definition) {
    +    return $definition['label'];
    +  }, \Drupal::service('plugin.manager.condition')->getDefinitions());
    +
    

    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?

  3. +++ b/core/modules/system/tests/fixtures/update/drupal-8.block-context-manager-2354889.php
    @@ -0,0 +1,158 @@
    +// Structure of a custom block with visibility settings.
    +$block_configs = [[
    +  'uuid' => '9d204071-a923-4707-8200-c298a540fb0c',
    +  'langcode' => 'en',
    +  'status' => TRUE,
    +  'dependencies' => [
    +    'content' => [
    +      'block_content:basic:c1895145-893e-460b-a24e-78cd2cefbb1f',
    +    ],
    +    'module' => [
    +      'block_content',
    +      'node',
    +      'user',
    +    ],
    +    'theme' => [
    +      'bartik',
    +    ],
    +  ],
    +  'id' => 'testfor2354889',
    

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

jibran’s picture

+++ b/core/modules/block/block.install
@@ -0,0 +1,102 @@
+          $new_context_id = explode('.', $context, 2);
+          $condition['context_mapping'][$key] = '@' . $context_service_id_map[$key] . ':' . $new_context_id[1];

Can we mention at someplace in comment that we are replacing 'user' => 'user.current_user' with user: '@user.current_user_context:current_user' or 'node' => 'node.node' with node: '@node.node_route_context:node'? I think having this in the update hook would help contrib developers while looking at the backup keys.

dawehner’s picture

Nitpick: end buildConfigurationForm with ().

Ha, true.

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?

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.

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

Oh yeah I got what you mean now. +1 to that idea

jhodgdon’s picture

Way nicer. :) All of my concerns have been addressed now. Thanks!

jhodgdon’s picture

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

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,101 @@
    + * Install, update and uninstall functions for the Block module.
    

    nitpick: needs comma after "update".

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,101 @@
    +  // Provides a list of plugin lables, keyed by plugin ID.
    

    lables => labels

dawehner’s picture

There we go.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

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

effulgentsia’s picture

Assigned: Unassigned » catch

This looks great to me as well, but as it's our first hook_update_N() for Drupal 8, assigning to catch for review/commit.

jibran’s picture

Any word on #82

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review

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

jhodgdon’s picture

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

jhodgdon’s picture

Actually I don't even think the change notice is correct about the data model change. It definitely needs some attention.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community

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

Fabianx’s picture

Agree, on leaving this RTBC, while docs can still be improved. If this gets in before, could still improve the docs.

catch’s picture

Status: Reviewed & tested by the community » Needs work

OK I'd hoped to give this a final review and commit, but found more issues.

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,101 @@
    +
    +  // We deal with config objects directly in order to avoid invoking Entity API
    +  // hooks. They can be problematic when two modules implement a specific hook,
    +  // their code is updated at the same time, yet the update functions run
    +  // sequentially so the second hook implementation can not rely on the data not
    +  // being altered by the first hook.
    +  $config_factory = \Drupal::configFactory();
    

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

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,101 @@
    +        }
    +      }
    +      if (isset($disabled_blocks[$block->get('id')])) {
    +        // This block will have an invalid context mapping service and must be
    +        // disabled in order to prevent information disclosure.
    +        $block->set('status', FALSE);
    +      }
    +      $block->set('visibility', $visibility);
    +
    

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

  3. On disabled blocks, that gets us to this:

  4. +++ b/core/modules/block/block.install
    @@ -0,0 +1,101 @@
    +
    +            $backup_values[] = $context;
    +            unset($visibility[$condition_plugin_id]);
    +            continue;
    +          }
    

    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.

dawehner’s picture

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.

I just doubt that this will work great in reality. You'll start with updating core, and well contrib is not updated at that point.

catch’s picture

Yep, that's exactly where I got to:

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

catch’s picture

Also #82/#90 are both good ideas.

dawehner’s picture

Hey, drupal.org please don't forget my patches. Well then let's adress #82 and #90 as well.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.58 KB
2.22 KB

I think I took into account now everything.

Fabianx’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/block.install
@@ -0,0 +1,133 @@
+  if ($disabled_blocks) {
+    $message = \Drupal::translation()->translate('Encountered a context mapping key coming probably from a contributed or custom module: One or more mappings could not be updated. Please manually review your visibility settings for the following blocks, which are disabled now:');
+    $message .= '<ul>';
+    foreach ($disabled_blocks as $disabled_block_id => $disabled_block_label) {
+      $message .= '<li>' . Drupal::translation()->translate('@label (Visibility: @plugin_ids)', ['@label' => $disabled_block_label, '@plugin_ids' => implode(', ', $problematic_visibility_plugins[$disabled_block_id])]) . '</li>';
+    }
+    $message .= '</ul>';
+  }

that message should definitely move to 8002. Else contrib cannot fix the data in between 8001 and 8002.

effulgentsia’s picture

+++ b/core/modules/block/block.install
@@ -0,0 +1,133 @@
+ * Note: For this update function its fine to use the entity API, see the
+ * explanation in block_update_8001().
+ *
+ * @see block_update_8001()
+ */
+function block_update_8002() {

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

catch’s picture

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.

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

jhodgdon’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.63 KB
3.37 KB

that message should definitely move to 8002. Else contrib cannot fix the data in between 8001 and 8002.

Really good point. Moved it.

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

Yeah that is a bit sad, that its working in that way.

I'm curious, does this issue still needs change record updates?

jhodgdon’s picture

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

Status: Needs review » Needs work

The last submitted patch, 105: 2528178-105.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.64 KB
2.27 KB

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

jhodgdon’s picture

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

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +  // unknown plugins are disabled and their previous visibility settings are saved
    +  // in key value storage; see change record https://www.drupal.org/node/2527840
    +  // for more explanation.
    

    these lines go over 80 chars (nitpick)

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +  // value to another value. In this update function we though deal with
    

    ==> In this update function, however, we deal with

  3. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +  // For updating the status flag of the block in the next update function we
    +  // though can use Entity API.
    +  $config_factory = \Drupal::configFactory();
    

    ==> ... in the next update function, however, we can use Entity API.

  4. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +          // We replace the previous format of "{$context_id}"
    +          // with "@{$service_id}:{$unqualified_context_id}".
    +          $new_context_id = explode('.', $context, 2);
    

    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.

  5. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +        // We not only store the missing context mappings but also the
    +        // previous block status so contrib modules could update
    +
    +        $update_backup[$block->get('id')] = ['missing_context_ids' => $backup_values, 'status' => $block->get('status')];
    +      }
    

    Comment should end in . and probably drop the extra line between comment and code?

  6. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    + * Disables all blocks which had missing context mappings.
    

    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.

  7. +++ b/core/modules/block/block.install
    @@ -0,0 +1,137 @@
    +  // Note: For this update function its fine to use the entity API, see the
    +  // explanation in block_update_8001().
    +  $block_update_8001 = \Drupal::keyValue('update_backup')->get('block_update_8001', []);
    

    its -> it's
    and the comma should be a ; or a .

xjm’s picture

Assigned: catch » Unassigned

@catch said @jhodgdon's feedback above includes what he was going to post, so unassigning for now to make it clear this is actionable. :)

effulgentsia’s picture

Status: Needs review » Needs work

Setting to needs work, per #109 and #110.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.65 KB
2.65 KB

I hope I didn't skipped something.

Thank you @jhodgdon for your fast feedback.

==> ... in the next update function, however, we can use Entity API.

Hei, it was consistent on being wrong.

node.node@block.node_route_context

Yeah that was earlier versions of the earlier issue. We changed in the meantime. Will work on it after posting the patch.

catch’s picture

I found two more nits which could be either ignored or fixed on commit, very minor:

  1. +++ b/core/modules/block/block.install
    @@ -0,0 +1,136 @@
    +  // For updating the status flag of the block in the next update function,
    +  // however, we can use Entity API.
    

    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.

  2. +++ b/core/modules/block/block.install
    @@ -0,0 +1,136 @@
    +  if (count($blocks) > 0) {
    +    $message = \Drupal::translation()
    +      ->translate('Encountered a context mapping key coming probably from a contributed or custom module: One or more mappings could not be updated. Please manually review your visibility settings for the following blocks, which are disabled now:');
    +    $message .= '<ul>';
    

    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.

dawehner’s picture

Just a good night patch.

jhodgdon’s picture

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

effulgentsia’s picture

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

effulgentsia’s picture

Assigned: Unassigned » catch
Status: Needs review » Reviewed & tested by the community

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

chx’s picture

Assigned: catch » Unassigned
Status: Reviewed & tested by the community » Needs work

Well then this is a good night very quick review. Just a very quick scan, I had a long flight from New York.

  $condition_plugin_id_label_map = array_map(function($definition) {
    return $definition['label'];
  }, \Drupal::service('plugin.manager.condition')->getDefinitions());

What about array_column(\Drupal::service('plugin.manager.condition')->getDefinitions(), 'label') instead? We did go PHP 5.5 so let's use it.

  $disabled_blocks = $problematic_visibility_plugins = $backup_values = [];

is this allowed in the Drupal coding standards these days?

foreach ($visibility as $condition_plugin_id => &$condition) {

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

chx’s picture

Status: Needs work » Reviewed & tested by the community

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

dawehner’s picture

What about array_column(\Drupal::service('plugin.manager.condition')->getDefinitions(), 'label') instead? We did go PHP 5.5 so let's use it.

Oh nice! Yeah we should use PHP 5.5 and educate people by using it.

is this allowed in the Drupal coding standards these days?

No idea, but I don't think its that bad.

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

Love it.

BTW. currently we not use the human readable name for the context IDs, since the extra update function.

dawehner’s picture

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

dawehner’s picture

This time again with

catch’s picture

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

Fabianx’s picture

#122 is still RTBC (x-post with #123)

catch’s picture

@effulgentsia pointed this out on that issue:

What about \Drupal::service('config.storage')->(listAll|read|write)()?

Hesitate to say this, but should the first update here be using that to avoid the event?

dawehner’s picture

FileSize
2.72 KB

This itself doesn't work, the test fails due to some problem on the actual site.

catch’s picture

Status: Reviewed & tested by the community » Needs review

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

Wim Leers’s picture

What about \Drupal::service('config.storage')->(listAll|read|write)()?

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

dawehner’s picture

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

effulgentsia’s picture

That doesn't invalidate cache tags of the affected blocks.

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

This storage relies on config events

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.

tim.plunkett’s picture

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

effulgentsia’s picture

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

Berdir’s picture

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

dawehner’s picture

Does that mean we should config API to import our entries?

dawehner’s picture

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

  • Don't fatal like we do at the moment when a block context is not there.
  • Change the test to use $update_free_access and expect people to do the same.

Status: Needs review » Needs work

The last submitted patch, 135: 2528178-135.patch, failed testing.

effulgentsia’s picture

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

xjm’s picture

Issue tags: +Triaged D8 critical

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.74 KB
4.25 KB
4.25 KB

Anyone 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 that update.php cannot be accessed with just $update_free_access, let's fix that. ...

Fabianx’s picture

  1. +++ b/core/modules/system/system.install
    @@ -526,7 +526,7 @@ function system_requirements($phase) {
    -        'description' => t('The update.php script is accessible to everyone without authentication check, which is a security risk. You must change the @settings_name value in your settings.php back to FALSE.', array('@settings_name' => '$settings[\'update_free_access\']')),
    +        'description' => t('The update.php script is accessible to everyone without authentication check, which is a security risk. You must change the @settings_name value in your settings.php back to FALSE.', array('@settings_name' => '$settings[\'ccess\']')),
    

    uhm?

  2. +++ b/core/modules/system/system.routing.yml
    @@ -454,6 +454,8 @@ system.db_update:
    +  options:
    +    _maintenance_access: TRUE
    

    lol :D

dawehner’s picture

Thank you for your quick feedback!

uhm?

Ups.

lol :D

Finally I actually understood why some of the behaviour I've seen on update.php was so weird.

Fabianx’s picture

I think we are ready again, so I am giving a RTBC + 1.

amateescu’s picture

Just a few cosmetic changes :) I also reviewed the code and I think this is ready.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Then lets make it so.

Berdir’s picture

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

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

Given that this issue was about providing a good example how to exactly do it, I would vote for doing it right here.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.79 KB
2.42 KB

Here we go, back to using the config api + the trusted data feature.

Status: Needs review » Needs work

The last submitted patch, 147: 2528178-147.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.8 KB
681 bytes

Let's fix the failures.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

#147 and #149 implemented what was agreed upon in the critical issue meeting today, so back to RTBC to get committer feedback.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs reroll
+++ b/core/modules/block/block.install
@@ -0,0 +1,131 @@
+  // be executed before block_update_8002().

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

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
16.83 KB
jibran’s picture

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

  1. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    +function block_update_8001() {
    

    We are not returning any string to display after this update. IMO we should return something.

  2. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    + * Disable all blocks from the previous update.
    

    This is not helpful description in UI. What previous update? It can be more elaborating.

  3. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    +  $block_storage = \Drupal::entityManager()->getStorage('block');
    +  $blocks = $block_storage->loadMultiple(array_keys($block_update_8001));
    

    No need to store block storage just make it chained. We are not using it again anywhere in the function AFAICS.

  4. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    +  /** @var \Drupal\Core\Config\Entity\ConfigEntityInterface $block */
    

    $block_storage returns BlockInterface :)

  5. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    +  // Override with the UI labels we are aware of. Sadly they are not machine
    +  // accessible, see
    

    Is there an issue for this to store these UI labels in plugin manager? We store all kind of details for entities.

  6. +++ b/core/modules/block/block.install
    @@ -16,3 +16,128 @@ function block_install() {
    +    $message = \Drupal::translation()
    ...
    +      $message .= '<li>' . Drupal::translation()
    

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

effulgentsia’s picture

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

dawehner’s picture

We are not returning any string to display after this update. IMO we should return something.

Agreed.

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.

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.

Re #153.3: what about using Block::loadMultiple() instead?

Well, I disagree with the usage, because we want to educate people.

Is there an issue for this to store these UI labels in plugin manager? We store all kind of details for entities.

Not sure what you mean with this. Well, its out of scope to change that behaviour, I think.

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

Yeah that is overthinking, IMHO.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 155: 2528178-155.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.66 KB
628 bytes

Let's fix it.

dawehner’s picture

+++ b/core/modules/block/block.install
@@ -140,4 +140,3 @@ function block_update_8002() {
->>>>>>> 147

oh, that is embarrassing.

Fabianx’s picture

Back to RTBC

If we want to get more nit-picky consider running phpcs on it and fix the too long lines, if not ignore:


  1. +++ b/core/modules/block/block.install
    @@ -16,3 +16,127 @@ function block_install() {
    +        $update_backup[$block->get('id')] = ['missing_context_ids' => $backup_values, 'status' => $block->get('status')];
    

    nit - should split across two lines.

  2. +++ b/core/modules/block/block.install
    @@ -16,3 +16,127 @@ function block_install() {
    +  $blocks = \Drupal::entityManager()->getStorage('block')->loadMultiple(array_keys($block_update_8001));
    

    nit - now longer than 80 characters.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
dawehner’s picture

Let's fix the array, this improves readability.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 161: 2528178-161.patch, failed testing.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
16.69 KB

mh

jibran’s picture

Thanks @dawehner everything looks super cool now.

Not sure what you mean with this. Well, its out of scope to change that behaviour, I think.

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.

jibran’s picture

Title: Provide an upgrade path for #2354889 (block context manager) » Provide an upgrade path for blocks context IDs #2354889 (context manager)

Isn't it just a context manager now?

jhodgdon’s picture

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

jhodgdon’s picture

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

dawehner’s picture

Status: Reviewed & tested by the community » Needs work

While 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

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.7 KB
613 bytes

There we go.

Status: Needs review » Needs work

The last submitted patch, 169: 2528178-169.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
16.74 KB
2.35 KB

There we go, let's fix the test failures.

jibran’s picture

+++ b/core/modules/block/block.install
@@ -16,3 +16,130 @@ function block_install() {
+  // Contributed modules should leverage hook_update_dependencies() in order to
+  // be executed before block_update_8002().

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

dawehner’s picture

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

Berdir’s picture

Hm, was hoping to be able to RTBC this but this doesn't look ready to me yet.

  1. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +  // Using the Entity API is fine as long we change the value from one valid
    +  // value to another value. In this update function however, we deal with
    +  // converting the format of the context_mapping, which makes code reacting
    +  // to Entity API hooks tricky, because they would need to be different for
    +  // before/after the update. For updating the status flag of the block in the
    +  // next update function, however, we can use Entity API.
    

    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.

  2. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +        foreach ($condition['context_mapping'] as $key => $context) {
    +          if (!isset($context_service_id_map[$key])) {
    

    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.

  3. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +          // We replace the previous format of "{$context_id}"
    +          // with "@{$service_id}:{$unqualified_context_id}".
    +          $new_context_id = explode('.', $context, 2);
    +          $condition['context_mapping'][$key] = '@' . $context_service_id_map[$key] . ':' . $new_context_id[1];
    

    The documentation here needs to be clearer, update the examples according to the suggested variable names above.

  4. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +    // Use the trusted data feature during hook_update_N() to mitigate issues
    +    // with schema changes.
    +    $block->save(TRUE);
    

    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?

  5. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +  return t('Block context IDs updated.');
    

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

  6. +++ b/core/modules/block/block.install
    @@ -16,3 +16,130 @@ function block_install() {
    +    // This block will have an invalid context mapping service and must be
    +    // disabled in order to prevent information disclosure.
    +    $block->setStatus(FALSE);
    +    $block->save();
    

    Same here, if the block was already disabled, there's no point in re-saving ?

dawehner’s picture

I see what you mean:

visibility:
  node_type:
    id: node_type
    bundles:
      article: article
      page: page
    negate: false
    context_mapping:
      node: node.node

So node.node is the actual context ID and the key is coming from the form?

Berdir’s picture

Status: Needs review » Needs work

The 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

*   context = {
 *     "banana" = @ContextDefinition("entity:node", label = @Translation("Banana node"))
 *   }

Then the the config would be "banana: node.node".

dawehner’s picture

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.

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

Berdir’s picture

No, 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".

catch’s picture

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

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

Berdir’s picture

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.49 KB
4.71 KB

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

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.

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

Same here, if the block was already disabled, there's no point in re-saving ?

I didn't got one. Nothing yet disabled the block. Do yo mind to explain it a bit?

catch’s picture

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

dawehner’s picture

After some discussion around #2540416: Move update.php back to a front controller let's remove the comment about entity API for now.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/block/block.install
    @@ -16,3 +16,129 @@ function block_install() {
    +  // This update function updates blocks for the change from
    +  // https://www.drupal.org/node/2354889
    

    nitpick: shouldn't this have a . at the end?

  2. +++ b/core/modules/block/block.install
    @@ -16,3 +16,129 @@ function block_install() {
    +  // These are all the contexts that Drupal core provides.
    +  $context_service_id_map = [
    +    'language' => 'language.current_language_context',
    +    'node.node' => '@node.node_route_context:node',
    +    'user.current_user' => '@user.current_user_context:current_user',
    +  ];
    +
    +  foreach (array_keys(\Drupal::languageManager()->getDefinedLanguageTypesInfo()) as $language_type_id) {
    +    $context_service_id_map['language.' . $language_type_id] = '@language.current_language_context:' . $language_type_id;
    +  }
    

    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.

  3. +++ b/core/modules/block/block.install
    @@ -16,3 +16,129 @@ function block_install() {
    +          // We replace the previous format of "{$context_id}"
    +          // with "@{$service_id}:{$unqualified_context_id}".
    

    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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.14 KB
1.31 KB

There we go.

catch’s picture

jibran’s picture

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record updates

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

dawehner’s picture

Let's also add the optimization.

dawehner’s picture

Copied the documentation of berdir.

catch’s picture

  • catch committed 6f96411 on
    Issue #2528178 by dawehner, amateescu, larowlan, tim.plunkett, jibran,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

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

jhodgdon’s picture

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

Xano’s picture

Status: Fixed » Closed (fixed)

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