Menu links should use a TranslationWrapper to encapsulate safe translatable strings from YAML files

So this is basically postponed on #2338081: Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated

Discussed with @xjm, @catch, @effulgentsia and @webchick and we agreed that this issue is critical because of the security concerns of passing user test into t().

Well, this is not as simple ... given that we need to discuss what we store in {menu_tree} potentially.

I don't think there is any really debate - we just need to store the serialized title & description.

Well, I get that we need to store it, but this means that we can no longer query it. Does that mean we should throw an exception when we call MenuTreeParameter with a title?

Ah yeah I see, so we just need to clearly communicate that querying by it is pointless. What about throwing an exception?

In genearl this looks great and probably needs some custom test coverage to ensure it doesn't causes issues.

Just some small bits of tests now.

Ah, so we should disallow loading by serialized properties?

Yeah, I mean it doesn't make sense at all, or at least it is not the expected behaviour, because people won't know that the serialized object is stored in there.

Regarding the test failures ... this happens when the update test ship with the OLD db.
I fear that we need to add some checks before we unserialize given that going to update.php might also trigger a menu rebuild.
Does that mean that update.php maybe should not rebuild the menu but just rebuild the router?

Are sure the update function can be that simple? Don't we need to update the values, as things from menu_link_content and maybe even other code
has entered data in there.

One idea, don't use the same value again but rather use title_serialized and description_serialized ... I think making that semantically more clear is also a good idea!

Mh, i'm not convinced that this approach is necessary the best one.

@pwolanin and myself talked and agreed that these two update path tests are wrong. They should setup their DB in a way that it has those test modules installed.

When we talked about it on saturday things seem so obvious but in reality it is not.

So one failure starts with the problem that some part of the test calls $this->rebuildAll(), which rebuilds everything from scratch. This obviously doesn't work
as long you haven't ran update.php

You can skip that rebuilding of course, but then many other things are not up to date.
Here is some progress but I actually think that #21 will result in less issues also on the actual site.

I am actually more convinced now that the trickyness of the early upgrade path and the risk of having a triggered menu rebuild is too dangerous so we need to rename the key.
This should fix the failures of #21

1. +++ b/core/includes/theme.inc
   @@ -1274,7 +1274,8 @@ function template_preprocess_html(&$variables) {
   +    debug($system_path);
   

   Not intended is it?

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -359,6 +357,16 @@ protected function preSave(array &$link, array $original) {
   +    if (isset($link['title'])) {
   +      $link['title_serialized'] = $link['title'];
   +      unset($link['title']);
   +    }
   +    if (isset($link['description'])) {
   +      $link['description_serialized'] = $link['description'];
   +      unset($link['description']);
   +    }
   
   @@ -619,8 +629,17 @@ protected function updateParentalStatus(array $link) {
   +    if (array_key_exists('title_serialized', $link)) {
   +      $link['title'] = $link['title_serialized'];
   +    }
   +    if (array_key_exists('description_serialized', $link)) {
   +      $link['description'] = $link['description_serialized'];
        }
   

   I think this would deserve some comment as to why it is done.

3. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -1241,31 +1276,19 @@ protected static function schemaDefinition() {
   +        'title_serialized' => array(
   +          'description' => 'The title for the link. May be a serialized TranslationWrapper',
   ...
   +        'description_serialized' => array(
              'description' => 'The description of this link - used for admin pages and title attribute.',
   

   Should use a similar comment for description.

Also needs change records. I'll start on those ASAP.

Came up while reviewing the patch for a change notice: Should this still have StringTranslationTrait on MenuLinkDefault? It was removed from the other link classes in #2338081: Local Tasks, Actions, and Contextual links mark strings from derivatives (or alter hooks) as safe and translated.

Added change notice draft at MenuLinkDefault now uses a TranslationWrapper for title and description, title_arguments removed https://www.drupal.org/node/2540620 -- may need more change notices later depending on the extent of change done.

@dawehner - I really disagree with changing the key. At the worst there are notices.
We need to just fix the DB dump

This is not true, we also need to fix tthe update path test itself, this is a rabit hole, just saying.
Also notices is something we should avoid ... especially if its just about renaming properties.
We are now storing something entirely different.

Looks like this has tests and an upgrade path, so removing those tags, but the upgrade path needs a test, so tagging for that.

1. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -141,7 +141,15 @@ protected function setUp() {
   +    // Remove the notices we get due to the menu link rebuild prior to running
   +    // the system updates for the schema change.
   +    foreach ($this->assertions as $key => $assertion) {
   +      if ($assertion['message_group'] == 'Notice' && basename($assertion['file']) == 'MenuTreeStorage.php' && strpos($assertion['message'], 'unserialize(): Error at offset 0') !== FALSE) {
   +        unset($this->assertions[$key]);
   +        $this->deleteAssert($assertion['message_id']);
   +        $this->results['#exception']--;
   +      }
   +    }
   

   If these notices only showed up as an artifact of the test runner, I think this would be fine. But I tried manually testing update.php (via the web ui) after applying this patch, and the notices happened in that case too (not to the screen, but to /admin/reports/dblog), so I think we need to fix that, either by figuring out where a rebuild is happening that shouldn't be, or by temporarily putting an @ in front of the pre-or-during-update-triggered unserialize calls with an @todo linking to an issue (perhaps #2540416: Move update.php back to a front controller) for removing that once we figure out how to.

2. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +    $menu_links = $database->queryRange('SELECT mlid, title, description FROM {menu_tree} ORDER BY mlid ASC', $sandbox['current'], $sandbox['current'] + 50)
   +      ->fetchAllAssoc('mlid');
   +
   +    foreach ($menu_links as $menu_link) {
   +      $menu_link = (array) $menu_link;
   +      // Convert title and description to serialized strings.
   +      $menu_link['title'] = serialize($menu_link['title']);
   +      $menu_link['description'] = serialize($menu_link['description']);
   +
   +      $database->update('menu_tree')
   +        ->fields($menu_link)
   +        ->condition('mlid', $menu_link['mlid'])
   +        ->execute();
   +
   +      $sandbox['current'] += 50;
   +    }
   

   I manually tested an update that included a menu_link_content link as well, and after the update, that one wasn't serialized. Perhaps because $sandbox['current'] += 50; needs to be outside of the foreach loop?

Given that we talk about a security issue here, let's ensure we have some test coverage for it.

Didn't had a look yet at the notices mentioned in #42.1

I can reproduce the notice, happens at the router rebuild at the end of the update. #42.2 might explain that if not every link actually gets updated.

#0 /8.x/core/lib/Drupal/Core/Logger/LoggerChannel.php(108): Drupal\dblog\Logger\DbLog->log(5, '%type: !message...', Array)
#1 /8.x/core/includes/errors.inc(157): Drupal\Core\Logger\LoggerChannel->log(5, '%type: !message...', Array)
#2 /8.x/core/includes/errors.inc(77): _drupal_log_error(Array, false)
#3 /8.x/core/includes/bootstrap.inc(672): _drupal_error_handler_real(8, 'unserialize(): ...', '/Users/catch/Si...', 743, Array)
#4 [internal function]: _drupal_error_handler(8, 'unserialize(): ...', '/Users/catch/Si...', 743, Array)
#5 /8.x/core/lib/Drupal/Core/Menu/MenuTreeStorage.php(743): unserialize('Search')
#6 /8.x/core/lib/Drupal/Core/Menu/MenuTreeStorage.php(721): Drupal\Core\Menu\MenuTreeStorage->loadFullMultiple(Array)
#7 /8.x/core/lib/Drupal/Core/Menu/MenuTreeStorage.php(278): Drupal\Core\Menu\MenuTreeStorage->loadFull('search.view')
#8 /8.x/core/lib/Drupal/Core/Menu/MenuTreeStorage.php(814): Drupal\Core\Menu\MenuTreeStorage->doSave(Array)
#9 /8.x/core/lib/Drupal/Core/Menu/MenuTreeStorage.php(173): Drupal\Core\Menu\MenuTreeStorage->saveRecursive('search.view', Array, Array)
#10 /8.x/core/lib/Drupal/Core/Menu/MenuLinkManager.php(204): Drupal\Core\Menu\MenuTreeStorage->rebuild(Array)
#11 /8.x/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php(66): Drupal\Core\Menu\MenuLinkManager->rebuild()
#12 /8.x/core/lib/Drupal/Core/EventSubscriber/MenuRouterRebuildSubscriber.php(54): Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->menuLinksRebuild()
#13 /8.x/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php(116): Drupal\Core\EventSubscriber\MenuRouterRebuildSubscriber->onRouterRebuild(Object(Symfony\Component\EventDispatcher\Event), 'routing.route_f...', Object(Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher))
#14 /8.x/core/lib/Drupal/Core/Routing/RouteBuilder.php(197): Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('routing.route_f...', Object(Symfony\Component\EventDispatcher\Event))
#15 /8.x/core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php(87): Drupal\Core\Routing\RouteBuilder->rebuild()
#16 /8.x/core/includes/common.inc(1340): Drupal\Core\ProxyClass\Routing\RouteBuilder->rebuild()
#17 /8.x/core/modules/system/src/Controller/DbUpdateController.php(626): drupal_flush_all_caches()
#18 [internal function]: Drupal\system\Controller\DbUpdateController::batchFinished(true, Array, Array, '0 sec')
#19 /8.x/core/includes/batch.inc(420): call_user_func_array(Array, Array)
#20 /8.x/core/includes/batch.inc(89): _batch_finished()
#21 /8.x/core/modules/system/src/Controller/DbUpdateController.php(191): _batch_page(Object(Symfony\Component\HttpFoundation\Request))
#22 [internal function]: Drupal\system\Controller\DbUpdateController->handle('start', Object(Symfony\Component\HttpFoundation\Request))
#23 /8.x/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(129): call_user_func_array(Array, Array)
#24 /8.x/core/lib/Drupal/Core/Render/Renderer.php(568): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#25 /8.x/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(130): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#26 /8.x/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(103): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
#27 [internal function]: Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#28 /8.x/core/vendor/symfony/http-kernel/HttpKernel.php(147): call_user_func_array(Object(Closure), Array)
#29 /8.x/core/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#30 /8.x/core/lib/Drupal/Core/StackMiddleware/Session.php(62): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#31 /8.x/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(53): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#32 /8.x/core/modules/page_cache/src/StackMiddleware/PageCache.php(104): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#33 /8.x/core/modules/page_cache/src/StackMiddleware/PageCache.php(83): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#34 /8.x/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(58): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#35 /8.x/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(67): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#36 /8.x/core/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#37 /8.x/core/lib/Drupal/Core/DrupalKernel.php(575): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#38 /8.x/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#39 {main}

I manually tested the latest iteration and there I no longer see the notice in the logs. They should also come up in the test, so we are fine here, at least regarding that problem.

spelling nits

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkManager.php
   @@ -41,15 +41,11 @@ class MenuLinkManager implements MenuLinkManagerInterface {
   +    // this maybe be a TranslationWrapper object.
   

   may be

2. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +    return t('Menu link conversion skipped, because the {menu_tree} table did not existed yet.');
   

   did not exist

3. +++ b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php
   @@ -0,0 +1,45 @@
   +   * ensures that a menu link does not cause an XSS issue.
   

   Ensures

4. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +        'description' => 'The title for the link. May be a serialized TranslationWrapper',
   

   TranslationWrapper.

Just a quick dreditor review. Looks good overall.

1. +++ b/core/lib/Drupal/Core/Menu/MenuLinkDefault.php
   @@ -67,26 +67,14 @@ public static function create(ContainerInterface $container, array $configuratio
      }
   
   +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -927,15 +931,19 @@ protected function loadLinks($menu_name, MenuTreeParameters $parameters) {
   +      $serialized = array_flip($this->serializedFields());
          foreach ($parameters->conditions as $column => $value) {
   -        if (!is_array($value)) {
   -          $query->condition($column, $value);
   -        }
   -        else {
   +        if (is_array($value)) {
              $operator = $value[1];
              $value = $value[0];
   -          $query->condition($column, $value, $operator);
            }
   +        else {
   +          $operator = '=';
   +        }
   +        if (isset($serialized[$column])) {
   +          $value = serialize($value);
   +        }
   

   Any particular reason array_flip() is used in combination with isset() instead of simply using in_array($column, $serialized) without flipping the array?
   Or even more simply in_array($column, $this->serializedFields())?
   Is this a performance optimization?

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
   @@ -927,15 +931,19 @@ protected function loadLinks($menu_name, MenuTreeParameters $parameters) {
    
   
   +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php
   @@ -67,6 +69,10 @@ public function testRediscover() {
        $this->assertEqual('route_name_2', $tree_element->link->getRouteName());
   +    $title = (string) $tree_element->link->getTitle();
   +    $this->assertFalse($title instanceof TranslationWrapper);
   +    $this->assertIdentical('<script>alert("Welcome to the discovered jungle!")</script>', $title);
   +    $this->assertFalse(SafeMarkup::isSafe($title));
   

   Good to test this like this.

3. +++ b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php
   @@ -0,0 +1,45 @@
   +  /**
   +   * ensures that a menu link does not cause an XSS issue.
   +   */
   

   Start with a capital letter.

4. +++ b/core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php
   @@ -0,0 +1,49 @@
   +    // Ensure that some fields got dropped.
   +    $this->assertFalse(db_field_exists('menu_tree', 'title_arguments'));
   +    $this->assertFalse(db_field_exists('menu_tree', 'title_contexts'));
   

   db_field_exists() is deprecated. Should be replaced with:

   Database::getConnection()->schema()->fieldExists($table, $field);

5. +++ b/core/modules/system/src/Tests/Update/MenuTreeSerializationTitleTest.php
   @@ -0,0 +1,49 @@
   +    // Ensure that all titles/descriptions are unserializable.
   +    $select = \Drupal::database()->select('menu_tree');
   +    $result = $select->fields('menu_tree', ['mlid', 'title', 'description'])
   +      ->execute()
   +      ->fetchAllAssoc('mlid');
   +
   +    foreach ($result as $link) {
   +      unserialize($link->title);
   +      unserialize($link->description);
   +    }
   

   This relies on the fact that unserialize() throws a notice when a string is not unserializable. This looks a bit weird though since it seems like nothing is asserted.

   Maybe add a line of documentation explaining this?

6. +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
   @@ -199,4 +199,21 @@ protected function runUpdates() {
   +    // Remove the notices we get due to the menu link rebuild prior to running
   +    // the system updates for the schema change.
   +    foreach ($this->assertions as $key => $assertion) {
   +      if ($assertion['message_group'] == 'Notice' && basename($assertion['file']) == 'MenuTreeStorage.php' && strpos($assertion['message'], 'unserialize(): Error at offset 0') !== FALSE) {
   +        unset($this->assertions[$key]);
   +        $this->deleteAssert($assertion['message_id']);
   +        $this->results['#exception']--;
   +      }
   +    }
   

   Yuck :D

7. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +        'type' => 'blob',
   ...
   +
   +      $sandbox['current']++;;
   +    }
   

   Double semicolon.

8. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +    $sandbox['#finished'] = empty($sandbox['max']) ? 1 : ($sandbox['current'] / $sandbox['max']);
   +
   +
   +    if ($sandbox['#finished'] >= 1) {
   

   Two empty lines here.

9. +++ b/core/modules/system/system.install
   @@ -1085,3 +1085,66 @@ function system_schema() {
   +  else {
   +    return t('Menu link conversion skipped, because the {menu_tree} table did not existed yet.');
   +  }
   

   Grammar: ".. did not exist yet .."

10. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/MenuLinkTestWithUnsafeTitle.php
    @@ -0,0 +1,30 @@
    +  /**
    +   * @inheritDoc
    +   */
    

    Should be {@inheritdoc}.

11. +++ b/core/modules/system/tests/modules/menu_test/src/Plugin/Derivative/MenuLinkTestWithUnsafeTitle.php
    @@ -0,0 +1,30 @@
    +  }
    +
    +
    +}
    

    Two empty lines.

Thank you for your reviews @stefan.r and @pfrenssen

Is this a performance optimization?

This is certainly micro opt, there aren't that many conditions, so I doubt you can actually matter, given that this function is a potential one time operation.

As far as I see all reviews have been addressed and there are no outstanding concerns.

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
@@ -199,4 +199,21 @@ protected function runUpdates() {
+    // Remove the notices we get due to the menu link rebuild prior to running
+    // the system updates for the schema change.
+    foreach ($this->assertions as $key => $assertion) {
+      if ($assertion['message_group'] == 'Notice' && basename($assertion['file']) == 'MenuTreeStorage.php' && strpos($assertion['message'], 'unserialize(): Error at offset 0') !== FALSE) {
+        unset($this->assertions[$key]);
+        $this->deleteAssert($assertion['message_id']);
+        $this->results['#exception']--;
+      }
+    }

This is pretty bad but if we have no better way, its better to get this done and deal with this ugliness.

Looking at my change notice draft from earlier, it looks good still as well vs. the current patch AFAIS: https://www.drupal.org/node/2540620

Looks good - some small nits:

1. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentDeriverTest.php
   @@ -67,6 +69,10 @@ public function testRediscover() {
   +    $title = (string) $tree_element->link->getTitle();
   +    $this->assertFalse($title instanceof TranslationWrapper);
   

   I don't think the string cast here is correct. It makes the next assertion pointless.

2. +++ b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php
   @@ -0,0 +1,45 @@
   +    $this->assertRaw(htmlspecialchars('<script>alert("Wild animals")</script>', ENT_QUOTES, 'UTF-8'));
   +    $this->assertRaw(htmlspecialchars('<script>alert("Even more wild animals")</script>', ENT_QUOTES, 'UTF-8'));
   

   $this->assertEscaped()?

3. +++ b/core/modules/system/system.install
   @@ -1160,3 +1160,66 @@ function system_schema() {
   +  if (db_table_exists('menu_tree')) {
   

   This method is deprecated, use $schema->tableExists() instead.

Fixed remarks from #52.

Yay, thanks!

+++ b/core/modules/system/src/Tests/Menu/MenuLinkSecurityTest.php
@@ -38,8 +38,8 @@ public function testMenuLink() {
+    $this->assertEscaped('<script>alert("Wild animals")</script>');
+    $this->assertEscaped('<script>alert("Even more wild animals")</script>');

Nice I didn't knew that this exists.

Yeah I wanted to answer that this is about conditions, so you really should not have tons of them. In case you have, you have bigger problems :)

+++ b/core/modules/system/system.install
@@ -1160,3 +1160,66 @@ function system_schema() {
+    if ($sandbox['#finished'] >= 1) {
+      // Drop unnecessary fields from {menu_tree}.
+      $schema->dropField('menu_tree', 'title_arguments');
+      $schema->dropField('menu_tree', 'title_context');
+    }

Aren't we potentially throwing data away here? Should we warn the user and back it up if there is data here? Core was not using it but contrib/custom could be.

I'm not sure about this so setting to needs review for more opinion.

Well yes theoretically this is indeed true, but on the other hand I think this would have been mainly used by the static yml files, which will get all the information from the drupal_flush_all_caches() at the end.
We could create one gigantic KV entry with all that information though, but the thing is, it is not entirely trivial to figure out which entries are from static yml files, so we might have to store all of them?

@alexpott: In core at least only statically defined items had arguments and context, eg. ViewsMenuLink does not allow entering a context (or argument) with titles. It cannot be ruled out that some contrib/custom did it.

So, anything with {menu_tree}.discovered=1 gets overwritten or purged during MenuTreeStorage::rebuild() anyway. Which means the data loss from the update function would be only for the following conditions:

• Either a contrib override of MenuTreeStorage or MenuLinkManager that doesn't override title_arguments/title_context for some {menu_tree} items whose discovered=1. I think this could be as simple as a MenuLinkManager override that removes those keys from $defaults.
• Or, a contrib menu link plugin that doesn't use discovery (just like menu_link_content doesn't), but that does use t() for translation (unlike menu_link_content). This would not be a best practice, as t() is for translating "the software" as opposed to "the configuration/content", so why would you have link plugins not using discovery but consider the strings to be part of "the software".

If we only wanted to worry about the 2nd case, then I think we could do something like backup data (into \Drupal::keyValue('update_backup')) for links we encounter that have discovered=0 and non-empty title_arguments or title_context.

If we wanted to worry about the 1st case, we'd probably need to back up all links with non-empty title_arguments or title_context.

I think a case could also be made that both cases are edge case enough, that such data loss is acceptable during a beta. The site should have database backups anyway, and could then deal with merging the data from those database backups. Then again, given there's few links with arguments/context anyway, I don't see much of a downside to backing all of them up in the KV.

I have one contrib project: menu_link_config, but as it works like menu_link_content it does not support arguments/contexts.

In general I agree with peter that the probably for arguments/contexts are really low, especially because contexts just make sense for t().

Agreed with @dawehner!

Let's at least not make the readability of the test worse than it has to be.¬

Committed 63596b2 and pushed to 8.0.x. Thanks!

Published https://www.drupal.org/node/2540620 too.

Something is going wrong with these serialized strings and they are not the correct length on PostgreSQL. This looks like the oject that is serialized is not fully serialized for some reason. I'll open a follow-up since this is several days old at this point.

unserialize(): Error at offset 61 of 64 bytes<pre class="backtrace">unserialize('O:48:"Drupal\Core\StringTranslation\TranslationWrapper":3:{s:9:"')
Drupal\system\Tests\Update\MenuTreeSerializationTitleTest->testUpdate()
Drupal\simpletest\TestBase->run(Array)

This broke --die-on-fail for all update tests. #2549045: Update Tests can't run with --die-on-fail

@mradcliffe @pwolanin - It looks like system_update_8001 fails in postgresql as after the update, the title column is still of type varchar(255). Its data, however, appears to be serialized. @mradcliffe - did you create a new ticket to follow-up on this?

As a result, the serialized TranslationWrapper objects are cut off. Almost all titles and descriptions of menu_tree items for the Admin menu look like this:

O:48:"Drupal\Core\StringTranslation\TranslationWrapper":3:{s:9:""

It's not the size of the column that cuts off the serialization. I believe it's a null byte. Priming the column by manually converting it to a bytea (Postgresl's blob) seems to ensure that all the serializations are saved during the update.

It is. I can't seem to find the issue but I remember seeing it.

The problem has to do with a long standing issue that can't just convert varchar to blob effectively in postgres. Implied casting or something.

@epong, follow-up is #2548725: Fix regression to menu serialization in upgrade path that causes thousands of errors in PostgreSQL.

This is why it broke postgres:

+++ b/core/modules/system/src/Tests/Update/UpdatePathTestBase.php
@@ -199,4 +199,21 @@ protected function runUpdates() {
+    // Remove the notices we get due to the menu link rebuild prior to running
+    // the system updates for the schema change.
+    foreach ($this->assertions as $key => $assertion) {
+      if ($assertion['message_group'] == 'Notice' && basename($assertion['file']) == 'MenuTreeStorage.php' && strpos($assertion['message'], 'unserialize(): Error at offset 0') !== FALSE) {
+        unset($this->assertions[$key]);
+        $this->deleteAssert($assertion['message_id']);
+        $this->results['#exception']--;
+      }
+    }

The error is slightly different on MySQL, so the hack didn't catch it.